[PATCH] Fix memory leak in "connect.c".

[PATCH] Fix memory leak in "connect.c".

[Christian Couder]

sq_quote allocates some memory that should be freed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 connect.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/connect.c b/connect.c
index 1c6429b..40a1c28 100644
--- a/connect.c
+++ b/connect.c
@@ -697,8 +697,9 @@ int git_connect(int fd[2], char *url, co
 	if (pid < 0)
 		die("unable to fork");
 	if (!pid) {
-		snprintf(command, sizeof(command), "%s %s", prog,
-			 sq_quote(path));
+		char *sq_path = sq_quote(path);
+		snprintf(command, sizeof(command), "%s %s", prog, sq_path);
+		free(sq_path);
 		dup2(pipefd[1][0], 0);
 		dup2(pipefd[0][1], 1);
 		close(pipefd[0][0]);
-- 
1.4.2.g1ccae49

Re: [PATCH] Fix memory leak in "connect.c".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> sq_quote allocates some memory that should be freed.

That's technically correct, but the code you are touching is
immediately before exec() or die() so I chose to be sloppy and
short ;-).

With sq_quote_buf() (which was introduced with 77d604 Oct 10
2005), you should be able to directly write into the command[]
buffer, I think.  The code you touched predates that function
(b10d0e July 8 2005).

[PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".)

[Christian Couder]

Le jeudi 7 septembre 2006 06:09, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > sq_quote allocates some memory that should be freed.
>
> That's technically correct, but the code you are touching is
> immediately before exec() or die() so I chose to be sloppy and
> short ;-).
>
> With sq_quote_buf() (which was introduced with 77d604 Oct 10
> 2005), you should be able to directly write into the command[]
> buffer, I think.  The code you touched predates that function
> (b10d0e July 8 2005).

The following two patches clean "connect.c" up using "add_to_string" 
from "rsh.c". The code is still longer, but there is no leak and it may be 
better to die with an error message than to truncate a too long command.

Shortlog:
	[PATCH 1/2] Move add_to_string to "quote.c" and make it extern.
	[PATCH 2/2] Fix a memory leak in "connect.c" and die if command too
long.

Diffstat:
 connect.c |   17 ++++++++++++++---
 quote.c   |   29 +++++++++++++++++++++++++++++
 quote.h   |    6 ++++++
 rsh.c     |   29 -----------------------------
 4 files changed, 49 insertions(+), 32 deletions(-)

Christian.

[PATCH 1/2] Move add_to_string to "quote.c" and make it extern.

[Christian Couder]

So that this function may be used in places other than "rsh.c".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 quote.c |   29 +++++++++++++++++++++++++++++
 quote.h |    6 ++++++
 rsh.c   |   29 -----------------------------
 3 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/quote.c b/quote.c
index a38786c..200e4e2 100644
--- a/quote.c
+++ b/quote.c
@@ -106,6 +106,35 @@ char *sq_quote_argv(const char** argv, i
 	return buf;
 }
 
+/*
+ * Append a string to a string buffer, with or without shell quoting.
+ * Return true if the buffer overflowed.
+ */
+int add_to_string(char **ptrp, int *sizep, const char *str, int quote)
+{
+	char *p = *ptrp;
+	int size = *sizep;
+	int oc;
+	int err = 0;
+
+	if ( quote ) {
+		oc = sq_quote_buf(p, size, str);
+	} else {
+		oc = strlen(str);
+		memcpy(p, str, (oc >= size) ? size-1 : oc);
+	}
+
+	if ( oc >= size ) {
+		err = 1;
+		oc = size-1;
+	}
+
+	*ptrp  += oc;
+	**ptrp  = '\0';
+	*sizep -= oc;
+	return err;
+}
+
 char *sq_dequote(char *arg)
 {
 	char *dst = arg;
diff --git a/quote.h b/quote.h
index a6c4611..1a29e79 100644
--- a/quote.h
+++ b/quote.h
@@ -33,6 +33,12 @@ extern void sq_quote_print(FILE *stream,
 extern size_t sq_quote_buf(char *dst, size_t n, const char *src);
 extern char *sq_quote_argv(const char** argv, int count);
 
+/*
+ * Append a string to a string buffer, with or without shell quoting.
+ * Return true if the buffer overflowed.
+ */
+extern int add_to_string(char **ptrp, int *sizep, const char *str, int quote);
+
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
  * produced.
diff --git a/rsh.c b/rsh.c
index 07166ad..8a1db45 100644
--- a/rsh.c
+++ b/rsh.c
@@ -8,35 +8,6 @@ #include "cache.h"
 
 #define COMMAND_SIZE 4096
 
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-static int add_to_string(char **ptrp, int *sizep, const char *str, int quote)
-{
-	char *p = *ptrp;
-	int size = *sizep;
-	int oc;
-	int err = 0;
-
-	if ( quote ) {
-		oc = sq_quote_buf(p, size, str);
-	} else {
-		oc = strlen(str);
-		memcpy(p, str, (oc >= size) ? size-1 : oc);
-	}
-
-	if ( oc >= size ) {
-		err = 1;
-		oc = size-1;
-	}
-
-	*ptrp  += oc;
-	**ptrp  = '\0';
-	*sizep -= oc;
-	return err;
-}
-
 int setup_connection(int *fd_in, int *fd_out, const char *remote_prog, 
 		     char *url, int rmt_argc, char **rmt_argv)
 {
-- 
1.4.2.g430e

[PATCH 2/2] Fix a memory leak in "connect.c" and die if command too long.

[Christian Couder]

Use "add_to_string" instead of "sq_quote" and "snprintf", so
that there is no memory allocation and no memory leak.
Also check if the command is too long to fit into the buffer
and die if this is the case, instead of truncating it to the
buffer size.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 connect.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 1c6429b..49251f9 100644
--- a/connect.c
+++ b/connect.c
@@ -599,12 +599,13 @@ static void git_proxy_connect(int fd[2],
 	close(pipefd[1][0]);
 }
 
+#define MAX_CMD_LEN 1024
+
 /*
  * Yeah, yeah, fixme. Need to pass in the heads etc.
  */
 int git_connect(int fd[2], char *url, const char *prog)
 {
-	char command[1024];
 	char *host, *path = url;
 	char *end;
 	int c;
@@ -697,8 +698,18 @@ int git_connect(int fd[2], char *url, co
 	if (pid < 0)
 		die("unable to fork");
 	if (!pid) {
-		snprintf(command, sizeof(command), "%s %s", prog,
-			 sq_quote(path));
+		char command[MAX_CMD_LEN];
+		char *posn = command;
+		int size = MAX_CMD_LEN;
+		int of = 0;
+
+		of |= add_to_string(&posn, &size, prog, 0);
+		of |= add_to_string(&posn, &size, " ", 0);
+		of |= add_to_string(&posn, &size, path, 1);
+
+		if (of)
+			die("command line too long");
+
 		dup2(pipefd[1][0], 0);
 		dup2(pipefd[0][1], 1);
 		close(pipefd[0][0]);
-- 
1.4.2.g430e

Re: [PATCH 0/2] Command preparation cleanup in "connect.c"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> The following two patches clean "connect.c" up using "add_to_string" 
> from "rsh.c". The code is still longer, but there is no leak and it may be 
> better to die with an error message than to truncate a too long command.

Nice, clean, simple and obviously correct.

Thanks.