Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fmt-merge-msg: plug small leak of commit buffer
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html