Re: memory leak when cloning a repository
- From
Jacob Keller <jacob.keller@gmail.com>
- Date
- Mar 5, 2026, 23:16 UTC
- Message-ID
- <CA+P7+xp7HTykrBdr8WKb__M3Hj09-WQ6HRrTb9ZiHbWV1U=GhA@mail.gmail.com>
- In-Reply-To
- <20260305220214.GB736322@coredump.intra.peff.net>
On Thu, Mar 5, 2026 at 2:02 PM Jeff King <peff@peff.net> wrote:
Show 42 quoted lines
>
> 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.
Show 23 quoted lines
> > 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.
Show 8 quoted lines
> 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 >