Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()

[Jeff King <peff@peff.net>]

On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:

> >   make SANITIZE=leak
> >   cd t
> >   GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
> > 
> > which yields many entries like:
> > 
> >   ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> >   Direct leak of 200 byte(s) in 1 object(s) allocated from:
> >       #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> >       #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> >       #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> >       #3 0x55cd45255473 in init_topo_walk revision.c:3845
> >       #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> >       #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> >       #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
> 
> Interesting. Makes me wonder whether we should modify linux-TEST-vars to
> also run with the leak checker enabled. Ideally we'd of course just do
> this for all jobs, but the overhead is probably way too high... yes,
> doing a simple benchmark shows a ~3x hit.
> 
> So this is definitely nothing we want to do for all jobs. But for the
> linux-TEST-vars job it might make sense, as it exercises a bunch of
> non-default code paths.

We already run a special leak job for linux-reftables. Why not turn that
job into "leaks plus reftables plus test-vars"? The only downside would
be potentially hiding leaks found by linux-reftables-leaks if the
test-vars features force us into a difference code path. But looking at
the list, it doesn't seem likely to me. None of them is particularly
ref-related.

In fact, I kind of wonder if we could fold linux-reftables into the
test-vars job completely.

> One thing worth noting: there are still six test suites that are failing
> with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
> failures are because of the missing call to `repo_unuse_commit_buffer()`
> in git-history(1), which we already noted elsewhere.
> 
> All of the remaining leaks in t0095, t4013 and t4211 seem to be related
> to bloom filters.

I sent some patches to fix the bloom-filter cases.

Building with OPENSSL_SHA1_UNSAFE turns up more. The core issue is that
recent versions of openssl require an allocation to open a sha1 context,
and we free it in git_hash_final(). So code paths that abort mid-hash
will leak the allocation, and we need a git_hash_discard().

It comes up mostly with csum-file.[ch], since that's where we use the
unsafe variant.

If you further build with OPENSSL_SHA1 (using it for _all_ hash
computations), there are a few more cases. It's hard to care too much
since that isn't a recommended build (and we've even discussed dropping
support for non-dc sha1 totally). But sha256 has the same issue, so
we'll want to fix it eventually (I didn't try leak-checking the
linux-sha256 build, but I expect it would complain a lot).

I have some patches but they need a bit of polish. In particular I think
we'll have to tweak the hash.h #define mess to expose a "discard"
primitive from each implementation (otherwise we have to finalize the
hash to discard, which is a little inefficient). I didn't quite have the
stomach for that tonight.

-Peff