* [PATCH] Fix memory leak in "connect.c". @ 2006-09-07 3:59 Christian Couder 2006-09-07 4:09 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Christian Couder @ 2006-09-07 3:59 UTC (permalink / raw) To: Junio Hamano; +Cc: git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix memory leak in "connect.c". 2006-09-07 3:59 [PATCH] Fix memory leak in "connect.c" Christian Couder @ 2006-09-07 4:09 ` Junio C Hamano 2006-09-11 4:43 ` [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) Christian Couder 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2006-09-07 4:09 UTC (permalink / raw) To: Christian Couder; +Cc: git 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). ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) 2006-09-07 4:09 ` Junio C Hamano @ 2006-09-11 4:43 ` Christian Couder 2006-09-11 4:59 ` [PATCH 1/2] Move add_to_string to "quote.c" and make it extern Christian Couder 2006-09-11 6:03 ` [PATCH 0/2] Command preparation cleanup in "connect.c" Junio C Hamano 0 siblings, 2 replies; 6+ messages in thread From: Christian Couder @ 2006-09-11 4:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Move add_to_string to "quote.c" and make it extern. 2006-09-11 4:43 ` [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) Christian Couder @ 2006-09-11 4:59 ` Christian Couder 2006-09-11 5:04 ` [PATCH 2/2] Fix a memory leak in "connect.c" and die if command too long Christian Couder 2006-09-11 6:03 ` [PATCH 0/2] Command preparation cleanup in "connect.c" Junio C Hamano 1 sibling, 1 reply; 6+ messages in thread From: Christian Couder @ 2006-09-11 4:59 UTC (permalink / raw) To: Junio Hamano; +Cc: git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Fix a memory leak in "connect.c" and die if command too long. 2006-09-11 4:59 ` [PATCH 1/2] Move add_to_string to "quote.c" and make it extern Christian Couder @ 2006-09-11 5:04 ` Christian Couder 0 siblings, 0 replies; 6+ messages in thread From: Christian Couder @ 2006-09-11 5:04 UTC (permalink / raw) To: Junio Hamano; +Cc: git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Command preparation cleanup in "connect.c" 2006-09-11 4:43 ` [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) Christian Couder 2006-09-11 4:59 ` [PATCH 1/2] Move add_to_string to "quote.c" and make it extern Christian Couder @ 2006-09-11 6:03 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2006-09-11 6:03 UTC (permalink / raw) To: Christian Couder; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-09-11 6:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-09-07 3:59 [PATCH] Fix memory leak in "connect.c" Christian Couder 2006-09-07 4:09 ` Junio C Hamano 2006-09-11 4:43 ` [PATCH 0/2] Command preparation cleanup in "connect.c" (was: [PATCH] Fix memory leak in "connect.c".) Christian Couder 2006-09-11 4:59 ` [PATCH 1/2] Move add_to_string to "quote.c" and make it extern Christian Couder 2006-09-11 5:04 ` [PATCH 2/2] Fix a memory leak in "connect.c" and die if command too long Christian Couder 2006-09-11 6:03 ` [PATCH 0/2] Command preparation cleanup in "connect.c" Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).