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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jun 30, 2026 at 02:43:01AM -0400, Jeff King wrote:
> In prepare_bases() we do a custom revision walk, separate from the main
> format-patch walk. After we finish, we fail to call release_revisions(),
> possibly leaking its contents.
> 
> We failed to notice it so far because the revision machinery doesn't
> always allocate. But at least one case can trigger the leak: if a commit
> graph is present, then the topo-walk allocates revs.topo_walk_info and
> some associated data structures. You can see it in the test suite by
> running:
> 
>   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.

> The un-released rev_info has been there since the code was added in
> fa2ab86d18 (format-patch: add '--base' option to record base tree info,
> 2016-04-26), but back then we didn't even have a way to release rev_info
> resources! The actual leak probably started around f0d9cc4196
> (revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's
> hard to bisect because there were so many other unrelated leaks back
> then.
> 
> So I'm not sure exactly when the leak started beyond "long ago", but it
> is easy-ish to find now (since we've plugged all those other leaks) and
> the solution is clear.
> 
> I didn't add a new test since we can demonstrate it with the existing
> ones, but it does require tweaking a test variable. We might consider
> ways to get more automatic leak-checking coverage there, but I think it
> should be done outside of this fix.

Yeah, agreed.

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.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/log.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index d027ce1e0b..350b35c556 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,
>  		bases->nr_patch_id++;
>  	}
>  	clear_commit_base(&commit_base);
> +	release_revisions(&revs);
>  }

The fix looks sensible to me. We always initialize `revs` before we take
this exit path here, and there is no other early return that we'd have
to adjust.

Thanks!

Patrick