* [PATCH/RFC] commit: fix memory-leak @ 2011-02-07 18:40 Erik Faye-Lund 2011-02-07 18:48 ` Matthieu Moy 0 siblings, 1 reply; 6+ messages in thread From: Erik Faye-Lund @ 2011-02-07 18:40 UTC (permalink / raw) To: git; +Cc: msysgit, blees The name, email and date strings are some times allocated on the heap, but not free'd. Fix this by making sure they are allways heap-allocated, so we can safely free the memory. At the same time, this fixes a problem with strict-POSIX getenv implementations. POSIX says "The return value from getenv() may point to static data which may be overwritten by subsequent calls to getenv()", so duplicating the strings is a potential bug. Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com> --- This was found when investigating how to fix UTF-8 support in getenv on Windows. I introduced the xgetenv-function (that returns a pointer that can be passed to free) because I suspect we'll find other similar code-paths. builtin/commit.c | 9 ++++++--- git-compat-util.h | 1 + wrapper.c | 6 ++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 03cff5a..e5a649e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident) { char *name, *email, *date; - name = getenv("GIT_AUTHOR_NAME"); - email = getenv("GIT_AUTHOR_EMAIL"); - date = getenv("GIT_AUTHOR_DATE"); + name = xgetenv("GIT_AUTHOR_NAME"); + email = xgetenv("GIT_AUTHOR_EMAIL"); + date = xgetenv("GIT_AUTHOR_DATE"); if (use_message && !renew_authorship) { const char *a, *lb, *rb, *eol; @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident) date = force_date; strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME)); + free(name); + free(email); + free(date); } static int ends_rfc2822_footer(struct strbuf *sb) diff --git a/git-compat-util.h b/git-compat-util.h index d6d269f..12f111f 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -409,6 +409,7 @@ typedef void (*try_to_free_t)(size_t); extern try_to_free_t set_try_to_free_routine(try_to_free_t); extern char *xstrdup(const char *str); +extern char *xgetenv(const char *name); extern void *xmalloc(size_t size); extern void *xmallocz(size_t size); extern void *xmemdupz(const void *data, size_t len); diff --git a/wrapper.c b/wrapper.c index 8d7dd31..e6173c4 100644 --- a/wrapper.c +++ b/wrapper.c @@ -30,6 +30,12 @@ char *xstrdup(const char *str) return ret; } +char *xgetenv(const char *name) +{ + char *tmp = getenv(name); + return tmp ? xstrdup(tmp) : NULL; +} + void *xmalloc(size_t size) { void *ret = malloc(size); -- 1.7.4.msysgit.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] commit: fix memory-leak 2011-02-07 18:40 [PATCH/RFC] commit: fix memory-leak Erik Faye-Lund @ 2011-02-07 18:48 ` Matthieu Moy 2011-02-07 19:22 ` Erik Faye-Lund 0 siblings, 1 reply; 6+ messages in thread From: Matthieu Moy @ 2011-02-07 18:48 UTC (permalink / raw) To: Erik Faye-Lund; +Cc: git, msysgit, blees Erik Faye-Lund <kusmabite@gmail.com> writes: > At the same time, this fixes a problem with strict-POSIX getenv > implementations. POSIX says "The return value from getenv() may > point to static data which may be overwritten by subsequent calls > to getenv()", so duplicating the strings is a potential bug. ^ not ? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] commit: fix memory-leak 2011-02-07 18:48 ` Matthieu Moy @ 2011-02-07 19:22 ` Erik Faye-Lund 2011-02-07 20:21 ` [PATCH v2] " Erik Faye-Lund 0 siblings, 1 reply; 6+ messages in thread From: Erik Faye-Lund @ 2011-02-07 19:22 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, msysgit, blees On Mon, Feb 7, 2011 at 7:48 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > Erik Faye-Lund <kusmabite@gmail.com> writes: > >> At the same time, this fixes a problem with strict-POSIX getenv >> implementations. POSIX says "The return value from getenv() may >> point to static data which may be overwritten by subsequent calls >> to getenv()", so duplicating the strings is a potential bug. > ^ > not > ? Indeed, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] commit: fix memory-leak 2011-02-07 19:22 ` Erik Faye-Lund @ 2011-02-07 20:21 ` Erik Faye-Lund 2011-02-07 21:12 ` Erik Faye-Lund 0 siblings, 1 reply; 6+ messages in thread From: Erik Faye-Lund @ 2011-02-07 20:21 UTC (permalink / raw) To: git; +Cc: matthieu.moy, msysgit, blees The name, email and date strings are some times allocated on the heap, but not free'd. Fix this by making sure they are allways heap-allocated, so we can safely free the memory. At the same time, this fixes a problem with strict-POSIX getenv implementations. POSIX says "The return value from getenv() may point to static data which may be overwritten by subsequent calls to getenv()", so not duplicating the strings is a potential bug. Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com> --- Fixed typo in commit message, as pointed out by Matthieu Moy. builtin/commit.c | 9 ++++++--- git-compat-util.h | 1 + wrapper.c | 6 ++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 03cff5a..e5a649e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident) { char *name, *email, *date; - name = getenv("GIT_AUTHOR_NAME"); - email = getenv("GIT_AUTHOR_EMAIL"); - date = getenv("GIT_AUTHOR_DATE"); + name = xgetenv("GIT_AUTHOR_NAME"); + email = xgetenv("GIT_AUTHOR_EMAIL"); + date = xgetenv("GIT_AUTHOR_DATE"); if (use_message && !renew_authorship) { const char *a, *lb, *rb, *eol; @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident) date = force_date; strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME)); + free(name); + free(email); + free(date); } static int ends_rfc2822_footer(struct strbuf *sb) diff --git a/git-compat-util.h b/git-compat-util.h index d6d269f..12f111f 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -409,6 +409,7 @@ typedef void (*try_to_free_t)(size_t); extern try_to_free_t set_try_to_free_routine(try_to_free_t); extern char *xstrdup(const char *str); +extern char *xgetenv(const char *name); extern void *xmalloc(size_t size); extern void *xmallocz(size_t size); extern void *xmemdupz(const void *data, size_t len); diff --git a/wrapper.c b/wrapper.c index 8d7dd31..e6173c4 100644 --- a/wrapper.c +++ b/wrapper.c @@ -30,6 +30,12 @@ char *xstrdup(const char *str) return ret; } +char *xgetenv(const char *name) +{ + char *tmp = getenv(name); + return tmp ? xstrdup(tmp) : NULL; +} + void *xmalloc(size_t size) { void *ret = malloc(size); -- 1.7.4.msysgit.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] commit: fix memory-leak 2011-02-07 20:21 ` [PATCH v2] " Erik Faye-Lund @ 2011-02-07 21:12 ` Erik Faye-Lund 2011-02-07 21:31 ` Erik Faye-Lund 0 siblings, 1 reply; 6+ messages in thread From: Erik Faye-Lund @ 2011-02-07 21:12 UTC (permalink / raw) To: git; +Cc: matthieu.moy, msysgit, blees On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote: > The name, email and date strings are some times allocated on the > heap, but not free'd. Fix this by making sure they are allways > heap-allocated, so we can safely free the memory. > > At the same time, this fixes a problem with strict-POSIX getenv > implementations. POSIX says "The return value from getenv() may > point to static data which may be overwritten by subsequent calls > to getenv()", so not duplicating the strings is a potential bug. > > Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com> > --- > Fixed typo in commit message, as pointed out by Matthieu Moy. > > builtin/commit.c | 9 ++++++--- > git-compat-util.h | 1 + > wrapper.c | 6 ++++++ > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 03cff5a..e5a649e 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident) > { > char *name, *email, *date; > > - name = getenv("GIT_AUTHOR_NAME"); > - email = getenv("GIT_AUTHOR_EMAIL"); > - date = getenv("GIT_AUTHOR_DATE"); > + name = xgetenv("GIT_AUTHOR_NAME"); > + email = xgetenv("GIT_AUTHOR_EMAIL"); > + date = xgetenv("GIT_AUTHOR_DATE"); > > if (use_message && !renew_authorship) { > const char *a, *lb, *rb, *eol; > @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident) > date = force_date; > strbuf_addstr(author_ident, fmt_ident(name, email, date, > IDENT_ERROR_ON_NO_NAME)); > + free(name); > + free(email); > + free(date); Hmm, but I'm getting a crash here on Linux. Guess I need to debug a bit... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] commit: fix memory-leak 2011-02-07 21:12 ` Erik Faye-Lund @ 2011-02-07 21:31 ` Erik Faye-Lund 0 siblings, 0 replies; 6+ messages in thread From: Erik Faye-Lund @ 2011-02-07 21:31 UTC (permalink / raw) To: git; +Cc: matthieu.moy, msysgit, blees On Mon, Feb 7, 2011 at 10:12 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote: > On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote: >> The name, email and date strings are some times allocated on the >> heap, but not free'd. Fix this by making sure they are allways >> heap-allocated, so we can safely free the memory. >> >> At the same time, this fixes a problem with strict-POSIX getenv >> implementations. POSIX says "The return value from getenv() may >> point to static data which may be overwritten by subsequent calls >> to getenv()", so not duplicating the strings is a potential bug. >> >> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com> >> --- >> Fixed typo in commit message, as pointed out by Matthieu Moy. >> >> builtin/commit.c | 9 ++++++--- >> git-compat-util.h | 1 + >> wrapper.c | 6 ++++++ >> 3 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/builtin/commit.c b/builtin/commit.c >> index 03cff5a..e5a649e 100644 >> --- a/builtin/commit.c >> +++ b/builtin/commit.c >> @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident) >> { >> char *name, *email, *date; >> >> - name = getenv("GIT_AUTHOR_NAME"); >> - email = getenv("GIT_AUTHOR_EMAIL"); >> - date = getenv("GIT_AUTHOR_DATE"); >> + name = xgetenv("GIT_AUTHOR_NAME"); >> + email = xgetenv("GIT_AUTHOR_EMAIL"); >> + date = xgetenv("GIT_AUTHOR_DATE"); >> >> if (use_message && !renew_authorship) { >> const char *a, *lb, *rb, *eol; >> @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident) >> date = force_date; >> strbuf_addstr(author_ident, fmt_ident(name, email, date, >> IDENT_ERROR_ON_NO_NAME)); >> + free(name); >> + free(email); >> + free(date); > > Hmm, but I'm getting a crash here on Linux. Guess I need to debug a bit... > Ah, it was the force_date-assignment: ---8<--- diff --git a/builtin/commit.c b/builtin/commit.c index e5a649e..1416c13 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -504,7 +504,7 @@ static void determine_author_info(struct strbuf *author_ident) } if (force_date) - date = force_date; + date = xstrdup(force_date); strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME)); free(name); ---8<--- But now I see that I was temporarily(?) struck with insanity: overwriting a heap-allocated pointer with another heap-allocated pointer doesn't fix a memory-leak. So let's add some more calls to free: diff --git a/builtin/commit.c b/builtin/commit.c index e5a649e..bdd0cfb 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -482,6 +482,10 @@ static void determine_author_info(struct strbuf *author_ident) if (!*lb || !*rb || !*eol) die("invalid commit: %s", use_message); + free(name); + free(email); + free(date); + if (lb == a + strlen("\nauthor ")) /* \nauthor <foo@example.com> */ name = xcalloc(1, 1); @@ -497,14 +501,19 @@ static void determine_author_info(struct strbuf *author_ident) const char *lb = strstr(force_author, " <"); const char *rb = strchr(force_author, '>'); + free(name); + free(email); + if (!lb || !rb) die("malformed --author parameter"); name = xstrndup(force_author, lb - force_author); email = xstrndup(lb + 2, rb - (lb + 2)); } - if (force_date) - date = force_date; + if (force_date) { + free(date); + date = xstrdup(force_date); + } strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME)); free(name); ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-07 21:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-07 18:40 [PATCH/RFC] commit: fix memory-leak Erik Faye-Lund 2011-02-07 18:48 ` Matthieu Moy 2011-02-07 19:22 ` Erik Faye-Lund 2011-02-07 20:21 ` [PATCH v2] " Erik Faye-Lund 2011-02-07 21:12 ` Erik Faye-Lund 2011-02-07 21:31 ` Erik Faye-Lund
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).