memory leak when cloning a repository

memory leak when cloning a repository

[Jacob Keller]

Hi,

I recently ran into a memory leak that appears consistently when I clone
a repository:

=================================================================
==581989==ERROR: LeakSanitizer: detected memory leaks

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
    #8 0x0000004dabdb in update_remote_refs ../builtin/clone.c:550
    #9 0x0000004dabdb in cmd_clone ../builtin/clone.c:1602
    #10 0x00000080a9b4 in run_builtin ../git.c:506
    #11 0x00000080a9b4 in handle_builtin ../git.c:780
    #12 0x000000810727 in run_argv ../git.c:863
    #13 0x000000810727 in cmd_main ../git.c:985
    #14 0x000000449e6f in main ../common-main.c:9
    #15 0x7f0e0f6105b4 in __libc_start_call_main (/lib64/libc.so.6+0x35b4) (BuildId: ff0267465bc3d76e21003b3bc5598fd5ee63e261)
    #16 0x7f0e0f610667 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x3667) (BuildId: ff0267465bc3d76e21003b3bc5598fd5ee63e261)
    #17 0x00000044c264 in _start (/home/jekeller/libexec/git-core/git+0x44c264) (BuildId: f75e04052d9435ea15ebf4480b490fe2eb150d92)

SUMMARY: AddressSanitizer: 27168 byte(s) leaked in 1 allocation(s).

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'")

Thanks,
Jake

Re: memory leak when cloning a repository

[Jeff King]

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:

> 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

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.

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.

-Peff

[PATCH 0/4] plugging some mmap() leaks

[Jeff King]

On Thu, Mar 05, 2026 at 05:02:14PM -0500, Jeff King wrote:

> 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.

So here is a more polished version. I decided to try running the whole
test suite with leak-checking and NO_MMAP, and it turned up one other
case. This series fixes that, too, and then turns on the flag for all
leak-checking builds.

  [1/4]: check_connected(): delay opening new_pack
  [2/4]: check_connected(): fix leak of pack-index mmap
  [3/4]: pack-revindex: avoid double-loading .rev files
  [4/4]: Makefile: turn on NO_MMAP when building with LSan

 Makefile        |  1 +
 connected.c     | 38 +++++++++++++++++++-------------------
 pack-revindex.c |  4 ++++
 3 files changed, 24 insertions(+), 19 deletions(-)

-Peff

[PATCH 1/4] check_connected(): delay opening new_pack

[Jeff King]

In check_connected(), if the transport tells us we got a single packfile
that has already been verified as self-contained and connected, then we
can skip checking connectivity for any tips that are mentioned in that
pack. This goes back to c6807a40dc (clone: open a shortcut for
connectivity check, 2013-05-26).

We don't need to open that pack until we are about to start sending oids
to our child rev-list process, since that's when we check whether they
are in the self-contained pack. Let's push the opening of that pack
further down in the function. That saves us from having to clean it up
when we leave the function early (and by the time have opened the
rev-list process, we never leave the function early, since we have to
clean up the child process).

Signed-off-by: Jeff King <peff@peff.net>
---
One thing I noticed here is that for a clone with a single
self-contained pack, we could probably skip running rev-list entirely. I
don't know if it matters much, though, as a noop rev-list process is not
that expensive compared to the cost of a clone. And in the worst case,
it would involve calling find_pack_entry() on each proposed ref tip an
extra time only to find that at least one does need to be sent. Though
that is also not very expensive.

I left it out of this series, though it would involve moving the
new_pack opening up above the start_command() invocation again.

I also wondered if this whole thing out to be written to avoid a one-off 
packed_git in the first place, like:

  - call reprepare_packed_git() to re-scan objects/pack

  - find the pack by name in the packed_git list

  - don't clean it up; it's owned by the repository struct now

But that's a somewhat bigger change, and I'm not sure it really buys us
that much.

 connected.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/connected.c b/connected.c
index 79403108dd..530357de54 100644
--- a/connected.c
+++ b/connected.c
@@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		return err;
 	}
 
