{"thread":{"id":"26437","subject":"[PATCH/RFC] commit: fix memory-leak","startedAt":"2011-02-07T18:40:44Z","lastAt":"2011-02-07T21:31:14Z","messageCount":6,"participants":["Erik Faye-Lund","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"160614","messageId":"1297104044-4684-1-git-send-email-kusmabite@gmail.com","threadId":"26437","inReplyTo":null,"subject":"[PATCH/RFC] commit: fix memory-leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-07T18:40:44Z","receivedAt":"2011-02-07T18:40:44Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"The name, email and date strings are some times allocated on the\nheap, but not free'd. Fix this by making sure they are allways\nheap-allocated, so we can safely free the memory.\n\nAt the same time, this fixes a problem with strict-POSIX getenv\nimplementations. POSIX says \"The return value from getenv() may\npoint to static data which may be overwritten by subsequent calls\nto getenv()\", so duplicating the strings is a potential bug.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n\nThis was found when investigating how to fix UTF-8 support in\ngetenv on Windows. I introduced the xgetenv-function (that returns\na pointer that can be passed to free) because I suspect we'll find\nother similar code-paths.\n\n builtin/commit.c  |    9 ++++++---\n git-compat-util.h |    1 +\n wrapper.c         |    6 ++++++\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 03cff5a..e5a649e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \n-\tname = getenv(\"GIT_AUTHOR_NAME\");\n-\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n-\tdate = getenv(\"GIT_AUTHOR_DATE\");\n+\tname = xgetenv(\"GIT_AUTHOR_NAME\");\n+\temail = xgetenv(\"GIT_AUTHOR_EMAIL\");\n+\tdate = xgetenv(\"GIT_AUTHOR_DATE\");\n \n \tif (use_message && !renew_authorship) {\n \t\tconst char *a, *lb, *rb, *eol;\n@@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\tdate = force_date;\n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n \t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\n+\tfree(name);\n+\tfree(email);\n+\tfree(date);\n }\n \n static int ends_rfc2822_footer(struct strbuf *sb)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d6d269f..12f111f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -409,6 +409,7 @@ typedef void (*try_to_free_t)(size_t);\n extern try_to_free_t set_try_to_free_routine(try_to_free_t);\n \n extern char *xstrdup(const char *str);\n+extern char *xgetenv(const char *name);\n extern void *xmalloc(size_t size);\n extern void *xmallocz(size_t size);\n extern void *xmemdupz(const void *data, size_t len);\ndiff --git a/wrapper.c b/wrapper.c\nindex 8d7dd31..e6173c4 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -30,6 +30,12 @@ char *xstrdup(const char *str)\n \treturn ret;\n }\n \n+char *xgetenv(const char *name)\n+{\n+\tchar *tmp = getenv(name);\n+\treturn tmp ? xstrdup(tmp) : NULL;\n+}\n+\n void *xmalloc(size_t size)\n {\n \tvoid *ret = malloc(size);\n-- \n1.7.4.msysgit.0\n"},{"id":"160616","messageId":"vpqei7jfzq5.fsf@bauges.imag.fr","threadId":"26437","inReplyTo":"1297104044-4684-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC] commit: fix memory-leak","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-02-07T18:48:02Z","receivedAt":"2011-02-07T18:48:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> At the same time, this fixes a problem with strict-POSIX getenv\n> implementations. POSIX says \"The return value from getenv() may\n> point to static data which may be overwritten by subsequent calls\n> to getenv()\", so duplicating the strings is a potential bug.\n                  ^\n                 not\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"160617","messageId":"AANLkTikKZ+2qUMF1T5pP60cUd9Ya3n2mfhTkX6L32zmn@mail.gmail.com","threadId":"26437","inReplyTo":"vpqei7jfzq5.fsf@bauges.imag.fr","subject":"Re: [PATCH/RFC] commit: fix memory-leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-07T19:22:49Z","receivedAt":"2011-02-07T19:22:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Feb 7, 2011 at 7:48 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Erik Faye-Lund <kusmabite@gmail.com> writes:\n>\n>> At the same time, this fixes a problem with strict-POSIX getenv\n>> implementations. POSIX says \"The return value from getenv() may\n>> point to static data which may be overwritten by subsequent calls\n>> to getenv()\", so duplicating the strings is a potential bug.\n>                  ^\n>                 not\n> ?\n\nIndeed, thanks.\n"},{"id":"160624","messageId":"1297110111-7620-1-git-send-email-kusmabite@gmail.com","threadId":"26437","inReplyTo":"AANLkTikKZ+2qUMF1T5pP60cUd9Ya3n2mfhTkX6L32zmn@mail.gmail.com","subject":"[PATCH v2] commit: fix memory-leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-07T20:21:51Z","receivedAt":"2011-02-07T20:21:51Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"The name, email and date strings are some times allocated on the\nheap, but not free'd. Fix this by making sure they are allways\nheap-allocated, so we can safely free the memory.\n\nAt the same time, this fixes a problem with strict-POSIX getenv\nimplementations. POSIX says \"The return value from getenv() may\npoint to static data which may be overwritten by subsequent calls\nto getenv()\", so not duplicating the strings is a potential bug.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\nFixed typo in commit message, as pointed out by Matthieu Moy.\n\n builtin/commit.c  |    9 ++++++---\n git-compat-util.h |    1 +\n wrapper.c         |    6 ++++++\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 03cff5a..e5a649e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \n-\tname = getenv(\"GIT_AUTHOR_NAME\");\n-\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n-\tdate = getenv(\"GIT_AUTHOR_DATE\");\n+\tname = xgetenv(\"GIT_AUTHOR_NAME\");\n+\temail = xgetenv(\"GIT_AUTHOR_EMAIL\");\n+\tdate = xgetenv(\"GIT_AUTHOR_DATE\");\n \n \tif (use_message && !renew_authorship) {\n \t\tconst char *a, *lb, *rb, *eol;\n@@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\tdate = force_date;\n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n \t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\n+\tfree(name);\n+\tfree(email);\n+\tfree(date);\n }\n \n static int ends_rfc2822_footer(struct strbuf *sb)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d6d269f..12f111f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -409,6 +409,7 @@ typedef void (*try_to_free_t)(size_t);\n extern try_to_free_t set_try_to_free_routine(try_to_free_t);\n \n extern char *xstrdup(const char *str);\n+extern char *xgetenv(const char *name);\n extern void *xmalloc(size_t size);\n extern void *xmallocz(size_t size);\n extern void *xmemdupz(const void *data, size_t len);\ndiff --git a/wrapper.c b/wrapper.c\nindex 8d7dd31..e6173c4 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -30,6 +30,12 @@ char *xstrdup(const char *str)\n \treturn ret;\n }\n \n+char *xgetenv(const char *name)\n+{\n+\tchar *tmp = getenv(name);\n+\treturn tmp ? xstrdup(tmp) : NULL;\n+}\n+\n void *xmalloc(size_t size)\n {\n \tvoid *ret = malloc(size);\n-- \n1.7.4.msysgit.0\n"},{"id":"160642","messageId":"AANLkTikr2+OVRU6n+0tA752_x80ir9dQh65RjUp3BxPR@mail.gmail.com","threadId":"26437","inReplyTo":"1297110111-7620-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2] commit: fix memory-leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-07T21:12:22Z","receivedAt":"2011-02-07T21:12:22Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> The name, email and date strings are some times allocated on the\n> heap, but not free'd. Fix this by making sure they are allways\n> heap-allocated, so we can safely free the memory.\n>\n> At the same time, this fixes a problem with strict-POSIX getenv\n> implementations. POSIX says \"The return value from getenv() may\n> point to static data which may be overwritten by subsequent calls\n> to getenv()\", so not duplicating the strings is a potential bug.\n>\n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> ---\n> Fixed typo in commit message, as pointed out by Matthieu Moy.\n>\n>  builtin/commit.c  |    9 ++++++---\n>  git-compat-util.h |    1 +\n>  wrapper.c         |    6 ++++++\n>  3 files changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 03cff5a..e5a649e 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident)\n>  {\n>        char *name, *email, *date;\n>\n> -       name = getenv(\"GIT_AUTHOR_NAME\");\n> -       email = getenv(\"GIT_AUTHOR_EMAIL\");\n> -       date = getenv(\"GIT_AUTHOR_DATE\");\n> +       name = xgetenv(\"GIT_AUTHOR_NAME\");\n> +       email = xgetenv(\"GIT_AUTHOR_EMAIL\");\n> +       date = xgetenv(\"GIT_AUTHOR_DATE\");\n>\n>        if (use_message && !renew_authorship) {\n>                const char *a, *lb, *rb, *eol;\n> @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident)\n>                date = force_date;\n>        strbuf_addstr(author_ident, fmt_ident(name, email, date,\n>                                              IDENT_ERROR_ON_NO_NAME));\n> +       free(name);\n> +       free(email);\n> +       free(date);\n\nHmm, but I'm getting a crash here on Linux. Guess I need to debug a bit...\n"},{"id":"160645","messageId":"AANLkTimvgDjiLon-2BAwxcYOQGVE9UZPNr637o93-9HQ@mail.gmail.com","threadId":"26437","inReplyTo":"AANLkTikr2+OVRU6n+0tA752_x80ir9dQh65RjUp3BxPR@mail.gmail.com","subject":"Re: [PATCH v2] commit: fix memory-leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-07T21:31:14Z","receivedAt":"2011-02-07T21:31:14Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Feb 7, 2011 at 10:12 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Mon, Feb 7, 2011 at 9:21 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> The name, email and date strings are some times allocated on the\n>> heap, but not free'd. Fix this by making sure they are allways\n>> heap-allocated, so we can safely free the memory.\n>>\n>> At the same time, this fixes a problem with strict-POSIX getenv\n>> implementations. POSIX says \"The return value from getenv() may\n>> point to static data which may be overwritten by subsequent calls\n>> to getenv()\", so not duplicating the strings is a potential bug.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>> ---\n>> Fixed typo in commit message, as pointed out by Matthieu Moy.\n>>\n>>  builtin/commit.c  |    9 ++++++---\n>>  git-compat-util.h |    1 +\n>>  wrapper.c         |    6 ++++++\n>>  3 files changed, 13 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 03cff5a..e5a649e 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -465,9 +465,9 @@ static void determine_author_info(struct strbuf *author_ident)\n>>  {\n>>        char *name, *email, *date;\n>>\n>> -       name = getenv(\"GIT_AUTHOR_NAME\");\n>> -       email = getenv(\"GIT_AUTHOR_EMAIL\");\n>> -       date = getenv(\"GIT_AUTHOR_DATE\");\n>> +       name = xgetenv(\"GIT_AUTHOR_NAME\");\n>> +       email = xgetenv(\"GIT_AUTHOR_EMAIL\");\n>> +       date = xgetenv(\"GIT_AUTHOR_DATE\");\n>>\n>>        if (use_message && !renew_authorship) {\n>>                const char *a, *lb, *rb, *eol;\n>> @@ -507,6 +507,9 @@ static void determine_author_info(struct strbuf *author_ident)\n>>                date = force_date;\n>>        strbuf_addstr(author_ident, fmt_ident(name, email, date,\n>>                                              IDENT_ERROR_ON_NO_NAME));\n>> +       free(name);\n>> +       free(email);\n>> +       free(date);\n>\n> Hmm, but I'm getting a crash here on Linux. Guess I need to debug a bit...\n>\n\nAh, it was the force_date-assignment:\n---8<---\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e5a649e..1416c13 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -504,7 +504,7 @@ static void determine_author_info(struct strbuf\n*author_ident)\n \t}\n\n \tif (force_date)\n-\t\tdate = force_date;\n+\t\tdate = xstrdup(force_date);\n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n \t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\n \tfree(name);\n\n---8<---\n\nBut now I see that I was temporarily(?) struck with insanity:\noverwriting a heap-allocated pointer with another heap-allocated\npointer doesn't fix a memory-leak. So let's add some more calls to\nfree:\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e5a649e..bdd0cfb 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -482,6 +482,10 @@ static void determine_author_info(struct strbuf\n*author_ident)\n \t\tif (!*lb || !*rb || !*eol)\n \t\t\tdie(\"invalid commit: %s\", use_message);\n\n+\t\tfree(name);\n+\t\tfree(email);\n+\t\tfree(date);\n+\n \t\tif (lb == a + strlen(\"\\nauthor \"))\n \t\t\t/* \\nauthor <foo@example.com> */\n \t\t\tname = xcalloc(1, 1);\n@@ -497,14 +501,19 @@ static void determine_author_info(struct strbuf\n*author_ident)\n \t\tconst char *lb = strstr(force_author, \" <\");\n \t\tconst char *rb = strchr(force_author, '>');\n\n+\t\tfree(name);\n+\t\tfree(email);\n+\n \t\tif (!lb || !rb)\n \t\t\tdie(\"malformed --author parameter\");\n \t\tname = xstrndup(force_author, lb - force_author);\n \t\temail = xstrndup(lb + 2, rb - (lb + 2));\n \t}\n\n-\tif (force_date)\n-\t\tdate = force_date;\n+\tif (force_date) {\n+\t\tfree(date);\n+\t\tdate = xstrdup(force_date);\n+\t}\n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n \t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\n \tfree(name);\n"}]}