# [PATCH] fmt-merge-msg: plug small leak of commit buffer 4 messages from 2015-04-15 to 2015-04-20. Participants: Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/39089 ## Junio C Hamano, 2015-04-15 21:25 Subject: [PATCH] fmt-merge-msg: plug small leak of commit buffer Message-ID: URL: https://gitlist.dev/e/xmqqegnljd80.fsf%40gitster.dls.corp.google.com ``` A broken or badly formatted commit might not record author or committer lines; the function record_person() returned after calling get_commit_buffer() without calling unuse_commit_buffer() on the memory, potentially leaking it. Signed-off-by: Junio C Hamano --- * to be applied on bc6b8fc1 (use get_commit_buffer everywhere, 2014-06-10) builtin/fmt-merge-msg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 01f6d59..076264d 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -238,8 +238,10 @@ static void record_person(int which, struct string_list *people, field = (which == 'a') ? "\nauthor " : "\ncommitter "; buffer = get_commit_buffer(commit); name = strstr(buffer, field); - if (!name) + if (!name) { + unuse_commit_buffer(commit, buffer); return; + } name += strlen(field); name_end = strchrnul(name, '<'); if (*name_end) -- 2.4.0-rc2-173-gefc434b ``` ## Junio C Hamano, 2015-04-15 21:30 Subject: Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer Message-ID: URL: https://gitlist.dev/e/xmqqa8y9jd06.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > A broken or badly formatted commit might not record author or > committer lines; the function record_person() returned after > calling get_commit_buffer() without calling unuse_commit_buffer() > on the memory, potentially leaking it. > > Signed-off-by: Junio C Hamano > --- > > * to be applied on bc6b8fc1 (use get_commit_buffer everywhere, > 2014-06-10) I spoke too soon. There are two error-exit paths in this function. -- >8 -- A broken or badly formatted commit might not record author or committer lines or we may not find a valid name on them. The function record_person() returned after calling get_commit_buffer() without calling unuse_commit_buffer() on the memory it obtained in such cases, potentially leaking it. Signed-off-by: Junio C Hamano --- builtin/fmt-merge-msg.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 01f6d59..76277d1 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -239,7 +239,7 @@ static void record_person(int which, struct string_list *people, buffer = get_commit_buffer(commit); name = strstr(buffer, field); if (!name) - return; + goto leave; name += strlen(field); name_end = strchrnul(name, '<'); if (*name_end) @@ -247,9 +247,8 @@ static void record_person(int which, struct string_list *people, while (isspace(*name_end) && name <= name_end) name_end--; if (name_end < name) - return; + goto leave; name_buf = xmemdupz(name, name_end - name + 1); - unuse_commit_buffer(commit, buffer); elem = string_list_lookup(people, name_buf); if (!elem) { @@ -258,6 +257,8 @@ static void record_person(int which, struct string_list *people, } elem->util = (void*)(util_as_integral(elem) + 1); free(name_buf); +leave: + unuse_commit_buffer(commit, buffer); } static int cmp_string_list_util_as_integral(const void *a_, const void *b_) -- 2.4.0-rc2-173-gefc434b ``` ## Jeff King, 2015-04-15 22:14 Subject: Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer Message-ID: <20150415221431.GA27566@peff.net> URL: https://gitlist.dev/e/20150415221431.GA27566%40peff.net In-Reply-To: ``` On Wed, Apr 15, 2015 at 02:30:17PM -0700, Junio C Hamano wrote: > I spoke too soon. There are two error-exit paths in this function. > > -- >8 -- > A broken or badly formatted commit might not record author or > committer lines or we may not find a valid name on them. The > function record_person() returned after calling get_commit_buffer() > without calling unuse_commit_buffer() on the memory it obtained in > such cases, potentially leaking it. Looks good. Thanks for cleaning my mess. I looked over the offending bc6b8fc1, and the other changed spots all look OK. I note that record_person does not seem to care about the commit at all, so an alternative fix would be: diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 1d962dc..9f0e608 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -223,16 +223,14 @@ static void add_branch_desc(struct strbuf *out, const char *name) #define util_as_integral(elem) ((intptr_t)((elem)->util)) -static void record_person(int which, struct string_list *people, - struct commit *commit) +static void record_person_from_buf(int which, struct string_list *people, + const char *buffer) { - const char *buffer; char *name_buf, *name, *name_end; struct string_list_item *elem; const char *field; field = (which == 'a') ? "\nauthor " : "\ncommitter "; - buffer = get_commit_buffer(commit, NULL); name = strstr(buffer, field); if (!name) return; @@ -245,7 +243,6 @@ static void record_person(int which, struct string_list *people, if (name_end < name) return; name_buf = xmemdupz(name, name_end - name + 1); - unuse_commit_buffer(commit, buffer); elem = string_list_lookup(people, name_buf); if (!elem) { @@ -256,6 +253,14 @@ static void record_person(int which, struct string_list *people, free(name_buf); } +static void record_person(int which, struct string_list *people, + struct commit *commit) +{ + const char *buf = get_commit_buffer(commit, NULL); + record_person_from_buf(which, people, buf); + unuse_commit_buffer(commit, buf); +} + static int cmp_string_list_util_as_integral(const void *a_, const void *b_) { const struct string_list_item *a = a_, *b = b_; This has the slight advantage that it adapts naturally if record_person grows more exits, but I don't think it is a big deal either way (it only matters if the new exit fails to copy the surrounding code and use "goto leave"). -Peff ``` ## Junio C Hamano, 2015-04-20 21:35 Subject: Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer Message-ID: URL: https://gitlist.dev/e/xmqqoamijxen.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150415221431.GA27566@peff.net> ``` Jeff King writes: > I note that record_person does not seem to care about the commit at all, > so an alternative fix would be: > > diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c > index 1d962dc..9f0e608 100644 > --- a/builtin/fmt-merge-msg.c > +++ b/builtin/fmt-merge-msg.c > @@ -223,16 +223,14 @@ static void add_branch_desc(struct strbuf *out, const char *name) > > #define util_as_integral(elem) ((intptr_t)((elem)->util)) > > -static void record_person(int which, struct string_list *people, > - struct commit *commit) > +static void record_person_from_buf(int which, struct string_list *people, > + const char *buffer) > { > - const char *buffer; > char *name_buf, *name, *name_end; > struct string_list_item *elem; > const char *field; > > field = (which == 'a') ? "\nauthor " : "\ncommitter "; > - buffer = get_commit_buffer(commit, NULL); > name = strstr(buffer, field); > if (!name) > return; > @@ -245,7 +243,6 @@ static void record_person(int which, struct string_list *people, > if (name_end < name) > return; > name_buf = xmemdupz(name, name_end - name + 1); > - unuse_commit_buffer(commit, buffer); > > elem = string_list_lookup(people, name_buf); > if (!elem) { > @@ -256,6 +253,14 @@ static void record_person(int which, struct string_list *people, > free(name_buf); > } > > +static void record_person(int which, struct string_list *people, > + struct commit *commit) > +{ > + const char *buf = get_commit_buffer(commit, NULL); > + record_person_from_buf(which, people, buf); > + unuse_commit_buffer(commit, buf); > +} > + > static int cmp_string_list_util_as_integral(const void *a_, const void *b_) > { > const struct string_list_item *a = a_, *b = b_; > > > This has the slight advantage that it adapts naturally if record_person > grows more exits, but I don't think it is a big deal either way (it only > matters if the new exit fails to copy the surrounding code and use "goto > leave"). Yeah, let me steal that from you. Thanks. ```