-	if (transport && transport->smart_options &&
-	    transport->smart_options->self_contained_and_connected &&
-	    transport->pack_lockfiles.nr == 1 &&
-	    strip_suffix(transport->pack_lockfiles.items[0].string,
-			 ".keep", &base_len)) {
-		struct strbuf idx_file = STRBUF_INIT;
-		strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
-			   base_len);
-		strbuf_addstr(&idx_file, ".idx");
-		new_pack = add_packed_git(the_repository, idx_file.buf,
-					  idx_file.len, 1);
-		strbuf_release(&idx_file);
-	}
-
 	if (repo_has_promisor_remote(the_repository)) {
 		/*
 		 * For partial clones, we don't want to have to do a regular
@@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 promisor_pack_found:
 			;
 		} while ((oid = fn(cb_data)) != NULL);
-		free(new_pack);
 		return 0;
 	}
 
@@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	else
 		rev_list.no_stderr = opt->quiet;
 
-	if (start_command(&rev_list)) {
-		free(new_pack);
+	if (start_command(&rev_list))
 		return error(_("Could not run 'git rev-list'"));
-	}
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
 	rev_list_in = xfdopen(rev_list.in, "w");
 
+	if (transport && transport->smart_options &&
+	    transport->smart_options->self_contained_and_connected &&
+	    transport->pack_lockfiles.nr == 1 &&
+	    strip_suffix(transport->pack_lockfiles.items[0].string,
+			 ".keep", &base_len)) {
+		struct strbuf idx_file = STRBUF_INIT;
+		strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
+			   base_len);
+		strbuf_addstr(&idx_file, ".idx");
+		new_pack = add_packed_git(the_repository, idx_file.buf,
+					  idx_file.len, 1);
+		strbuf_release(&idx_file);
+	}
+
 	do {
 		/*
 		 * If index-pack already checked that:
-- 
2.53.0.786.g466665faa3

[PATCH 2/4] check_connected(): fix leak of pack-index mmap

[Jeff King]

Since c6807a40dc (clone: open a shortcut for connectivity check,
2013-05-26), we may open a one-off packed_git struct to check what's in
the pack we just received. At the end of the function we throw away the
struct (rather than linking it into the repository struct as usual).

We used to leak the struct until dd4143e7bf (connected.c: free the
"struct packed_git", 2022-11-08), which calls free(). But that's not
sufficient; inside the struct we'll have mmap'd the pack idx data from
disk, which needs an munmap() call.

Building with SANITIZE=leak doesn't detect this, because we are leaking
our own mmap(), and it only finds heap allocations from malloc(). But if
we use our compat mmap implementation like this:

  make NO_MMAP=MapsBecomeMallocs SANITIZE=leak

then LSan will notice the leak, because now it's a regular heap buffer
allocated by malloc().

We can fix it by calling close_pack(), which will free any associated
memory. Note that we need to check for NULL ourselves; unlike free(), it
is not safe to pass a NULL pointer to close_pack().

Signed-off-by: Jeff King <peff@peff.net>
---
 connected.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/connected.c b/connected.c
index 530357de54..6718503649 100644
--- a/connected.c
+++ b/connected.c
@@ -159,6 +159,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		err = error_errno(_("failed to close rev-list's stdin"));
 
 	sigchain_pop(SIGPIPE);
-	free(new_pack);
+	if (new_pack) {
+		close_pack(new_pack);
+		free(new_pack);
+	}
 	return finish_command(&rev_list) || err;
 }
-- 
2.53.0.786.g466665faa3

[PATCH 3/4] pack-revindex: avoid double-loading .rev files

[Jeff King]

The usual entry point for loading the pack revindex is the
load_pack_revindex() function. It returns immediately if the packed_git
has a non-NULL revindex or revindex data field (representing an
in-memory or mmap'd .rev file, respectively), since the data is already
loaded.

But in 5a6072f631 (fsck: validate .rev file header, 2023-04-17) the fsck
code path switched to calling load_pack_revindex_from_disk() directly,
since it wants to check the on-disk data (if there is any). But that
function does _not_ check to see if the data has already been loaded; it
just maps the file, overwriting the revindex_map pointer (and pointing
revindex_data inside that map). And in that case we've leaked the mmap()
pointed to by revindex_map (if it was non-NULL).

This usually doesn't happen, since fsck wouldn't need to load the
revindex for any reason before we get to these checks. But there are
some cases where it does. For example, is_promisor_object() runs
odb_for_each_object() with the PACK_ORDER flag, which uses the revindex.

This happens a few times in our test suite, but SANITIZE=leak doesn't
detect it because we are leaking an mmap(), not a heap-allocated buffer
from malloc(). However, if you build with NO_MMAP, then our compat mmap
will read into a heap buffer instead, and LSan will complain. This
causes failures in t5601, t0410, t5702, and t5616.

We can fix it by checking for existing revindex_data when loading from
disk. This is redundant when we're called from load_pack_revindex(), but
it's a cheap check. The alternative is to teach check_pack_rev_indexes()
in fsck to skip the load, but that seems messier; it doesn't otherwise
know about internals like revindex_map and revindex_data.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-revindex.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pack-revindex.c b/pack-revindex.c
index 56cd803a67..1fe0afe899 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -277,6 +277,10 @@ int load_pack_revindex_from_disk(struct packed_git *p)
 {
 	char *revindex_name;
 	int ret;
+
+	if (p->revindex_data)
+		return 0;
+
 	if (open_pack_index(p))
 		return -1;
 
-- 
2.53.0.786.g466665faa3

[PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan

[Jeff King]

The past few commits fixed some cases where we leak memory allocated by
mmap(). Building with SANITIZE=leak doesn't detect these because it
covers only heap buffers allocated by malloc().

But if we build with NO_MMAP, our compat mmap() implementation will
allocate a heap buffer and pread() into it. And thus Lsan will detect
these leaks for free.

Using NO_MMAP is less performant, of course, since we have to use extra
memory and read in the whole file, rather than faulting in pages from
disk. But LSan builds are already slow, and this doesn't make them
measurably worse. Getting extra coverage for our leak-checking is worth
it.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index f3264d0a37..4cf1afd395 100644
--- a/Makefile
+++ b/Makefile
@@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 ifneq ($(filter leak,$(SANITIZERS)),)
 BASIC_CFLAGS += -O0
+NO_MMAP = CatchMapLeaks
 SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
-- 
2.53.0.786.g466665faa3

Re: memory leak when cloning a repository

[Jacob Keller]

On Thu, Mar 5, 2026 at 2:02 PM Jeff King <peff@peff.net> 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
>

Re: [PATCH 1/4] check_connected(): delay opening new_pack

[Jacob Keller]

On Thu, Mar 5, 2026 at 3:08 PM Jeff King <peff@peff.net> wrote:
>
> In check_connected(), if the transport tells us we got a single packfile
> that has already been verified as self-contained and connected, then we
> can skip checking connectivity for any tips that are mentioned in that
> pack. This goes back to c6807a40dc (clone: open a shortcut for
> connectivity check, 2013-05-26).
>
> We don't need to open that pack until we are about to start sending oids
> to our child rev-list process, since that's when we check whether they
> are in the self-contained pack. Let's push the opening of that pack
> further down in the function. That saves us from having to clean it up
> when we leave the function early (and by the time have opened the
> rev-list process, we never leave the function early, since we have to
> clean up the child process).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> One thing I noticed here is that for a clone with a single
> self-contained pack, we could probably skip running rev-list entirely. I
> don't know if it matters much, though, as a noop rev-list process is not
> that expensive compared to the cost of a clone. And in the worst case,
> it would involve calling find_pack_entry() on each proposed ref tip an
> extra time only to find that at least one does need to be sent. Though
> that is also not very expensive.
>
> I left it out of this series, though it would involve moving the
> new_pack opening up above the start_command() invocation again.
>
> I also wondered if this whole thing out to be written to avoid a one-off
> packed_git in the first place, like:
>
>   - call reprepare_packed_git() to re-scan objects/pack
>
>   - find the pack by name in the packed_git list
>
>   - don't clean it up; it's owned by the repository struct now
>
> But that's a somewhat bigger change, and I'm not sure it really buys us
> that much.

I agree, this seems like the best low hanging fruit improvement to
avoid the unnecessary cleanup.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>
>  connected.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index 79403108dd..530357de54 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>                 return err;
>         }
>
> -       if (transport && transport->smart_options &&
> -           transport->smart_options->self_contained_and_connected &&
> -           transport->pack_lockfiles.nr == 1 &&
> -           strip_suffix(transport->pack_lockfiles.items[0].string,
> -                        ".keep", &base_len)) {
> -               struct strbuf idx_file = STRBUF_INIT;
> -               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
> -                          base_len);
> -               strbuf_addstr(&idx_file, ".idx");
> -               new_pack = add_packed_git(the_repository, idx_file.buf,
> -                                         idx_file.len, 1);
> -               strbuf_release(&idx_file);
> -       }
> -
>         if (repo_has_promisor_remote(the_repository)) {
>                 /*
>                  * For partial clones, we don't want to have to do a regular
> @@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  promisor_pack_found:
>                         ;
>                 } while ((oid = fn(cb_data)) != NULL);
> -               free(new_pack);
>                 return 0;
>         }
>
> @@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>         else
>                 rev_list.no_stderr = opt->quiet;
>
> -       if (start_command(&rev_list)) {
> -               free(new_pack);
> +       if (start_command(&rev_list))
>                 return error(_("Could not run 'git rev-list'"));
> -       }
>
>         sigchain_push(SIGPIPE, SIG_IGN);
>
>         rev_list_in = xfdopen(rev_list.in, "w");
>
> +       if (transport && transport->smart_options &&
> +           transport->smart_options->self_contained_and_connected &&
> +           transport->pack_lockfiles.nr == 1 &&
> +           strip_suffix(transport->pack_lockfiles.items[0].string,
> +                        ".keep", &base_len)) {
> +               struct strbuf idx_file = STRBUF_INIT;
> +               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
> +                          base_len);
> +               strbuf_addstr(&idx_file, ".idx");
> +               new_pack = add_packed_git(the_repository, idx_file.buf,
> +                                         idx_file.len, 1);
> +               strbuf_release(&idx_file);
> +       }
> +
>         do {
>                 /*
>                  * If index-pack already checked that:
> --
> 2.53.0.786.g466665faa3
>
>

Re: [PATCH 2/4] check_connected(): fix leak of pack-index mmap

[Jacob Keller]

On Thu, Mar 5, 2026 at 3:10 PM Jeff King <peff@peff.net> wrote:
>
> Since c6807a40dc (clone: open a shortcut for connectivity check,
> 2013-05-26), we may open a one-off packed_git struct to check what's in
> the pack we just received. At the end of the function we throw away the
> struct (rather than linking it into the repository struct as usual).
>
> We used to leak the struct until dd4143e7bf (connected.c: free the
> "struct packed_git", 2022-11-08), which calls free(). But that's not
> sufficient; inside the struct we'll have mmap'd the pack idx data from
> disk, which needs an munmap() call.
>
> Building with SANITIZE=leak doesn't detect this, because we are leaking
> our own mmap(), and it only finds heap allocations from malloc(). But if
> we use our compat mmap implementation like this:
>
>   make NO_MMAP=MapsBecomeMallocs SANITIZE=leak
>
> then LSan will notice the leak, because now it's a regular heap buffer
> allocated by malloc().
>
> We can fix it by calling close_pack(), which will free any associated
> memory. Note that we need to check for NULL ourselves; unlike free(), it
> is not safe to pass a NULL pointer to close_pack().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Reviewed-by: Jacob Keller <jacob.keller@gmail.com>

Re: [PATCH 0/4] plugging some mmap() leaks

[Ramsay Jones]

On 05/03/2026 11:03 pm, Jeff King wrote:
> On Thu, Mar 05, 2026 at 05:02:14PM -0500, Jeff King wrote:
> 
>> 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.
> 
> So here is a more polished version. I decided to try running the whole
> test suite with leak-checking and NO_MMAP, and it turned up one other
> case. This series fixes that, too, and then turns on the flag for all
> leak-checking builds.
> 

Hmm, this gives me flash-backs. ;)

Many moons ago, when the cygwin build routinely set NO_MMAP I had an
valgrind build of git fail with a 'double free' caused by a call to
git_munmap() for a pointer that had already been git_munmap-ed!

In addition, the failure was not reproducible (or at least I could not
find such a test). This was at a time when the testsuite took 4+ hours
to run for a regular build, let alone a valgrind build. So, to try and
pin down the failure, I created a debug version of the mmap compat
functions, which I ran with for several weeks, without failing ... :(

It just so happens that about this time I was also testing running the
cygwin build without NO_MMAP set. This was a success, so I dropped
the NO_MMAP investigation, never having found the cause of the failure!

I have had the 'mmap' branch, with a version of the debug patch, in my
cygwin repo for ever (well, the 'author date' says sep 9th 2012, but I
know it was somewhat before then). This version of the patch removed the
'debug' output and was only concerned with the error return behaviour of
the 'emulated' syscalls. (it was also somewhat non-performant if you had
many mmap's; luckily, that wasn't the case then, and I tended to git-gc
very often - which I still do to this day!)

Anyway, just some food for thought. I have nearly deleted that branch
many times. I should probably do that now! (Hmm, patch given below just
FYI).

Thanks.

ATB,
Ramsay Jones

-------- >8 --------
From 40442aa06901720ec55005144438c8c733025cbb Mon Sep 17 00:00:00 2001
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Sun, 9 Sep 2012 20:50:32 +0100
Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug

When compiling with the NO_MMAP build variable set, the built-in
'git_mmap()' and 'git_munmap()' compatability routines use simple
memory allocation and file I/O to emulate the required behaviour.
The current implementation is vunerable to the "double-delete" bug
(where the pointer returned by malloc() is passed to free() two or
more times), should the mapped memory block address be passed to
munmap() multiple times.

In order to guard the implementation from such a calling sequence,
we keep a list of mmap-block descriptors, which we then consult to
determine the validity of the input pointer to munmap(). This then
allows 'git_munmap()' to return -1 on error, as required, with
errno set to EINVAL.

Using a list in the log of mmap-ed blocks, along with the resulting
linear search, means that the performance of the code is directly
proportional to the number of concurrently active memory mapped
file regions. The number of such regions is not expected to be
excessive.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 compat/mmap.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/compat/mmap.c b/compat/mmap.c
index 7f662fef7b..137c6dc005 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -1,14 +1,61 @@
 #include "../git-compat-util.h"
 
+struct mmbd {  /* memory mapped block descriptor */
+	struct mmbd *next;  /* next in list */
+	void   *start;      /* pointer to memory mapped block */
+	size_t length;      /* length of memory mapped block */
+};
+
+static struct mmbd *head;  /* head of mmb descriptor list */
+
+
+static void add_desc(struct mmbd *desc, void *start, size_t length)
+{
+	desc->start = start;
+	desc->length = length;
+	desc->next = head;
+	head = desc;
+}
+
+static void free_desc(struct mmbd *desc)
+{
+	if (head == desc)
+		head = head->next;
+	else {
+		struct mmbd *d = head;
+		for (; d; d = d->next) {
+			if (d->next == desc) {
+				d->next = desc->next;
+				break;
+			}
+		}
+	}
+	free(desc);
+}
+
+static struct mmbd *find_desc(void *start)
+{
+	struct mmbd *d = head;
+	for (; d; d = d->next) {
+		if (d->start == start)
+			return d;
+	}
+	return NULL;
+}
+
 void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
 	size_t n = 0;
