# [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry 5 messages from 2014-02-20 to 2014-02-22. Participants: Nguyễn Thái Ngọc Duy, Jeff King, Junio C Hamano, Thomas Rast. Thread: https://gitlist.dev/t/35922 ## Nguyễn Thái Ngọc Duy, 2014-02-20 23:47 Subject: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry Message-ID: <1392940067-4830-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1392940067-4830-1-git-send-email-pclouds%40gmail.com ``` This delta_stack array can grow to any length depending on the actual delta chain, but we forget to free it. Normally it does not matter because we use small_delta_stack[] from stack and small_delta_stack can hold 64-delta chains, more than standard --depth=50 in pack-objects. Signed-off-by: Nguyễn Thái Ngọc Duy --- Found when trying to see if making some objects loose at this phase could help git-blame and how many objects will be loosened. Gotta go soon, didn't really test it, but I bet it'll work. sha1_file.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sha1_file.c b/sha1_file.c index 6e8c05d..57ab15d 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2289,6 +2289,10 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset, *final_size = size; unuse_pack(&w_curs); + + if (delta_stack != small_delta_stack) + free(delta_stack); + return data; } -- 1.9.0.40.gaa8c3ea ``` ## Jeff King, 2014-02-21 05:41 Subject: Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry Message-ID: <20140221054148.GA24882@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140221054148.GA24882%40sigill.intra.peff.net In-Reply-To: <1392940067-4830-1-git-send-email-pclouds@gmail.com> ``` On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote: > This delta_stack array can grow to any length depending on the actual > delta chain, but we forget to free it. Normally it does not matter > because we use small_delta_stack[] from stack and small_delta_stack > can hold 64-delta chains, more than standard --depth=50 in pack-objects. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Found when trying to see if making some objects loose at this phase > could help git-blame and how many objects will be loosened. Gotta go > soon, didn't really test it, but I bet it'll work. This looks correct to me. For fun, I measured "git fsck" on linux.git via massif. The peak memory usage dropped from 368MB to 306MB. That's probably an extreme case because it's a big repo and was packed with "--aggressive", but it's still a nice improvement. -Peff ``` ## Junio C Hamano, 2014-02-21 18:09 Subject: Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry Message-ID: URL: https://gitlist.dev/e/xmqqmwhk49sy.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140221054148.GA24882@sigill.intra.peff.net> ``` Jeff King writes: > On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote: > >> This delta_stack array can grow to any length depending on the actual >> delta chain, but we forget to free it. Normally it does not matter >> because we use small_delta_stack[] from stack and small_delta_stack >> can hold 64-delta chains, more than standard --depth=50 in pack-objects. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> Found when trying to see if making some objects loose at this phase >> could help git-blame and how many objects will be loosened. Gotta go >> soon, didn't really test it, but I bet it'll work. > > This looks correct to me. This comes from abe601bb, right? The change looks correct to me, too. > For fun, I measured "git fsck" on linux.git via massif. The peak memory > usage dropped from 368MB to 306MB. That's probably an extreme case > because it's a big repo and was packed with "--aggressive", but it's > still a nice improvement. Thanks. ``` ## Jeff King, 2014-02-22 08:12 Subject: Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry Message-ID: <20140222081231.GC1576@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140222081231.GC1576%40sigill.intra.peff.net In-Reply-To: ``` On Fri, Feb 21, 2014 at 10:09:33AM -0800, Junio C Hamano wrote: > >> This delta_stack array can grow to any length depending on the actual > >> delta chain, but we forget to free it. Normally it does not matter > >> because we use small_delta_stack[] from stack and small_delta_stack > >> can hold 64-delta chains, more than standard --depth=50 in pack-objects. > [...] > > This comes from abe601bb, right? The change looks correct to me, too. Yes, it does. -Peff ``` ## Thomas Rast, 2014-02-22 15:44 Subject: Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry Message-ID: <87ob1zrw3e.fsf@thomasrast.ch> URL: https://gitlist.dev/e/87ob1zrw3e.fsf%40thomasrast.ch In-Reply-To: ``` Junio C Hamano writes: > Jeff King writes: > >> On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote: >> >>> This delta_stack array can grow to any length depending on the actual >>> delta chain, but we forget to free it. Normally it does not matter >>> because we use small_delta_stack[] from stack and small_delta_stack >>> can hold 64-delta chains, more than standard --depth=50 in pack-objects. >>> >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>> Found when trying to see if making some objects loose at this phase >>> could help git-blame and how many objects will be loosened. Gotta go >>> soon, didn't really test it, but I bet it'll work. >> >> This looks correct to me. > > This comes from abe601bb, right? The change looks correct to me, too. Ow, sorry about that. Thanks for the fix! -- Thomas Rast tr@thomasrast.ch ```