[PATCH 0/2] small leak fix in format-patch

[PATCH 0/2] small leak fix in format-patch

[Jeff King]

This fixes a leak I found while discussing an unrelated leak in another
thread[1]. As a bonus, this fixes some minor recent breakage of
leak-reporting when running the test suite under prove. The patches can
be split into separate topics if we want.

  [1/2]: t: move LSan errors from stdout to stderr
  [2/2]: format-patch: fix leak of rev_info in prepare_bases()

 builtin/log.c | 1 +
 t/test-lib.sh | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

-Peff

[1] https://lore.kernel.org/git/20260630055026.GE2495216@coredump.intra.peff.net/

[PATCH 1/2] t: move LSan errors from stdout to stderr

[Jeff King]

When we find LSan errors, we dump them via "say_color", which goes to
stdout. This is mostly harmless, since stdout and stderr tend to go to
the same place (either the user's terminal, or to the ".out" file with
--verbose-log).

But when running under a TAP harness like prove, they are split and
stdout is interpreted as TAP output. Historically even this was fine, as
the extra lines on stdout would be ignored. But since 389c83025d (t: let
prove fail when parsing invalid TAP output, 2026-06-04) we instruct the
TAP reader to complain, and a leaking test will result in complaints
like this (this is a real leak which we have yet to fix):

  $ GIT_TEST_COMMIT_GRAPH=1 make SANITIZE=leak test
  [...]
  Test Summary Report
  -------------------
  t4014-format-patch.sh (Wstat: 256 (exited 1) Tests: 226 Failed: 30)
    Failed tests:  197-226
    Non-zero exit status: 1
    Parse errors: Unknown TAP token: ""
                  Unknown TAP token: "================================================================="
                  Unknown TAP token: "==git==3693658==ERROR: LeakSanitizer: detected memory leaks"
                  Unknown TAP token: ""
                  Unknown TAP token: "Direct leak of 200 byte(s) in 1 object(s) allocated from:"
  Displayed the first 5 of 1531 TAP syntax errors.
  Re-run prove with the -p option to see them all.

You still see the failing tests, so it's mostly just an annoyance. We
can fix it by redirecting to stderr (actually descriptor 4, which is our
verbose-respecting variant). I confirmed manually that the output still
appears with --verbose-log, and even with a single-test "-i
--verbose-only=197" going to the terminal.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ceefb99bff..d390c53ec1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1217,14 +1217,14 @@ check_test_results_san_file_ () {
 	then
 		return
 	fi &&
-	say_color error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
+	say_color >&4 error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
 
 	if test "$test_failure" = 0
 	then
-		say "Our logs revealed a memory leak, exit non-zero!" &&
+		say >&4 "Our logs revealed a memory leak, exit non-zero!" &&
 		invert_exit_code=t
 	else
-		say "Our logs revealed a memory leak..."
+		say >&4 "Our logs revealed a memory leak..."
 	fi
 }
 
-- 
2.55.0.346.g83d0ea82e4

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

[Jeff King]

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

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.

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);
 }
 
 static void print_bases(struct base_tree_info *bases, FILE *file)
-- 
2.55.0.346.g83d0ea82e4

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

[Patrick Steinhardt]

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