+	struct mmbd *desc = NULL;
 
 	if (start != NULL || !(flags & MAP_PRIVATE))
 		die("Invalid usage of mmap when built with NO_MMAP");
 
 	start = xmalloc(length);
-	if (start == NULL) {
+	desc = xmalloc(sizeof(*desc));
+	if (!start || !desc) {
+		free(start);
+		free(desc);
 		errno = ENOMEM;
 		return MAP_FAILED;
 	}
@@ -23,18 +70,26 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 
 		if (count < 0) {
 			free(start);
+			free(desc);
 			errno = EACCES;
 			return MAP_FAILED;
 		}
 
 		n += count;
 	}
+	add_desc(desc, start, length);
 
 	return start;
 }
 
 int git_munmap(void *start, size_t length)
 {
+	struct mmbd *d = find_desc(start);
+	if (!d) {
+		errno = EINVAL;
+		return -1;
+	}
+	free_desc(d);
 	free(start);
 	return 0;
 }
-- 
2.53.0

Re: [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan

[Jacob Keller]

On 3/5/2026 3:13 PM, Jeff King wrote:
> The past few commits fixed some cases where we leak memory allocated by
> mmap(). Building with SANITIZE=leak doesn't detect these because it
> covers only heap buffers allocated by malloc().
> 
> But if we build with NO_MMAP, our compat mmap() implementation will
> allocate a heap buffer and pread() into it. And thus Lsan will detect
> these leaks for free.
> 
> Using NO_MMAP is less performant, of course, since we have to use extra
> memory and read in the whole file, rather than faulting in pages from
> disk. But LSan builds are already slow, and this doesn't make them
> measurably worse. Getting extra coverage for our leak-checking is worth
> it.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index f3264d0a37..4cf1afd395 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
>  ifneq ($(filter leak,$(SANITIZERS)),)
>  BASIC_CFLAGS += -O0
> +NO_MMAP = CatchMapLeaks
>  SANITIZE_LEAK = YesCompiledWithIt
>  endif
>  ifneq ($(filter address,$(SANITIZERS)),)

Should this patch also affect the meson.build?

There is the following in meson.build:

if host_machine.system() == 'windows'
  libgit_c_args += '-DUSE_WIN32_MMAP'
else
  checkfuncs += {
    # provided by compat/mingw.c.
    'unsetenv' : ['unsetenv.c'],
    # provided by compat/mingw.c.
    'getpagesize' : [],
  }

  if get_option('b_sanitize').contains('address')
    libgit_c_args += '-DNO_MMAP'
    libgit_sources += 'compat/mmap.c'
  else
    checkfuncs += { 'mmap': ['mmap.c'] }
  endif
endif

This probably needs to also check if it contains leak, no?

Also I think this might be somewhat less flexible than Make since you
can't forcibly enable mmap even with sanitizers enabled. I suppose thats
not a big deal since enabling sanitizers already has a high cost.

Thanks,
Jake

Re: [PATCH 0/4] plugging some mmap() leaks

[Jeff King]

On Fri, Mar 06, 2026 at 04:37:49AM +0000, Ramsay Jones wrote:

> Many moons ago, when the cygwin build routinely set NO_MMAP I had an
> valgrind build of git fail with a 'double free' caused by a call to
> git_munmap() for a pointer that had already been git_munmap-ed!
> 
> In addition, the failure was not reproducible (or at least I could not
> find such a test). This was at a time when the testsuite took 4+ hours
> to run for a regular build, let alone a valgrind build. So, to try and
> pin down the failure, I created a debug version of the mmap compat
> functions, which I ran with for several weeks, without failing ... :(
> 
> It just so happens that about this time I was also testing running the
> cygwin build without NO_MMAP set. This was a success, so I dropped
> the NO_MMAP investigation, never having found the cause of the failure!

Interesting. I guess a double-free via munmap() is probably a
harmless-ish noop, rather than a heap corruption. I could believe we
have such a bug somewhere, and it may even be racy (e.g., if it requires
reprepare_packed_git(), or maybe even has to do with stat freshness when
diff.c tries to reuse working tree files).

We've been testing ASan builds with NO_MMAP for a few months now, so
it's possible that might help flush it out. Though if you ran into it in
2012, it's possible it has since been unknowingly fixed. ;)

> Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug
> [...]
> In order to guard the implementation from such a calling sequence,
> we keep a list of mmap-block descriptors, which we then consult to
> determine the validity of the input pointer to munmap(). This then
> allows 'git_munmap()' to return -1 on error, as required, with
> errno set to EINVAL.

Gross. :)

This is a clever workaround, but I think we should consider it a bug if
we are calling munmap() twice and fix that.

-Peff

[PATCH 5/4] meson: turn on NO_MMAP when building with LSan

[Jeff King]

On Fri, Mar 06, 2026 at 01:17:24AM -0800, Jacob Keller wrote:

> > diff --git a/Makefile b/Makefile
> > index f3264d0a37..4cf1afd395 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
> >  endif
> >  ifneq ($(filter leak,$(SANITIZERS)),)
> >  BASIC_CFLAGS += -O0
> > +NO_MMAP = CatchMapLeaks
> >  SANITIZE_LEAK = YesCompiledWithIt
> >  endif
> >  ifneq ($(filter address,$(SANITIZERS)),)
> 
> Should this patch also affect the meson.build?

Ugh, yes.

I don't think we use meson in the CI sanitizer builds (which is where
I'd guess most leak-checking happens), but the two systems should remain
consistent.

Patch below (that can go on top or be squashed into 4/4).

> Also I think this might be somewhat less flexible than Make since you
> can't forcibly enable mmap even with sanitizers enabled. I suppose thats
> not a big deal since enabling sanitizers already has a high cost.

I don't pay much attention to the meson support, but yeah, it looks like
there's no equivalent to tweak the NO_MMAP knob independently there. I
doubt anybody is clamoring for it.

-- >8 --
Subject: [PATCH] meson: turn on NO_MMAP when building with LSan

The previous commit taught the Makefile to turn on NO_MMAP in this
instance. We should do the same with meson for consistency. We already
do this for ASan builds, so we can just tweak one conditional.

Signed-off-by: Jeff King <peff@peff.net>
---
Tested and confirmed this finds the test failures fixed by the earlier
patches.

 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 4b536e0124..4e13afbb41 100644
--- a/meson.build
+++ b/meson.build
@@ -1426,7 +1426,7 @@ else
     'getpagesize' : [],
   }
 
