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

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

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(-)