{"thread":{"id":"39089","subject":"[PATCH] fmt-merge-msg: plug small leak of commit buffer","startedAt":"2015-04-15T21:25:35Z","lastAt":"2015-04-20T21:35:28Z","messageCount":4,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259447","messageId":"xmqqegnljd80.fsf@gitster.dls.corp.google.com","threadId":"39089","inReplyTo":null,"subject":"[PATCH] fmt-merge-msg: plug small leak of commit buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T21:25:35Z","receivedAt":"2015-04-15T21:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A broken or badly formatted commit might not record author or\ncommitter lines; the function record_person() returned after\ncalling get_commit_buffer() without calling unuse_commit_buffer()\non the memory, potentially leaking it.\n\nSigned-off-by: Junio C Hamano \n---\n\n * to be applied on bc6b8fc1 (use get_commit_buffer everywhere,\n 2014-06-10)\n\n builtin/fmt-merge-msg.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 01f6d59..076264d 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -238,8 +238,10 @@ static void record_person(int which, struct string_list *people,\n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n \tbuffer = get_commit_buffer(commit);\n \tname = strstr(buffer, field);\n-\tif (!name)\n+\tif (!name) {\n+\t\tunuse_commit_buffer(commit, buffer);\n \t\treturn;\n+\t}\n \tname += strlen(field);\n \tname_end = strchrnul(name, '<');\n \tif (*name_end)\n-- \n2.4.0-rc2-173-gefc434b\n"},{"id":"259448","messageId":"xmqqa8y9jd06.fsf@gitster.dls.corp.google.com","threadId":"39089","inReplyTo":"xmqqegnljd80.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T21:30:17Z","receivedAt":"2015-04-15T21:30:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> A broken or badly formatted commit might not record author or\n> committer lines; the function record_person() returned after\n> calling get_commit_buffer() without calling unuse_commit_buffer()\n> on the memory, potentially leaking it.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * to be applied on bc6b8fc1 (use get_commit_buffer everywhere,\n> 2014-06-10)\n\nI spoke too soon. There are two error-exit paths in this function.\n\n-- >8 --\nA broken or badly formatted commit might not record author or\ncommitter lines or we may not find a valid name on them. The\nfunction record_person() returned after calling get_commit_buffer()\nwithout calling unuse_commit_buffer() on the memory it obtained in\nsuch cases, potentially leaking it.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/fmt-merge-msg.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 01f6d59..76277d1 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -239,7 +239,7 @@ static void record_person(int which, struct string_list *people,\n \tbuffer = get_commit_buffer(commit);\n \tname = strstr(buffer, field);\n \tif (!name)\n-\t\treturn;\n+\t\tgoto leave;\n \tname += strlen(field);\n \tname_end = strchrnul(name, '<');\n \tif (*name_end)\n@@ -247,9 +247,8 @@ static void record_person(int which, struct string_list *people,\n \twhile (isspace(*name_end) && name <= name_end)\n \t\tname_end--;\n \tif (name_end < name)\n-\t\treturn;\n+\t\tgoto leave;\n \tname_buf = xmemdupz(name, name_end - name + 1);\n-\tunuse_commit_buffer(commit, buffer);\n \n \telem = string_list_lookup(people, name_buf);\n \tif (!elem) {\n@@ -258,6 +257,8 @@ static void record_person(int which, struct string_list *people,\n \t}\n \telem->util = (void*)(util_as_integral(elem) + 1);\n \tfree(name_buf);\n+leave:\n+\tunuse_commit_buffer(commit, buffer);\n }\n \n static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\n-- \n2.4.0-rc2-173-gefc434b\n"},{"id":"259449","messageId":"20150415221431.GA27566@peff.net","threadId":"39089","inReplyTo":"xmqqa8y9jd06.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-15T22:14:32Z","receivedAt":"2015-04-15T22:14:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2015 at 02:30:17PM -0700, Junio C Hamano wrote:\n\n> I spoke too soon. There are two error-exit paths in this function.\n> \n> -- >8 --\n> A broken or badly formatted commit might not record author or\n> committer lines or we may not find a valid name on them. The\n> function record_person() returned after calling get_commit_buffer()\n> without calling unuse_commit_buffer() on the memory it obtained in\n> such cases, potentially leaking it.\n\nLooks good. Thanks for cleaning my mess. I looked over the offending\nbc6b8fc1, and the other changed spots all look OK.\n\nI note that record_person does not seem to care about the commit at all,\nso an alternative fix would be:\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 1d962dc..9f0e608 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -223,16 +223,14 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n \n #define util_as_integral(elem) ((intptr_t)((elem)->util))\n \n-static void record_person(int which, struct string_list *people,\n-\t\t\t struct commit *commit)\n+static void record_person_from_buf(int which, struct string_list *people,\n+\t\t\t\t const char *buffer)\n {\n-\tconst char *buffer;\n \tchar *name_buf, *name, *name_end;\n \tstruct string_list_item *elem;\n \tconst char *field;\n \n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n-\tbuffer = get_commit_buffer(commit, NULL);\n \tname = strstr(buffer, field);\n \tif (!name)\n \t\treturn;\n@@ -245,7 +243,6 @@ static void record_person(int which, struct string_list *people,\n \tif (name_end < name)\n \t\treturn;\n \tname_buf = xmemdupz(name, name_end - name + 1);\n-\tunuse_commit_buffer(commit, buffer);\n \n \telem = string_list_lookup(people, name_buf);\n \tif (!elem) {\n@@ -256,6 +253,14 @@ static void record_person(int which, struct string_list *people,\n \tfree(name_buf);\n }\n \n+static void record_person(int which, struct string_list *people,\n+\t\t\t struct commit *commit)\n+{\n+\tconst char *buf = get_commit_buffer(commit, NULL);\n+\trecord_person_from_buf(which, people, buf);\n+\tunuse_commit_buffer(commit, buf);\n+}\n+\n static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\n {\n \tconst struct string_list_item *a = a_, *b = b_;\n\n\nThis has the slight advantage that it adapts naturally if record_person\ngrows more exits, but I don't think it is a big deal either way (it only\nmatters if the new exit fails to copy the surrounding code and use \"goto\nleave\").\n\n-Peff\n"},{"id":"259709","messageId":"xmqqoamijxen.fsf@gitster.dls.corp.google.com","threadId":"39089","inReplyTo":"20150415221431.GA27566@peff.net","subject":"Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T21:35:28Z","receivedAt":"2015-04-20T21:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I note that record_person does not seem to care about the commit at all,\n> so an alternative fix would be:\n>\n> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n> index 1d962dc..9f0e608 100644\n> --- a/builtin/fmt-merge-msg.c\n> +++ b/builtin/fmt-merge-msg.c\n> @@ -223,16 +223,14 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n> \n> #define util_as_integral(elem) ((intptr_t)((elem)->util))\n> \n> -static void record_person(int which, struct string_list *people,\n> -\t\t\t struct commit *commit)\n> +static void record_person_from_buf(int which, struct string_list *people,\n> +\t\t\t\t const char *buffer)\n> {\n> -\tconst char *buffer;\n> \tchar *name_buf, *name, *name_end;\n> \tstruct string_list_item *elem;\n> \tconst char *field;\n> \n> \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n> -\tbuffer = get_commit_buffer(commit, NULL);\n> \tname = strstr(buffer, field);\n> \tif (!name)\n> \t\treturn;\n> @@ -245,7 +243,6 @@ static void record_person(int which, struct string_list *people,\n> \tif (name_end < name)\n> \t\treturn;\n> \tname_buf = xmemdupz(name, name_end - name + 1);\n> -\tunuse_commit_buffer(commit, buffer);\n> \n> \telem = string_list_lookup(people, name_buf);\n> \tif (!elem) {\n> @@ -256,6 +253,14 @@ static void record_person(int which, struct string_list *people,\n> \tfree(name_buf);\n> }\n> \n> +static void record_person(int which, struct string_list *people,\n> +\t\t\t struct commit *commit)\n> +{\n> +\tconst char *buf = get_commit_buffer(commit, NULL);\n> +\trecord_person_from_buf(which, people, buf);\n> +\tunuse_commit_buffer(commit, buf);\n> +}\n> +\n> static int cmp_string_list_util_as_integral(const void *a_, const void *b_)\n> {\n> \tconst struct string_list_item *a = a_, *b = b_;\n>\n>\n> This has the slight advantage that it adapts naturally if record_person\n> grows more exits, but I don't think it is a big deal either way (it only\n> matters if the new exit fails to copy the surrounding code and use \"goto\n> leave\").\n\nYeah, let me steal that from you.\n\nThanks.\n"}]}