Re: [PATCH v2] commit: fix memory-leak
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Feb 7, 2011, 21:31 UTC
- Message-ID
- <AANLkTimvgDjiLon-2BAwxcYOQGVE9UZPNr637o93-9HQ@mail.gmail.com>
- In-Reply-To
- <AANLkTikr2+OVRU6n+0tA752_x80ir9dQh65RjUp3BxPR@mail.gmail.com>
On Mon, Feb 7, 2011 at 10:12 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 46 quoted lines
> 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);