Re: [PATCH/RFC] commit: fix memory-leak

Erik Faye-Lund [off-list ref] 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/

On Mon, Feb 7, 2011 at 7:48 PM, Matthieu Moy
[off-list ref] wrote:

Erik Faye-Lund [off-list ref] writes:

quoted

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.

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 <redacted>
---
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

On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund [off-list ref] wrote:

Hmm, but I'm getting a crash here on Linux. Guess I need to debug a bit...

On Mon, Feb 7, 2011 at 10:12 PM, Erik Faye-Lund [off-list ref] wrote:

On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund [off-list ref] wrote:

quoted

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 <redacted>
---
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 [off-list ref] */
 			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);