-  if get_option('b_sanitize').contains('address')
+  if get_option('b_sanitize').contains('address') or get_option('b_sanitize').contains('leak')
     libgit_c_args += '-DNO_MMAP'
     libgit_sources += 'compat/mmap.c'
   else
-- 
2.53.0.791.g8baeb4ea4d

Re: [PATCH 0/4] plugging some mmap() leaks

[Ramsay Jones]

On 06/03/2026 4:21 pm, Jeff King wrote:
> On Fri, Mar 06, 2026 at 04:37:49AM +0000, Ramsay Jones wrote:
> 
>> Many moons ago, when the cygwin build routinely set NO_MMAP I had an
>> valgrind build of git fail with a 'double free' caused by a call to
>> git_munmap() for a pointer that had already been git_munmap-ed!
>>
>> In addition, the failure was not reproducible (or at least I could not
>> find such a test). This was at a time when the testsuite took 4+ hours
>> to run for a regular build, let alone a valgrind build. So, to try and
>> pin down the failure, I created a debug version of the mmap compat
>> functions, which I ran with for several weeks, without failing ... :(
>>
>> It just so happens that about this time I was also testing running the
>> cygwin build without NO_MMAP set. This was a success, so I dropped
>> the NO_MMAP investigation, never having found the cause of the failure!
> 
> Interesting. I guess a double-free via munmap() is probably a
> harmless-ish noop, rather than a heap corruption. I could believe we
> have such a bug somewhere, and it may even be racy (e.g., if it requires
> reprepare_packed_git(), or maybe even has to do with stat freshness when
> diff.c tries to reuse working tree files).
> 
> We've been testing ASan builds with NO_MMAP for a few months now, so
> it's possible that might help flush it out. Though if you ran into it in
> 2012, it's possible it has since been unknowingly fixed. ;)

Yep, it was before 2012 and I suspect it has been 'fixed' (but could, of
course, still be lingering ...). ;)

>> Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug
>> [...]
>> In order to guard the implementation from such a calling sequence,
>> we keep a list of mmap-block descriptors, which we then consult to
>> determine the validity of the input pointer to munmap(). This then
>> allows 'git_munmap()' to return -1 on error, as required, with
>> errno set to EINVAL.
> 
> Gross. :)

Heh, agreed! :) There were several reasons I didn't submit it in all these
years.

> This is a clever workaround, but I think we should consider it a bug if
> we are calling munmap() twice and fix that.

Agreed. (I just wanted to bring this to your attention).

ATB,
Ramsay Jones

Re: [PATCH 5/4] meson: turn on NO_MMAP when building with LSan

[Ramsay Jones]

On 06/03/2026 4:25 pm, Jeff King wrote:
> On Fri, Mar 06, 2026 at 01:17:24AM -0800, Jacob Keller wrote:
> 
[snip]

>> Also I think this might be somewhat less flexible than Make since you
>> can't forcibly enable mmap even with sanitizers enabled. I suppose thats
>> not a big deal since enabling sanitizers already has a high cost.
> 
> I don't pay much attention to the meson support, but yeah, it looks like
> there's no equivalent to tweak the NO_MMAP knob independently there. I
> doubt anybody is clamoring for it.

Ignoring old Cygwin, IRIX, IRIX64, Minix, Nonstop and OS/390 set NO_MMAP
in the config.mak.uname file. I suspect none of them are using meson to
build git, so it shouldn't be an issue. (although I don't quite know why
I suspect that!).

