From: Jacob Keller Date: Thu, 05 Mar 2026 23:16:35 GMT Subject: Re: memory leak when cloning a repository Message-ID: In-Reply-To: <20260305220214.GB736322@coredump.intra.peff.net> On Thu, Mar 5, 2026 at 2:02 PM Jeff King wrote: > > On Thu, Mar 05, 2026 at 12:51:17PM -0800, Jacob Keller wrote: > > > I tried digging into why this leak occurs but so far I don't have a good idea. > > > > This happens when running on next: 7842e34a6654 ("Sync with 'master'") > > I can reproduce it on master. This seems to fix it: > > diff --git a/connected.c b/connected.c > index 79403108dd..e0f8ff38cb 100644 > --- a/connected.c > +++ b/connected.c > @@ -90,6 +90,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > promisor_pack_found: > ; > } while ((oid = fn(cb_data)) != NULL); > + close_pack(new_pack); > free(new_pack); > return 0; > } > @@ -128,6 +129,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > rev_list.no_stderr = opt->quiet; > > if (start_command(&rev_list)) { > + close_pack(new_pack); > free(new_pack); > return error(_("Could not run 'git rev-list'")); > } > @@ -162,6 +164,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > err = error_errno(_("failed to close rev-list's stdin")); > > sigchain_pop(SIGPIPE); > + close_pack(new_pack); > free(new_pack); > return finish_command(&rev_list) || err; > } > > > I think this has been leaky forever, but it's usually leaking a single > mmap, so nobody notices. But I noticed something odd about your trace: > Wow thanks for the quick response. I tried looking at this but I wasn't sure where it was correct to put the pack and I was having trouble tracking the storage of the mmap through the compat_mmap. Yea, we're leaking but its not a huge deal if the program is about to exit generally. > > Direct leak of 27168 byte(s) in 1 object(s) allocated from: > > #0 0x7f0e100e6f2b in malloc (/lib64/libasan.so.8+0xe6f2b) (BuildId: 25975f766867e9e604dc5a71a8befeaed3301942) > > #1 0x00000122ab77 in git_mmap ../compat/mmap.c:15 > > #2 0x000001169466 in xmmap_gently ../wrapper.c:884 > > #3 0x00000116959b in xmmap ../wrapper.c:907 > > #4 0x000000d168fd in check_packed_git_idx ../packfile.c:179 > > #5 0x000000d16cce in open_pack_index ../packfile.c:282 > > #6 0x000000d25273 in find_pack_entry_one ../packfile.c:2078 > > #7 0x00000099f969 in check_connected ../connected.c:148 > > We're in the compat git_mmap, which implies you're building with > NO_MMAP. We turn that on automatically when building with ASan (so that > we can detect single-byte overflows even when mmap would round up to a > page boundary). But as a side effect, the "mmap" for index and pack data > is done with a heap-allocated buffer. So now ASan/LSan will notice and > complain about it. > > We usually disable leak-checking for our ASan builds, so we wouldn't run > the tests with the compat mmap. And our leak-checking builds use LSan, > which doesn't set NO_MMAP. But if you combine them with: > > make SANITIZE=address,leak > Right. I built with meson and set the option to build with -fsanitize=address. I might have set leak too I am not certain. I was not aware of the NO_MMAP. > or even just build with: > > make NO_MMAP=MallocHarder SANITIZE=leak > > then the leak will be reported. I guess maybe you're building with > SANITIZE=address, but then running the result independently, without > setting ASAN_OPTIONS=detect_leaks=0. > Ya, I don't have that set. The only options I have is (as of recently) to set LSAN_OPTIONS=exit_code=0 to avoid changing the exit code on a leak detection (after many hours wondering why my bash completion was failing due to the leak I reported a while ago...) > Anyway, I think the solution is probably something like the patch above, > though probably it needs to cover the case where new_pack is NULL. > I can double check that later today. Its low priority, but I do think it is important to avoid leaks since code can be refactored into library status over time where a leak becomes more problematic. > -Peff >