{"thread":{"id":"38930","subject":"[PATCH V2 0/6] Memory leaks once again","startedAt":"2015-03-27T22:32:43Z","lastAt":"2015-03-31T03:29:53Z","messageCount":14,"participants":["Stefan Beller","Eric Sunshine","John Keeping","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"258612","messageId":"1427495569-10863-1-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":null,"subject":"[PATCH V2 0/6] Memory leaks once again","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:43Z","receivedAt":"2015-03-27T22:32:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Here comes another bunch of memory leaks fixed.\npatches 1-4 are safe bets, but 5 and 6 are not so.\n\nIn patch 5 I wonder if we need to fix more aggressively and\nin patch 6 I just know there is a leak but I have no idea how to\nactually fix it.\n\nThe patches are apply-able to origin/master.\n\nVersion 2 has no changes in code but in recipients. CC'ing parties who\nmay be qualified to review the patches.\n\nStefan Beller (6):\n shallow: fix a memleak\n line-log.c: fix a memleak\n line-log.c: fix a memleak\n wt-status.c: fix a memleak\n pack-bitmap: fix a memleak\n WIP/RFC/entry.c: fix a memleak\n\n entry.c | 4 +++-\n line-log.c | 4 ++++\n pack-bitmap.c | 27 ++++++++++++++++++---------\n shallow.c | 4 ++--\n wt-status.c | 2 ++\n 5 files changed, 29 insertions(+), 12 deletions(-)\n\n-- \n2.3.0.81.gc37f363\n"},{"id":"258614","messageId":"1427495569-10863-2-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 1/6] shallow: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:44Z","receivedAt":"2015-03-27T22:32:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n shallow.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex d8bf40a..f8b0458 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n \tuint32_t *bitmap = paint_alloc(info);\n \tstruct commit *c = lookup_commit_reference_gently(sha1, 1);\n \tif (!c)\n-\t\treturn;\n+\t\tgoto out;\n \tmemset(bitmap, 0, bitmap_size);\n \tbitmap[id / 32] |= (1 << (id % 32));\n \tcommit_list_insert(c, &head);\n@@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n \t\tif (o && o->type == OBJ_COMMIT)\n \t\t\to->flags &= ~SEEN;\n \t}\n-\n+out:\n \tfree(tmp);\n }\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258613","messageId":"1427495569-10863-3-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 2/6] line-log.c: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:45Z","receivedAt":"2015-03-27T22:32:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The `filepair` is assigned new memory with any iteration via\nprocess_diff_filepair, so free it before the current iteration ends.\n\nSigned-off-by: Stefan Beller \n---\n line-log.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/line-log.c b/line-log.c\nindex a490efe..b43ac58 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1099,6 +1099,7 @@ static int process_all_files(struct line_log_data **range_out,\n \t\t\trg->pair = diff_filepair_dup(queue->queue[i]);\n \t\t\tmemcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));\n \t\t}\n+\t\tfree(pairdiff);\n \t}\n \n \treturn changed;\n-- \n2.3.0.81.gc37f363\n"},{"id":"258615","messageId":"1427495569-10863-4-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 3/6] line-log.c: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:46Z","receivedAt":"2015-03-27T22:32:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n line-log.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/line-log.c b/line-log.c\nindex b43ac58..db6e58d 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c\n \tchanged = process_all_files(&parent_range, rev, &queue, range);\n \tif (parent)\n \t\tadd_line_range(rev, parent, parent_range);\n+\telse\n+\t\tfree_line_log_data(parent_range);\n+\n \treturn changed;\n }\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258617","messageId":"1427495569-10863-5-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 4/6] wt-status.c: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:47Z","receivedAt":"2015-03-27T22:32:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In any code path of shorten_unambiguous_ref the return value is a\nxstrdup(some string), so it is safe to free the variable `base`\nin any codepath.\n\nSigned-off-by: Stefan Beller \n---\n wt-status.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 853419f..9d2a349 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n \n \tcolor_fprintf(s->fp, header_color, \"]\");\n \tfputc(s->null_termination ? '\\0' : '\\n', s->fp);\n+\n+\tfree((char *)base);\n }\n \n void wt_shortstatus_print(struct wt_status *s)\n-- \n2.3.0.81.gc37f363\n"},{"id":"258618","messageId":"1427495569-10863-6-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 5/6] pack-bitmap: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:48Z","receivedAt":"2015-03-27T22:32:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`recent_bitmaps` is allocated in the function load_bitmap_entries_v1\nand it is not passed into any function, so it's safe to free it before\nleaving that function.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n I wonder however if we need to free the actual bitmaps\n stored in the recent_bitmaps as well.\n\n pack-bitmap.c | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 365f9d9..34823e9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n {\n \tstatic const size_t MAX_XOR_OFFSET = 160;\n \n-\tuint32_t i;\n+\tuint32_t i, ret = 0;\n \tstruct stored_bitmap **recent_bitmaps;\n \n \trecent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));\n@@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tsha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);\n \n \t\tbitmap = read_bitmap_1(index);\n-\t\tif (!bitmap)\n-\t\t\treturn -1;\n+\t\tif (!bitmap) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n \n-\t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n-\t\t\treturn error(\"Corrupted bitmap pack index\");\n+\t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {\n+\t\t\tret = error(\"Corrupted bitmap pack index\");\n+\t\t\tgoto out;\n+\t\t}\n \n \t\tif (xor_offset > 0) {\n \t\t\txor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];\n \n-\t\t\tif (xor_bitmap == NULL)\n-\t\t\t\treturn error(\"Invalid XOR offset in bitmap pack index\");\n+\t\t\tif (xor_bitmap == NULL) {\n+\t\t\t\tret = error(\"Invalid XOR offset in bitmap pack index\");\n+\t\t\t\tgoto out;\n+\t\t\t}\n \t\t}\n \n \t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n \t\t\tindex, bitmap, sha1, xor_bitmap, flags);\n \t}\n-\n-\treturn 0;\n+out:\n+\tfree(recent_bitmaps);\n+\treturn ret;\n }\n \n static char *pack_bitmap_filename(struct packed_git *p)\n@@ -986,6 +993,8 @@ void test_bitmap_walk(struct rev_info *revs)\n \t\tfprintf(stderr, \"OK!\\n\");\n \telse\n \t\tfprintf(stderr, \"Mismatch!\\n\");\n+\n+\tfree(result);\n }\n \n static int rebuild_bitmap(uint32_t *reposition,\n-- \n2.3.0.81.gc37f363\n"},{"id":"258616","messageId":"1427495569-10863-7-git-send-email-sbeller@google.com","threadId":"38930","inReplyTo":"1427495569-10863-1-git-send-email-sbeller@google.com","subject":"[PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-27T22:32:49Z","receivedAt":"2015-03-27T22:32:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I observe that filter is going out of scope, but the\nimplementation proposed in this patch produces just a\ncrash instead of any helpful fix.\n\nSigned-off-by: Stefan Beller \n---\n entry.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 1eda8e9..5383001 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,\n \t\tif (filter &&\n \t\t !streaming_write_entry(ce, path, filter,\n \t\t\t\t\t state, to_tempfile,\n-\t\t\t\t\t &fstat_done, &st))\n+\t\t\t\t\t &fstat_done, &st)) {\n+\t\t\tfree_stream_filter(filter);\n \t\t\tgoto finish;\n+\t\t}\n \t}\n \n \tswitch (ce_mode_s_ifmt) {\n-- \n2.3.0.81.gc37f363\n"},{"id":"258619","messageId":"CAPig+cTpVwKG+6uK0HCyPUt0qKc7R6qWnLgVr8C9kodW4j2krg@mail.gmail.com","threadId":"38930","inReplyTo":"1427495569-10863-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH V2 1/6] shallow: fix a memleak","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-27T23:01:15Z","receivedAt":"2015-03-27T23:01:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/shallow.c b/shallow.c\n> index d8bf40a..f8b0458 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n> uint32_t *bitmap = paint_alloc(info);\n> struct commit *c = lookup_commit_reference_gently(sha1, 1);\n> if (!c)\n> - return;\n> + goto out;\n\nRather than slapping on a band-aid, a cleaner fix would be to move the\nallocation of 'tmp' below this conditional since 'tmp' is never used\nbefore the point of this early return.\n\nPerhaps also move allocation of 'bitmap' below the early return (if\nyou find that it's safe to do so).\n\n> memset(bitmap, 0, bitmap_size);\n> bitmap[id / 32] |= (1 << (id % 32));\n> commit_list_insert(c, &head);\n> @@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n> if (o && o->type == OBJ_COMMIT)\n> o->flags &= ~SEEN;\n> }\n> -\n> +out:\n> free(tmp);\n> }\n"},{"id":"258620","messageId":"CAPig+cT8_0+tgcDa4yOPgOLUA8Xtwa26JhMWo84c-J3_bjbTfw@mail.gmail.com","threadId":"38930","inReplyTo":"1427495569-10863-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH V2 4/6] wt-status.c: fix a memleak","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-27T23:16:08Z","receivedAt":"2015-03-27T23:16:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> In any code path of shorten_unambiguous_ref the return value is a\n> xstrdup(some string), so it is safe to free the variable `base`\n> in any codepath.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> wt-status.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index 853419f..9d2a349 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)\n>\n> color_fprintf(s->fp, header_color, \"]\");\n> fputc(s->null_termination ? '\\0' : '\\n', s->fp);\n> +\n> + free((char *)base);\n\nThere's an early 'return' before this point (at line 1560) which will\nleak 'base', so this is an unreliable fix. A better solution would be\nto free(base) immediately after it's final use (before the early\n'return').\n\n> }\n>\n> void wt_shortstatus_print(struct wt_status *s)\n> --\n> 2.3.0.81.gc37f363\n"},{"id":"258621","messageId":"CAPig+cREgQAx-CqBYT_ss0U-RN86ONWeZuv3VCb56eZA-VsuXQ@mail.gmail.com","threadId":"38930","inReplyTo":"1427495569-10863-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH V2 5/6] pack-bitmap: fix a memleak","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-27T23:27:18Z","receivedAt":"2015-03-27T23:27:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> `recent_bitmaps` is allocated in the function load_bitmap_entries_v1\n> and it is not passed into any function, so it's safe to free it before\n> leaving that function.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 365f9d9..34823e9 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> {\n> static const size_t MAX_XOR_OFFSET = 160;\n>\n> - uint32_t i;\n> + uint32_t i, ret = 0;\n> struct stored_bitmap **recent_bitmaps;\n>\n> recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));\n> @@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> sha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);\n>\n> bitmap = read_bitmap_1(index);\n> - if (!bitmap)\n> - return -1;\n> + if (!bitmap) {\n> + ret = -1;\n> + goto out;\n> + }\n>\n> - if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n> - return error(\"Corrupted bitmap pack index\");\n> + if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {\n> + ret = error(\"Corrupted bitmap pack index\");\n> + goto out;\n> + }\n>\n> if (xor_offset > 0) {\n> xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];\n>\n> - if (xor_bitmap == NULL)\n> - return error(\"Invalid XOR offset in bitmap pack index\");\n> + if (xor_bitmap == NULL) {\n> + ret = error(\"Invalid XOR offset in bitmap pack index\");\n> + goto out;\n> + }\n> }\n>\n> recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n> index, bitmap, sha1, xor_bitmap, flags);\n> }\n> -\n> - return 0;\n> +out:\n> + free(recent_bitmaps);\n> + return ret;\n> }\n>\n> static char *pack_bitmap_filename(struct packed_git *p)\n> @@ -986,6 +993,8 @@ void test_bitmap_walk(struct rev_info *revs)\n> fprintf(stderr, \"OK!\\n\");\n> else\n> fprintf(stderr, \"Mismatch!\\n\");\n> +\n> + free(result);\n\nThis fix is not mentioned in the commit message.\n\n> }\n>\n> static int rebuild_bitmap(uint32_t *reposition,\n> --\n> 2.3.0.81.gc37f363\n"},{"id":"258622","messageId":"CAPig+cSHntg6KwZr3h6nhO7AK0v+FcbVAA4b_kvHdEg_=9o_eg@mail.gmail.com","threadId":"38930","inReplyTo":"1427495569-10863-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-27T23:32:11Z","receivedAt":"2015-03-27T23:32:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> I observe that filter is going out of scope, but the\n> implementation proposed in this patch produces just a\n> crash instead of any helpful fix.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/entry.c b/entry.c\n> index 1eda8e9..5383001 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,\n> if (filter &&\n> !streaming_write_entry(ce, path, filter,\n> state, to_tempfile,\n> - &fstat_done, &st))\n> + &fstat_done, &st)) {\n> + free_stream_filter(filter);\n\nAside from the crash you are seeing, this is a bogus fix anyway.\nYou're only freeing 'filter' if it was allocated _and_ if\nstreaming_write_entry() returned 0. I would guess your intention was\nto free 'filter' regardless of the result of streaming_write_entry().\n\n> goto finish;\n> + }\n> }\n>\n> switch (ce_mode_s_ifmt) {\n> --\n> 2.3.0.81.gc37f363\n"},{"id":"258627","messageId":"CAPig+cR8zA_FLhhnUgKZPdUFpCM3yNKNfbEBH+yoY=G44xGk8w@mail.gmail.com","threadId":"38930","inReplyTo":"CAPig+cSHntg6KwZr3h6nhO7AK0v+FcbVAA4b_kvHdEg_=9o_eg@mail.gmail.com","subject":"Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-28T00:14:28Z","receivedAt":"2015-03-28T00:14:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Friday, March 27, 2015, Eric Sunshine wrote:\n> On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> > I observe that filter is going out of scope, but the\n> > implementation proposed in this patch produces just a\n> > crash instead of any helpful fix.\n> >\n> > Signed-off-by: Stefan Beller \n> > ---\n> > diff --git a/entry.c b/entry.c\n> > index 1eda8e9..5383001 100644\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,\n> > if (filter &&\n> > !streaming_write_entry(ce, path, filter,\n> > state, to_tempfile,\n> > - &fstat_done, &st))\n> > + &fstat_done, &st)) {\n> > + free_stream_filter(filter);\n>\n> Aside from the crash you are seeing, this is a bogus fix anyway.\n> You're only freeing 'filter' if it was allocated _and_ if\n> streaming_write_entry() returned 0. I would guess your intention was\n> to free 'filter' regardless of the result of streaming_write_entry().\n\nUnless streaming_write_entry() is freeing the filter for you -- there\nis a free_stream_filter() call in close_method_decl() in streaming.c\n-- in which case your new free_stream_filter() call would attempt to\nfree the already-freed filter.\n"},{"id":"258630","messageId":"20150328110957.GB1097@serenity.lan","threadId":"38930","inReplyTo":"CAPig+cR8zA_FLhhnUgKZPdUFpCM3yNKNfbEBH+yoY=G44xGk8w@mail.gmail.com","subject":"Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-03-28T11:09:57Z","receivedAt":"2015-03-28T11:09:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Mar 27, 2015 at 08:14:28PM -0400, Eric Sunshine wrote:\n> On Friday, March 27, 2015, Eric Sunshine wrote:\n> > On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller wrote:\n> > > I observe that filter is going out of scope, but the\n> > > implementation proposed in this patch produces just a\n> > > crash instead of any helpful fix.\n> > >\n> > > Signed-off-by: Stefan Beller \n> > > ---\n> > > diff --git a/entry.c b/entry.c\n> > > index 1eda8e9..5383001 100644\n> > > --- a/entry.c\n> > > +++ b/entry.c\n> > > @@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,\n> > > if (filter &&\n> > > !streaming_write_entry(ce, path, filter,\n> > > state, to_tempfile,\n> > > - &fstat_done, &st))\n> > > + &fstat_done, &st)) {\n> > > + free_stream_filter(filter);\n> >\n> > Aside from the crash you are seeing, this is a bogus fix anyway.\n> > You're only freeing 'filter' if it was allocated _and_ if\n> > streaming_write_entry() returned 0. I would guess your intention was\n> > to free 'filter' regardless of the result of streaming_write_entry().\n> \n> Unless streaming_write_entry() is freeing the filter for you -- there\n> is a free_stream_filter() call in close_method_decl() in streaming.c\n> -- in which case your new free_stream_filter() call would attempt to\n> free the already-freed filter.\n\nYes, I think the correct fix for this leak is to make\nstream_blob_to_fd() always free the filter, since there's only one path\nout that doesn't at the moment and there's no way for the caller to\nfigure out whether or not the filter has been freed:\n\n-- >8 --\ndiff --git a/streaming.c b/streaming.c\nindex 2ff036a..811fcc2 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f\n \tint result = -1;\n \n \tst = open_istream(sha1, &type, &sz, filter);\n-\tif (!st)\n+\tif (!st) {\n+\t\tif (filter)\n+\t\t\tfree_stream_filter(filter);\n \t\treturn result;\n+\t}\n \tif (type != OBJ_BLOB)\n \t\tgoto close_and_exit;\n \tfor (;;) {\n"},{"id":"258726","messageId":"20150331032953.GA842@peff.net","threadId":"38930","inReplyTo":"1427495569-10863-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH V2 5/6] pack-bitmap: fix a memleak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-31T03:29:53Z","receivedAt":"2015-03-31T03:29:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 27, 2015 at 03:32:48PM -0700, Stefan Beller wrote:\n\n> `recent_bitmaps` is allocated in the function load_bitmap_entries_v1\n> and it is not passed into any function, so it's safe to free it before\n> leaving that function.\n\nI think this is OK, though it might be easier still to just turn the\narray into a stack variable. It's only 160 * sizeof(ptr), or about 1280\nbytes on a 64-bit system. Note that the xcalloc there looks wrong (it is\nway over-allocating by using the sizeof the struct, not a pointer to the\nstruct).\n\n> Notes:\n> I wonder however if we need to free the actual bitmaps\n> stored in the recent_bitmaps as well.\n\nNo, those are just weak pointers. The memory is owned by the hash that\nstore_bitmap puts the bitmaps into.\n\n> \t\tbitmap = read_bitmap_1(index);\n\nThis line allocates, too. So if we get down to...\n\n> -\t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n> -\t\t\treturn error(\"Corrupted bitmap pack index\");\n> +\t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {\n> +\t\t\tret = error(\"Corrupted bitmap pack index\");\n> +\t\t\tgoto out;\n> +\t\t}\n\n...here, for example, where we have not yet called store_bitmap, then\nit's a leak, too.\n\n-Peff\n"}]}