threads / patch / 5502

patchFix memory leak in "connect.c".

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

## tl;dr

6 messages between Sep 7, 2006 and Sep 11, 2006. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Christian Couder· Sep 7, 2006, 03:59 UTC · lore
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(-)
Show changes to connect.c +3 −2
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
Junio C Hamano· Sep 7, 2006, 04:09 UTC · re: Christian Couder · lore

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

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).

Christian Couder· Sep 11, 2006, 04:43 UTC · re: Junio C Hamano · lore

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

Le jeudi 7 septembre 2006 06:09, Junio C Hamano a écrit :
Show 11 quoted lines
> 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.
Christian Couder· Sep 11, 2006, 04:59 UTC · re: Christian Couder · lore

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

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(-)
Show changes to 3 files +35 −29

quote.c, quote.h, rsh.c

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
Christian Couder· Sep 11, 2006, 05:04 UTC · re: Christian Couder · lore

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

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(-)
Show changes to connect.c +14 −3
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
Junio C Hamano· Sep 11, 2006, 06:03 UTC · re: Christian Couder · lore

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

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.

← back to recent threads