* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.