[NOTE that NO_MMAP is not set in GIT-BUILD-OPTIONS, so ...]

ATB,
Ramsay Jones

Re: [PATCH 0/4] plugging some mmap() leaks

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> When compiling with the NO_MMAP build variable set, the built-in
> 'git_mmap()' and 'git_munmap()' compatability routines use simple
> memory allocation and file I/O to emulate the required behaviour.
> The current implementation is vunerable to the "double-delete" bug
> (where the pointer returned by malloc() is passed to free() two or
> more times), should the mapped memory block address be passed to
> munmap() multiple times.

Sorry if I am missing something glaringly obvious, but quite
honestly I am confused.  Wouldn't it be a bug to call munmap() again
on the same region of memory obtained from mmap() and then already
unmapped by calling munmap()?

Or can the emulation layer cause such a second free() even if the
munmap() is done once and only once per memory region obtained from
a single mmap()?

Re: [PATCH 0/4] plugging some mmap() leaks

[Ramsay Jones]

On 06/03/2026 6:37 pm, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> When compiling with the NO_MMAP build variable set, the built-in
>> 'git_mmap()' and 'git_munmap()' compatability routines use simple
>> memory allocation and file I/O to emulate the required behaviour.
>> The current implementation is vunerable to the "double-delete" bug
>> (where the pointer returned by malloc() is passed to free() two or
>> more times), should the mapped memory block address be passed to
>> munmap() multiple times.
> 
> Sorry if I am missing something glaringly obvious, but quite
> honestly I am confused.  Wouldn't it be a bug to call munmap() again
> on the same region of memory obtained from mmap() and then already
> unmapped by calling munmap()?

Yes. The (second) call to munmap() with the (already unmapped) memory
region would return -1 with errno set to EINVAL.

The emulation layer does not detect this situation and simply calls
free() on the given pointer. Hence the 'double-delete' bug.

> Or can the emulation layer cause such a second free() even if the
> munmap() is done once and only once per memory region obtained from
> a single mmap()?

No. If you only git_munmap() once for a given memory region, everything
is fine.

Thanks.

ATB,
Ramsay Jones

Re: [PATCH 0/4] plugging some mmap() leaks

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 06/03/2026 6:37 pm, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>> 
>>> When compiling with the NO_MMAP build variable set, the built-in
>>> 'git_mmap()' and 'git_munmap()' compatability routines use simple
>>> memory allocation and file I/O to emulate the required behaviour.
>>> The current implementation is vunerable to the "double-delete" bug
>>> (where the pointer returned by malloc() is passed to free() two or
>>> more times), should the mapped memory block address be passed to
>>> munmap() multiple times.
>> 
>> Sorry if I am missing something glaringly obvious, but quite
>> honestly I am confused.  Wouldn't it be a bug to call munmap() again
>> on the same region of memory obtained from mmap() and then already
>> unmapped by calling munmap()?
>
> Yes. The (second) call to munmap() with the (already unmapped) memory
> region would return -1 with errno set to EINVAL.
>
> The emulation layer does not detect this situation and simply calls
> free() on the given pointer. Hence the 'double-delete' bug.
>
>> Or can the emulation layer cause such a second free() even if the
>> munmap() is done once and only once per memory region obtained from
>> a single mmap()?
>
> No. If you only git_munmap() once for a given memory region, everything
> is fine.

Hmph.  You make it sound as if we have some code that calls munmap()
on something that we are not sure if we have unmapped just in case,
trusting that it won't crash us and instead give us EINVAL, and that
is very much deliberate?  Unless we have such a code, bending over
backwards to track what has already been unmapped and return -1 with
EINVAL from munmap() for a second call is of dubious value, no?  I
still must be missing something...

Thanks.

Re: [PATCH 0/4] plugging some mmap() leaks

[Ramsay Jones]

On 06/03/2026 10:05 pm, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> On 06/03/2026 6:37 pm, Junio C Hamano wrote:
>>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>>
>>>> When compiling with the NO_MMAP build variable set, the built-in
>>>> 'git_mmap()' and 'git_munmap()' compatability routines use simple
>>>> memory allocation and file I/O to emulate the required behaviour.
>>>> The current implementation is vunerable to the "double-delete" bug
>>>> (where the pointer returned by malloc() is passed to free() two or
>>>> more times), should the mapped memory block address be passed to
>>>> munmap() multiple times.
>>>
>>> Sorry if I am missing something glaringly obvious, but quite
>>> honestly I am confused.  Wouldn't it be a bug to call munmap() again
>>> on the same region of memory obtained from mmap() and then already
>>> unmapped by calling munmap()?
>>
>> Yes. The (second) call to munmap() with the (already unmapped) memory
>> region would return -1 with errno set to EINVAL.
>>
>> The emulation layer does not detect this situation and simply calls
>> free() on the given pointer. Hence the 'double-delete' bug.
>>
>>> Or can the emulation layer cause such a second free() even if the
>>> munmap() is done once and only once per memory region obtained from
>>> a single mmap()?
>>
>> No. If you only git_munmap() once for a given memory region, everything
>> is fine.
> 
> Hmph.  You make it sound as if we have some code that calls munmap()
> on something that we are not sure if we have unmapped just in case,
> trusting that it won't crash us and instead give us EINVAL, and that
> is very much deliberate?  Unless we have such a code, bending over
> backwards to track what has already been unmapped and return -1 with
> EINVAL from munmap() for a second call is of dubious value, no?  I
> still must be missing something...

Sorry to confuse you, I'm not trying to, honest! :)

First, forget that patch. It was not meant to be applied by anyone to
anything. It was just to (hopefully) support the explanation (to Jeff
specifically) of a bug which happened to me long ago.

In particular, valgrind demonstrated a bug, which has probably been fixed
in the intervening years, which directly implied that some code called
munmap() on a memory region which had already been unmapped.

