# [PATCH] Fix memory leak in "connect.c". 6 messages from 2006-09-07 to 2006-09-11. Participants: Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/5502 ## Christian Couder, 2006-09-07 03:59 Subject: [PATCH] Fix memory leak in "connect.c". Message-ID: <20060907055952.dafffe2b.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20060907055952.dafffe2b.chriscool%40tuxfamily.org ``` sq_quote allocates some memory that should be freed. Signed-off-by: Christian Couder --- 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 ``` ## Junio C Hamano, 2006-09-07 04:09 Subject: Re: [PATCH] Fix memory leak in "connect.c". Message-ID: <7vwt8g48rt.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwt8g48rt.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060907055952.dafffe2b.chriscool@tuxfamily.org> ``` Christian Couder 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, 2006-09-11 04:43 Subject: [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) Message-ID: <200609110643.13486.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/200609110643.13486.chriscool%40tuxfamily.org In-Reply-To: <7vwt8g48rt.fsf@assigned-by-dhcp.cox.net> ``` Le jeudi 7 septembre 2006 06:09, Junio C Hamano a écrit : > Christian Couder 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, 2006-09-11 04:59 Subject: [PATCH 1/2] Move add_to_string to "quote.c" and make it extern. Message-ID: <20060911065922.93139e5d.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20060911065922.93139e5d.chriscool%40tuxfamily.org In-Reply-To: <200609110643.13486.chriscool@tuxfamily.org> ``` So that this function may be used in places other than "rsh.c". Signed-off-by: Christian Couder --- 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 ``` ## Christian Couder, 2006-09-11 05:04 Subject: [PATCH 2/2] Fix a memory leak in "connect.c" and die if command too long. Message-ID: <20060911070450.bf4cc778.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20060911070450.bf4cc778.chriscool%40tuxfamily.org In-Reply-To: <20060911065922.93139e5d.chriscool@tuxfamily.org> ``` 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 --- 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 ``` ## Junio C Hamano, 2006-09-11 06:03 Subject: Re: [PATCH 0/2] Command preparation cleanup in "connect.c" Message-ID: <7v3bazndn1.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v3bazndn1.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200609110643.13486.chriscool@tuxfamily.org> ``` Christian Couder 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. ```