# [PATCHv3 6/6] entry.c: fix a memleak 16 messages from 2015-03-31 to 2015-03-31. Participants: Stefan Beller, Eric Sunshine, Jeff King, Torsten Bögershausen, Junio C Hamano, John Keeping. Thread: https://gitlist.dev/t/38954 ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 0/6] Memory leaks once again Message-ID: <1427764931-27745-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-1-git-send-email-sbeller%40google.com ``` I changed the patches based on Erics feedback, all of them look sane to me now. I still retain 2 patches which I need to convince myself are reasonable to send to the list though. John Keeping (1): entry.c: fix a memleak Stefan Beller (5): shallow: fix a memleak line-log.c: fix a memleak line-log.c: fix a memleak wt-status.c: fix a memleak pack-bitmap.c: fix a memleak line-log.c | 4 ++++ pack-bitmap.c | 2 ++ shallow.c | 5 +++-- streaming.c | 5 ++++- wt-status.c | 1 + 5 files changed, 14 insertions(+), 3 deletions(-) -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 1/6] shallow: fix a memleak Message-ID: <1427764931-27745-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-2-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` Signed-off-by: Stefan Beller --- shallow.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shallow.c b/shallow.c index d8bf40a..11d5c4e 100644 --- a/shallow.c +++ b/shallow.c @@ -412,11 +412,12 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1, struct commit_list *head = NULL; int bitmap_nr = (info->nr_bits + 31) / 32; int bitmap_size = bitmap_nr * sizeof(uint32_t); - uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */ - uint32_t *bitmap = paint_alloc(info); + uint32_t *tmp, *bitmap; struct commit *c = lookup_commit_reference_gently(sha1, 1); if (!c) return; + tmp = xmalloc(bitmap_size); /* to be freed before return */ + bitmap = paint_alloc(info); memset(bitmap, 0, bitmap_size); bitmap[id / 32] |= (1 << (id % 32)); commit_list_insert(c, &head); -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 2/6] line-log.c: fix a memleak Message-ID: <1427764931-27745-3-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-3-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` The `filepair` is assigned new memory with any iteration via process_diff_filepair, so free it before the current iteration ends. Signed-off-by: Stefan Beller --- line-log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/line-log.c b/line-log.c index a490efe..b43ac58 100644 --- a/line-log.c +++ b/line-log.c @@ -1099,6 +1099,7 @@ static int process_all_files(struct line_log_data **range_out, rg->pair = diff_filepair_dup(queue->queue[i]); memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges)); } + free(pairdiff); } return changed; -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 3/6] line-log.c: fix a memleak Message-ID: <1427764931-27745-4-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-4-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` Signed-off-by: Stefan Beller --- line-log.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/line-log.c b/line-log.c index b43ac58..db6e58d 100644 --- a/line-log.c +++ b/line-log.c @@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c changed = process_all_files(&parent_range, rev, &queue, range); if (parent) add_line_range(rev, parent, parent_range); + else + free_line_log_data(parent_range); + return changed; } -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 4/6] wt-status.c: fix a memleak Message-ID: <1427764931-27745-5-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-5-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` In any code path of shorten_unambiguous_ref the return value is a xstrdup(some string), so it is safe to free the variable `base` in any codepath. Signed-off-by: Stefan Beller --- wt-status.c | 1 + 1 file changed, 1 insertion(+) diff --git a/wt-status.c b/wt-status.c index 853419f..38cb165 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1550,6 +1550,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) base = shorten_unambiguous_ref(base, 0); color_fprintf(s->fp, header_color, "..."); color_fprintf(s->fp, branch_color_remote, "%s", base); + free((char *)base); if (!upstream_is_gone && !num_ours && !num_theirs) { fputc(s->null_termination ? '\0' : '\n', s->fp); -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 5/6] pack-bitmap.c: fix a memleak Message-ID: <1427764931-27745-6-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-6-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` Signed-off-by: Stefan Beller --- pack-bitmap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pack-bitmap.c b/pack-bitmap.c index 365f9d9..62a98cc 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -986,6 +986,8 @@ void test_bitmap_walk(struct rev_info *revs) fprintf(stderr, "OK!\n"); else fprintf(stderr, "Mismatch!\n"); + + free(result); } static int rebuild_bitmap(uint32_t *reposition, -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-03-31 01:22 Subject: [PATCHv3 6/6] entry.c: fix a memleak Message-ID: <1427764931-27745-7-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1427764931-27745-7-git-send-email-sbeller%40google.com In-Reply-To: <1427764931-27745-1-git-send-email-sbeller@google.com> ``` From: John Keeping stream_blob_to_fd() always frees the filter now, so there is no memory leak in entry.c:152 just before the `goto finish`. Signed-off-by: John Keeping Signed-off-by: Stefan Beller --- I added Johns signoff here tentatively, John can you confirm the sign off? streaming.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/streaming.c b/streaming.c index 2ff036a..811fcc2 100644 --- a/streaming.c +++ b/streaming.c @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f int result = -1; st = open_istream(sha1, &type, &sz, filter); - if (!st) + if (!st) { + if (filter) + free_stream_filter(filter); return result; + } if (type != OBJ_BLOB) goto close_and_exit; for (;;) { -- 2.3.0.81.gc37f363 ``` ## Eric Sunshine, 2015-03-31 02:52 Subject: Re: [PATCHv3 1/6] shallow: fix a memleak Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRi9L%2BbEJsjFdihLQJjb38Bg500%3DsZ%2Bw4FubR5f3%3Dn-bQ%40mail.gmail.com In-Reply-To: <1427764931-27745-2-git-send-email-sbeller@google.com> ``` On Mon, Mar 30, 2015 at 9:22 PM, Stefan Beller wrote: > shallow: fix a memleak > Signed-off-by: Stefan Beller > --- > diff --git a/shallow.c b/shallow.c > index d8bf40a..11d5c4e 100644 > --- a/shallow.c > +++ b/shallow.c > @@ -412,11 +412,12 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1, > struct commit_list *head = NULL; > int bitmap_nr = (info->nr_bits + 31) / 32; > int bitmap_size = bitmap_nr * sizeof(uint32_t); > - uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */ > - uint32_t *bitmap = paint_alloc(info); > + uint32_t *tmp, *bitmap; > struct commit *c = lookup_commit_reference_gently(sha1, 1); > if (!c) > return; > + tmp = xmalloc(bitmap_size); /* to be freed before return */ > + bitmap = paint_alloc(info); You've made two unrelated changes in this patch, only one of which is a fix for the memory leak mentioned by the commit message. The 'tmp' change fixes the memory leak. The 'bitmap' change, however, merely avoids doing work unnecessarily in the "early return" case. Since 'bitmap' gets released by the caller of paint_down() when the paint_info structure is freed, it is misleading to claim that that particular change is a "memleak" fix. Either split the two changes into separate patches or augment the commit message to say something along the lines of: While here, also avoid unnecessarily allocating 'bitmap' within paint_info in the early-return case. > memset(bitmap, 0, bitmap_size); > bitmap[id / 32] |= (1 << (id % 32)); > commit_list_insert(c, &head); > -- > 2.3.0.81.gc37f363 ``` ## Jeff King, 2015-03-31 03:32 Subject: Re: [PATCHv3 5/6] pack-bitmap.c: fix a memleak Message-ID: <20150331033202.GB842@peff.net> URL: https://gitlist.dev/e/20150331033202.GB842%40peff.net In-Reply-To: <1427764931-27745-6-git-send-email-sbeller@google.com> ``` On Mon, Mar 30, 2015 at 06:22:10PM -0700, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > pack-bitmap.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 365f9d9..62a98cc 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -986,6 +986,8 @@ void test_bitmap_walk(struct rev_info *revs) > fprintf(stderr, "OK!\n"); > else > fprintf(stderr, "Mismatch!\n"); > + > + free(result); I think this one is fine. I see you dropped the other bits from load_bitmap_entries_v1. There are definitely some leaks there in the error case, and I left some notes in the other thread. I don't think those leaks are all that critical (we only load the bitmaps one per process, and the memory persists if we succeed, so the worst case is that we spend the memory but do not get to use the bitmaps). But if your goal is making us valgrind-clean, they would certainly count. -Peff ``` ## Torsten Bögershausen, 2015-03-31 04:42 Subject: Re: [PATCHv3 3/6] line-log.c: fix a memleak Message-ID: <551A25B1.80309@web.de> URL: https://gitlist.dev/e/551A25B1.80309%40web.de In-Reply-To: <1427764931-27745-4-git-send-email-sbeller@google.com> ``` On 03/31/2015 03:22 AM, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > line-log.c | 3 +++ > 1 file changed, 3 insertions(+) > Would it make sense to join 2/6 and 3/6 into a single patch ? Both fix the memleaks in the same file, and the header can be "fix memleaks". ``` ## Junio C Hamano, 2015-03-31 05:06 Subject: Re: [PATCHv3 3/6] line-log.c: fix a memleak Message-ID: URL: https://gitlist.dev/e/xmqqsiclsqlz.fsf%40gitster.dls.corp.google.com In-Reply-To: <1427764931-27745-4-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > Signed-off-by: Stefan Beller > --- > line-log.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/line-log.c b/line-log.c > index b43ac58..db6e58d 100644 > --- a/line-log.c > +++ b/line-log.c > @@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c > changed = process_all_files(&parent_range, rev, &queue, range); > if (parent) > add_line_range(rev, parent, parent_range); > + else > + free_line_log_data(parent_range); > + Should this be in the else clause? From a cursory scan add-line-range feeds it to either line-log-data-merge or line-log-data-copy to create "new" that is a separate thing from the parent_range we are feeding it, and that copy is used by add-decoration. If add-line-range does not take ownership of parent_range from us, shouldn't we be freeing it whether we called it or not??? > return changed; > } ``` ## Stefan Beller, 2015-03-31 05:35 Subject: Re: [PATCHv3 3/6] line-log.c: fix a memleak Message-ID: <551A323F.9030305@gmail.com> URL: https://gitlist.dev/e/551A323F.9030305%40gmail.com In-Reply-To: ``` On 30.03.2015 22:06, Junio C Hamano wrote: > If add-line-range does not take ownership of > parent_range from us, shouldn't we be freeing it whether we called > it or not??? In my understanding it does take ownership of it. (According to my code review on friday) ``` ## Junio C Hamano, 2015-03-31 05:41 Subject: Re: [PATCHv3 6/6] entry.c: fix a memleak Message-ID: URL: https://gitlist.dev/e/xmqqoan9soym.fsf%40gitster.dls.corp.google.com In-Reply-To: <1427764931-27745-7-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > From: John Keeping > > stream_blob_to_fd() always frees the filter now, so there is no memory > leak in entry.c:152 just before the `goto finish`. > > Signed-off-by: John Keeping > Signed-off-by: Stefan Beller > --- > > I added Johns signoff here tentatively, John can you confirm the sign off? > > streaming.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/streaming.c b/streaming.c > index 2ff036a..811fcc2 100644 > --- a/streaming.c > +++ b/streaming.c > @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f > int result = -1; > > st = open_istream(sha1, &type, &sz, filter); > - if (!st) > + if (!st) { > + if (filter) > + free_stream_filter(filter); > return result; > + } I think this one is fine. I see you moved the callsite to free this thing from entry.c down to streaming (stream_blob_to_fd()), which means you would need to adjust the title of the change, too. It somewhat feels dirty that we free what the caller gave us, but given that closing a filtered stream frees the filter at the end, I think this is the right place to discard the filter. In essence, the overall use of the API in this code is equivalent to filter = ... prepare a filter ... if (open_istream(... filter) == OK) { ... use filter ... close_istream(); /* which frees the filter */ } else { /* we failed to free filter here without the patch */ free_stream_filter(filter); } so the patch makes sense. We may want to teach free_stream_filter() that a request to free a NULL filter is OK, though. That would be a separate clean-up topic. > if (type != OBJ_BLOB) > goto close_and_exit; > for (;;) { ``` ## John Keeping, 2015-03-31 08:36 Subject: Re: [PATCHv3 6/6] entry.c: fix a memleak Message-ID: <20150331083647.GA2248@serenity.lan> URL: https://gitlist.dev/e/20150331083647.GA2248%40serenity.lan In-Reply-To: <1427764931-27745-7-git-send-email-sbeller@google.com> ``` On Mon, Mar 30, 2015 at 06:22:11PM -0700, Stefan Beller wrote: > From: John Keeping > > stream_blob_to_fd() always frees the filter now, so there is no memory > leak in entry.c:152 just before the `goto finish`. > > Signed-off-by: John Keeping > Signed-off-by: Stefan Beller > --- > > I added Johns signoff here tentatively, John can you confirm the sign off? Signed-off-by: John Keeping > streaming.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/streaming.c b/streaming.c > index 2ff036a..811fcc2 100644 > --- a/streaming.c > +++ b/streaming.c > @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f > int result = -1; > > st = open_istream(sha1, &type, &sz, filter); > - if (!st) > + if (!st) { > + if (filter) > + free_stream_filter(filter); > return result; > + } > if (type != OBJ_BLOB) > goto close_and_exit; > for (;;) { > -- > 2.3.0.81.gc37f363 > ``` ## Junio C Hamano, 2015-03-31 19:03 Subject: Re: [PATCHv3 3/6] line-log.c: fix a memleak Message-ID: URL: https://gitlist.dev/e/xmqqr3s5q9an.fsf%40gitster.dls.corp.google.com In-Reply-To: <551A323F.9030305@gmail.com> ``` Stefan Beller writes: > On 30.03.2015 22:06, Junio C Hamano wrote: >> If add-line-range does not take ownership of >> parent_range from us, shouldn't we be freeing it whether we called >> it or not??? > > In my understanding it does take ownership of it. (According to my code > review on friday) Hmmm. add_line_range() is given the "parent_range" as its third parameter, i.e. "range". If there is nothing for the commit yet, it calls line_log_data_copy() on it to make a deep copy and stores that in new, which is then used as the decoration for the commit---the copy is what is retained. I am not quite sure what the lifetime rule is for the original of this deep copy, i.e. "range", aka "parent_range" of the caller. Who holds pointers into it after this function is done? ``` ## Junio C Hamano, 2015-03-31 19:12 Subject: Re: [PATCHv3 6/6] entry.c: fix a memleak Message-ID: URL: https://gitlist.dev/e/xmqqmw2tq8uk.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150331083647.GA2248@serenity.lan> ``` John Keeping writes: > On Mon, Mar 30, 2015 at 06:22:11PM -0700, Stefan Beller wrote: >> From: John Keeping >> >> stream_blob_to_fd() always frees the filter now, so there is no memory >> leak in entry.c:152 just before the `goto finish`. >> >> Signed-off-by: John Keeping >> Signed-off-by: Stefan Beller >> --- >> >> I added Johns signoff here tentatively, John can you confirm the sign off? > > Signed-off-by: John Keeping Thanks. I'll queue with a retitle, as this no longer is about entry.c at all. > >> streaming.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/streaming.c b/streaming.c >> index 2ff036a..811fcc2 100644 >> --- a/streaming.c >> +++ b/streaming.c >> @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f >> int result = -1; >> >> st = open_istream(sha1, &type, &sz, filter); >> - if (!st) >> + if (!st) { >> + if (filter) >> + free_stream_filter(filter); >> return result; >> + } >> if (type != OBJ_BLOB) >> goto close_and_exit; >> for (;;) { >> -- >> 2.3.0.81.gc37f363 >> ```