Given that this was on cygwin and, at that time, was built with NO_MMAP,
this meant that free() was called twice on the same malloc()ed memory.
(Indeed this is what valgrind reported).

If you look at the (currently 31) calls to munmap(), only one seems to look
at the return (in refs/packed-backend.c:183). So, calling munmap() twice
on the same memory region will probably go unnoticed when NO_MMAP is not
set. I have no idea why munmap() was called twice on the same memory region,
since I didn't track down the code responsible.

It was just an FYI about a _potential_ lurking bug when using the mmap compat
routines.

Have I cleared that up, or confused you more. :)

ATB,
Ramsay Jones

Re: [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
>  ifneq ($(filter leak,$(SANITIZERS)),)
>  BASIC_CFLAGS += -O0
> +NO_MMAP = CatchMapLeaks
>  SANITIZE_LEAK = YesCompiledWithIt
>  endif
>  ifneq ($(filter address,$(SANITIZERS)),)

And of course, this "breaks" the leaks job at CI without being the
true culprit.

    https://github.com/git/git/actions/runs/22786105918/job/66103114142

My bisection between v2.52.0 and v2.53.0 with the following

    $ git bisect start v2.53.0 v2.52.0
    $ git bisect run sh :doit

where :doit has the shell script attached at the end of this message
blames this commit.  I didn't dig further than that.

commit 4c89d31494bff4bde6079a0e0821f1437e37d07b
Author: Patrick Steinhardt <ps@pks.im>
Date:   Sun Nov 23 19:59:37 2025 +0100

    streaming: rely on object sources to create object stream
    
    When creating an object stream we first look up the object info and, if
    it's present, we call into the respective backend that contains the
    object to create a new stream for it.
    
    This has the consequence that, for loose object source, we basically
    iterate through the object sources twice: we first discover that the
    file exists as a loose object in the first place by iterating through
    all sources. And, once we have discovered it, we again walk through all
    sources to try and map the object. The same issue will eventually also
    surface once the packfile store becomes per-object-source.
    
    Furthermore, it feels rather pointless to first look up the object only
    to then try and read it.
    
    Refactor the logic to be centered around sources instead. Instead of
    first reading the object, we immediately ask the source to create the
    object stream for us. If the object exists we get stream, otherwise
    we'll try the next source.
    
    Like this we only have to iterate through sources once. But even more
    importantly, this change also helps us to make the whole logic
    pluggable. The object read stream subsystem does not need to be aware of
    the different source backends anymore, but eventually it'll only have to
    call the source's callback function.
    
    Note that at the current point in time we aren't fully there yet:
    
      - The packfile store still sits on the object database level and is
        thus agnostic of the sources.
    
      - We still have to call into both the packfile store and the loose
        object source.
    
    But both of these issues will soon be addressed.
    
    This refactoring results in a slight change to semantics: previously, it
    was `odb_read_object_info_extended()` that picked the source for us, and
    it would have favored packed (non-deltified) objects over loose objects.
    And while we still favor packed over loose objects for a single source
    with the new logic, we'll now favor a loose object from an earlier
    source over a packed object from a later source.
    
    Ultimately this shouldn't matter though: the stream doesn't indicate to
    the caller which source it is from and whether it was created from a
    packed or loose object, so such details are opaque to the caller. And
    other than that we should be able to assume that two objects with the
    same object ID should refer to the same content, so the streamed data
    would be the same, too.
    
    Signed-off-by: Patrick Steinhardt <ps@pks.im>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

 streaming.c | 65 +++++++++++++++++++++++--------------------------------------
 1 file changed, 24 insertions(+), 41 deletions(-)


---- >8 ----

#!/bin/sh

git apply -3 <"$0" || {
	git reset --hard
	exit 125
}

(
	export SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true 
	make NO_MMAP=CatchMapLeaks CC=clang &&
	cd t && sh t1060-object-corruption.sh
)

status=$?

make distclean
git reset --hard

exit $status

diff --git i/compat/mmap.c w/compat/mmap.c
index 2fe1c7732e..1a118711f7 100644
--- i/compat/mmap.c
+++ w/compat/mmap.c
@@ -38,7 +38,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 	return start;
 }
 
-int git_munmap(void *start, size_t length)
+int git_munmap(void *start, size_t length UNUSED)
 {
 	free(start);
 	return 0;

Re: [PATCH 0/4] plugging some mmap() leaks

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> If you look at the (currently 31) calls to munmap(), only one seems to look
> at the return (in refs/packed-backend.c:183). So, calling munmap() twice
> on the same memory region will probably go unnoticed when NO_MMAP is not
> set. I have no idea why munmap() was called twice on the same memory region,
> since I didn't track down the code responsible.
>
> It was just an FYI about a _potential_ lurking bug when using the mmap compat
> routines.
>
> Have I cleared that up, or confused you more. :)

Oh, absolutely.  Thanks.

[PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()

[Jeff King]

On Fri, Mar 06, 2026 at 05:14:28PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
> >  endif
> >  ifneq ($(filter leak,$(SANITIZERS)),)
> >  BASIC_CFLAGS += -O0
> > +NO_MMAP = CatchMapLeaks
> >  SANITIZE_LEAK = YesCompiledWithIt
> >  endif
> >  ifneq ($(filter address,$(SANITIZERS)),)
> 
> And of course, this "breaks" the leaks job at CI without being the
> true culprit.
> 
>     https://github.com/git/git/actions/runs/22786105918/job/66103114142
> 
> My bisection between v2.52.0 and v2.53.0 with the following
> 
>     $ git bisect start v2.53.0 v2.52.0
>     $ git bisect run sh :doit
> 
> where :doit has the shell script attached at the end of this message
> blames this commit.  I didn't dig further than that.

Interesting. I ran my tests on "master", which would include v2.53.0,
and it came up clean. But I use gcc locally; switching to clang does
indeed report a leak for me.

Even more curiously, if I try testing the tip of jch, then gcc does find
the same leak! Bisecting, it starts to find the leak as of 1f3fd68e06
(odb/source: make `read_object_stream()` function pluggable,
2026-03-05).

There is a real leak here; the fix is below. But curiously, it is _not_
the fault of the commit you found by bisection.

In the test in question, we die() shortly after the leak happens. We've
definitely left the function that holds the pointer to the leaked
buffer, so it's a true leak. But my guess is that the leak detector
doesn't quite know which parts of stack memory are valid or not when we
die(), so it scans the whole thing looking for plausible pointers to
allocations. If it gets "lucky", then the stale out-of-scope pointer is
still in stack memory, and we consider it still reachable.

And whether that happens or not can depend on the compiler, or even
compile options. And as the code is refactored to use the more abstract
odb API (and call more functions), it is increasingly likely that
something else has re-used that bit of stack memory.

So that's why the leak "appears" in 4c89d31494 (streaming: rely on
object sources to create object stream, 2025-11-23) for clang, and
1f3fd68e06 (odb/source: make `read_object_stream()` function pluggable,
2026-03-05) for gcc. But it was really there all along.

Anyway, here's the fix. It should probably be slotted in before patch 4
(which turns on NO_MMAP for leak-check builds).

-- >8 --
Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()

We mmap() a loose object file, storing the result in the local variable
"mapped", which is eventually assigned into our stream struct as
"st.mapped". If we hit an error, we jump to an error label which does:

  munmap(st.mapped, st.mapsize);

to clean up. But this is wrong; we don't assign st.mapped until the end
of the function, after all of the "goto error" jumps. So this munmap()
is never cleaning up anything (st.mapped is always NULL, because we
initialize the struct with calloc).

Instead, we should feed the local variable to munmap().

This leak is due to 595296e124 (streaming: allocate stream inside the
backend-specific logic, 2025-11-23), which introduced the local
variable. Before that, we assigned the mmap result directly into
st.mapped. It was probably switched there so that we do not have to
allocate/free the struct when the map operation fails (e.g., because we
don't have the loose object). Before that commit, the struct was passed
in from the caller, so there was no allocation at all.

You can see the leak in the test suite by building with:

  make SANITIZE=leak NO_MMAP=1 CC=clang

and running t1060. We need NO_MMAP so that the mmap() is backed by an
actual malloc(), which allows LSan to detect it. And the leak seems not
to be detected when compiling with gcc, probably due to some internal
compiler decisions about how the stack memory is written.

Signed-off-by: Jeff King <peff@peff.net>
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index 3094140055..ab2fb9c4eb 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2197,7 +2197,7 @@ int odb_source_loose_read_object_stream(struct odb_read_stream **out,
 	return 0;
 error:
 	git_inflate_end(&st->z);
-	munmap(st->mapped, st->mapsize);
+	munmap(mapped, mapsize);
 	free(st);
 	return -1;
 }
-- 
2.53.0.791.g8baeb4ea4d

Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()
>
> We mmap() a loose object file, storing the result in the local variable
> "mapped", which is eventually assigned into our stream struct as
> "st.mapped". If we hit an error, we jump to an error label which does:
>
>   munmap(st.mapped, st.mapsize);
>
> to clean up. But this is wrong; we don't assign st.mapped until the end
> of the function, after all of the "goto error" jumps. So this munmap()
> is never cleaning up anything (st.mapped is always NULL, because we
> initialize the struct with calloc).
>
> Instead, we should feed the local variable to munmap().
>
> This leak is due to 595296e124 (streaming: allocate stream inside the
> backend-specific logic, 2025-11-23), which introduced the local
> variable. Before that, we assigned the mmap result directly into
> st.mapped. It was probably switched there so that we do not have to
> allocate/free the struct when the map operation fails (e.g., because we
> don't have the loose object). Before that commit, the struct was passed
> in from the caller, so there was no allocation at all.

Makes sense.  Thanks for finding and fixing the issue so quickly.


>
> You can see the leak in the test suite by building with:
>
>   make SANITIZE=leak NO_MMAP=1 CC=clang
>
> and running t1060. We need NO_MMAP so that the mmap() is backed by an
> actual malloc(), which allows LSan to detect it. And the leak seems not
> to be detected when compiling with gcc, probably due to some internal
> compiler decisions about how the stack memory is written.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  object-file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/object-file.c b/object-file.c
> index 3094140055..ab2fb9c4eb 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2197,7 +2197,7 @@ int odb_source_loose_read_object_stream(struct odb_read_stream **out,
>  	return 0;
>  error:
>  	git_inflate_end(&st->z);
> -	munmap(st->mapped, st->mapsize);
> +	munmap(mapped, mapsize);
>  	free(st);
>  	return -1;
>  }

Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()

[Patrick Steinhardt]

On Fri, Mar 06, 2026 at 09:35:08PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()
> >
> > We mmap() a loose object file, storing the result in the local variable
> > "mapped", which is eventually assigned into our stream struct as
> > "st.mapped". If we hit an error, we jump to an error label which does:
> >
> >   munmap(st.mapped, st.mapsize);
> >
> > to clean up. But this is wrong; we don't assign st.mapped until the end
> > of the function, after all of the "goto error" jumps. So this munmap()
> > is never cleaning up anything (st.mapped is always NULL, because we
> > initialize the struct with calloc).
> >
> > Instead, we should feed the local variable to munmap().
> >
> > This leak is due to 595296e124 (streaming: allocate stream inside the
> > backend-specific logic, 2025-11-23), which introduced the local
> > variable. Before that, we assigned the mmap result directly into
> > st.mapped. It was probably switched there so that we do not have to
> > allocate/free the struct when the map operation fails (e.g., because we
> > don't have the loose object). Before that commit, the struct was passed
> > in from the caller, so there was no allocation at all.
> 
> Makes sense.  Thanks for finding and fixing the issue so quickly.

Yup, indeed, this is an obvious fix. Thanks for cleaning up after me!

Patrick