[PATCH 0/3] leak tests: mark remaining tests leak-free as such

[PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Taylor Blau]

While working on another topic that cleared up some leaks, I wanted to
see if any new tests became leak-free, so I ran:

    $ make SANITIZE=leak
    $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test

, and was surprised to see so many tests appear in the "failed" list
(indicating that they are free of leaks, but did not mark themselves as
such).

In fact, the patch I had written at the time didn't make a dent one way
or another in the list of leak-free tests, as the same results were
produced on 'master' without my changes.

This series marks all leak-free tests as such, meaning that the above
"make test" invocation will pass after this series. The bulk of the
tests which are marked here in the first patch were always
leak-free[^1]. The remaining two patches address a couple of special
cases of tests which are also leak-free.

Thanks in advance for your review!

[^1]: At least as far back as v2.38.0, when the "check" mode of
  GIT_TEST_PASSING_SANITIZE_LEAK was first introduced.

Taylor Blau (3):
  leak tests: mark a handful of tests as leak-free
  leak tests: mark t3321-notes-stripspace.sh as leak-free
  leak tests: mark t5583-push-branches.sh as leak-free

 t/t3321-notes-stripspace.sh | 1 +
 t/t5571-pre-push-hook.sh    | 1 +
 t/t5583-push-branches.sh    | 1 +
 t/t7516-commit-races.sh     | 2 ++
 4 files changed, 5 insertions(+)

-- 
2.42.0.3.g4011eb6a8b

[PATCH 1/3] leak tests: mark a handful of tests as leak-free

[Taylor Blau]

In the topic merged via 5a4f8381b6 (Merge branch
'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
were marked as leak-free.

As far as I can tell, each patch from that series ran tests from a
handful of subject areas, such as "some ls-files tests", or "all trace2
tests". This left some gaps in which tests had and hadn't been audited
to be leak-free.

This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
before sourcing t/test-lib.sh on most remaining leak-free tests. This
list was compiled by doing:

    $ make SANITIZE=leak
    $ make \
        GIT_TEST_PASSING_SANITIZE_LEAK=check \
        GIT_TEST_SANITIZE_LEAK_LOG=true \
        GIT_TEST_OPTS=-vi test

and looking through the list of failing tests in the output.

There are a couple of other tests which are similarly leak-free, but not
included in the list of tests touched by this patch. The remaining tests
will be addressed in the subsequent two patches.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5571-pre-push-hook.sh | 1 +
 t/t7516-commit-races.sh  | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index a11b20e378..448134c4bf 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -4,6 +4,7 @@ test_description='check pre-push hooks'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
index 2d38a16480..bb95f09810 100755
--- a/t/t7516-commit-races.sh
+++ b/t/t7516-commit-races.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git commit races'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'race to create orphan commit' '
-- 
2.42.0.3.g4011eb6a8b

Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free

[Jeff King]

On Thu, Aug 24, 2023 at 02:40:38PM -0400, Taylor Blau wrote:

> In the topic merged via 5a4f8381b6 (Merge branch
> 'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
> were marked as leak-free.
> 
> As far as I can tell, each patch from that series ran tests from a
> handful of subject areas, such as "some ls-files tests", or "all trace2
> tests". This left some gaps in which tests had and hadn't been audited
> to be leak-free.
> 
> This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
> before sourcing t/test-lib.sh on most remaining leak-free tests. This
> list was compiled by doing:
> 
>     $ make SANITIZE=leak
>     $ make \
>         GIT_TEST_PASSING_SANITIZE_LEAK=check \
>         GIT_TEST_SANITIZE_LEAK_LOG=true \
>         GIT_TEST_OPTS=-vi test

So having resolved my "oops, lsan logs are racy" problem, my system now
agrees with yours on which tests are now leak-free. And we definitely
_were_ leak free less than year ago when I posted that other patch. So
I'm not sure I buy the "these were missed in 5a4f8381b6" logic.

The one in t5571, I mentioned earlier that I bisected to 861c56f6f9
(branch: fix a leak in setup_tracking, 2023-06-11).

The one in t7516 seems to have been fixed by 866b43e644
(do_read_index(): always mark index as initialized unless erroring out,
2023-06-29).

I found both by bisecting between v2.39.0 (which shows the leak) and
v2.42.0 (which doesn't).

-Peff

Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free

[Taylor Blau]

On Thu, Aug 24, 2023 at 05:02:38PM -0400, Jeff King wrote:
> On Thu, Aug 24, 2023 at 02:40:38PM -0400, Taylor Blau wrote:
>
> > In the topic merged via 5a4f8381b6 (Merge branch
> > 'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
> > were marked as leak-free.
> >
> > As far as I can tell, each patch from that series ran tests from a
> > handful of subject areas, such as "some ls-files tests", or "all trace2
> > tests". This left some gaps in which tests had and hadn't been audited
> > to be leak-free.
> >
> > This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
> > before sourcing t/test-lib.sh on most remaining leak-free tests. This
> > list was compiled by doing:
> >
> >     $ make SANITIZE=leak
> >     $ make \
> >         GIT_TEST_PASSING_SANITIZE_LEAK=check \
> >         GIT_TEST_SANITIZE_LEAK_LOG=true \
> >         GIT_TEST_OPTS=-vi test
>
> So having resolved my "oops, lsan logs are racy" problem, my system now
> agrees with yours on which tests are now leak-free. And we definitely
> _were_ leak free less than year ago when I posted that other patch. So
> I'm not sure I buy the "these were missed in 5a4f8381b6" logic.

Phew ;-). Thanks for chasing that down, I'm glad that our two sides
agree.

> The one in t5571, I mentioned earlier that I bisected to 861c56f6f9
> (branch: fix a leak in setup_tracking, 2023-06-11).
>
> The one in t7516 seems to have been fixed by 866b43e644
> (do_read_index(): always mark index as initialized unless erroring out,
> 2023-06-29).
>
> I found both by bisecting between v2.39.0 (which shows the leak) and
> v2.42.0 (which doesn't).

Much appreciated. I'm happy to fold those details into a new round if
you think they are useful enough to live in the commit history. I could
grab your patch as a preparatory step, too. But if you are happy with
this as-is, I am too.

Thanks,
Taylor

Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free

[Jeff King]

On Fri, Aug 25, 2023 at 03:05:02PM -0400, Taylor Blau wrote:

> > The one in t5571, I mentioned earlier that I bisected to 861c56f6f9
> > (branch: fix a leak in setup_tracking, 2023-06-11).
> >
> > The one in t7516 seems to have been fixed by 866b43e644
> > (do_read_index(): always mark index as initialized unless erroring out,
> > 2023-06-29).
> >
> > I found both by bisecting between v2.39.0 (which shows the leak) and
> > v2.42.0 (which doesn't).
> 
> Much appreciated. I'm happy to fold those details into a new round if
> you think they are useful enough to live in the commit history. I could
> grab your patch as a preparatory step, too. But if you are happy with
> this as-is, I am too.

I would definitely mention them if writing the commit message from
scratch. I'm not sure if it is worth a re-roll or not.

I do think we should apply the racy-thread log fix, though. I thought we
had discussed it at the time, but there doesn't seem to be anything in
the archive. And I was willing to let it go as a weird one-off at the
time, but now that it wasted another 30 minutes of my life discovering
the problem again, I'm in favor of applying it.

Whether it happens as part of your re-rolled series, or is applied
separately, I am OK either way. :)

-Peff

Re: [PATCH 1/3] leak tests: mark a handful of tests as leak-free

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do think we should apply the racy-thread log fix, though. I thought we
> had discussed it at the time, but there doesn't seem to be anything in
> the archive. And I was willing to let it go as a weird one-off at the
> time, but now that it wasted another 30 minutes of my life discovering
> the problem again, I'm in favor of applying it.
>
> Whether it happens as part of your re-rolled series, or is applied
> separately, I am OK either way. :)

Whether it comes from you or Taylor, I do favor to see it as a new
message at lore archive than having to fish an older message from it
;-)

Thanks.

[PATCH] test-lib: ignore uninteresting LSan output

[Jeff King]

On Mon, Aug 28, 2023 at 11:24:50AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I do think we should apply the racy-thread log fix, though. I thought we
> > had discussed it at the time, but there doesn't seem to be anything in
> > the archive. And I was willing to let it go as a weird one-off at the
> > time, but now that it wasted another 30 minutes of my life discovering
> > the problem again, I'm in favor of applying it.
> >
> > Whether it happens as part of your re-rolled series, or is applied
> > separately, I am OK either way. :)
> 
> Whether it comes from you or Taylor, I do favor to see it as a new
> message at lore archive than having to fish an older message from it
> ;-)

I re-sent it already as part of this thread. But I guess the definition
of "older" may depend on whether you were paying attention to the thread
at that point. ;)

Here it is again. Let's just consider it as its own topic. There is
nothing in Taylor's series that depends on it (it's just that it was
useful for me to reproduce his findings).

-- >8 --
Subject: [PATCH] test-lib: ignore uninteresting LSan output

When I run the tests in leak-checking mode the same way our CI job does,
like:

  make SANITIZE=leak \
       GIT_TEST_PASSING_SANITIZE_LEAK=true \
       GIT_TEST_SANITIZE_LEAK_LOG=true \
       test

then LSan can racily produce useless entries in the log files that look
like this:

  ==git==3034393==Unable to get registers from thread 3034307.

I think they're mostly harmless based on the source here:

  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414

which reads:

    PtraceRegistersStatus have_registers =
        suspended_threads.GetRegistersAndSP(i, &registers, &sp);
    if (have_registers != REGISTERS_AVAILABLE) {
      Report("Unable to get registers from thread %llu.\n", os_id);
      // If unable to get SP, consider the entire stack to be reachable unless
      // GetRegistersAndSP failed with ESRCH.
      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
        continue;
      sp = stack_begin;
    }

The program itself still runs fine and LSan doesn't cause us to abort.
But test-lib.sh looks for any non-empty LSan logs and marks the test as
a failure anyway, under the assumption that we simply missed the failing
exit code somehow.

I don't think I've ever seen this happen in the CI job, but running
locally using clang-14 on an 8-core machine, I can't seem to make it
through a full run of the test suite without having at least one
failure. And it's a different one every time (though they do seem to
often be related to packing tests, which makes sense, since that is one
of our biggest users of threaded code).

We can hack around this by only counting LSan log files that contain a
line that doesn't match our known-uninteresting pattern.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 293caf0f20..5ea5d1d62a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {
 	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+	xargs grep -lv "Unable to get registers from thread" |
 	wc -l
 }
 
-- 
2.42.0.448.g0caf9a9e14

[PATCH 2/3] leak tests: mark t3321-notes-stripspace.sh as leak-free

[Taylor Blau]

This test was leak-free when t3321 was originally introduced, but never
marked as such:

    $ rev="$(git log --format='%H' --reverse -1 HEAD^ -- t/t3321-notes-stripspace.sh)"
    $ git checkout $rev

    $ make SANITIZE=leak
    [...]

    $ make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=--immediate t3321-notes-stripspace.sh
    [...]
    # passed all 27 test(s)
    1..27
    # faking up non-zero exit with --invert-exit-code
    make: *** [Makefile:66: t3321-notes-stripspace.sh] Error 1
    make: Leaving directory '/home/ttaylorr/src/git/t'

Mark this test as leak-free accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t3321-notes-stripspace.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh
index 028d825e8f..36abdca5ee 100755
--- a/t/t3321-notes-stripspace.sh
+++ b/t/t3321-notes-stripspace.sh
@@ -5,6 +5,7 @@
 
 test_description='Test commit notes with stripspace behavior'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 MULTI_LF="$LF$LF$LF"
-- 
2.42.0.3.g4011eb6a8b

[PATCH 3/3] leak tests: mark t5583-push-branches.sh as leak-free

[Taylor Blau]

When t5583-push-branches.sh was originally introduced via 425b4d7f47
(push: introduce '--branches' option, 2023-05-06), it was not leak-free.
In fact, the test did not even run correctly until 022fbb655d (t5583:
fix shebang line, 2023-05-12), but after applying that patch, we see a
failure at t5583.8:

    ==2529087==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 384 byte(s) in 1 object(s) allocated from:
        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
        #1 0x55e07606cbf9 in xrealloc wrapper.c:140
        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42
        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917
        #4 0x55e075fe9cce in add_missing_tags remote.c:1518
        #5 0x55e075fea1e4 in match_push_refs remote.c:1665
        #6 0x55e076050a8e in transport_push transport.c:1378
        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401
        #8 0x55e075e2edb0 in do_push builtin/push.c:458
        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702
        #10 0x55e075d8aaf0 in run_builtin git.c:452
        #11 0x55e075d8af08 in handle_builtin git.c:706
        #12 0x55e075d8b12c in run_argv git.c:770
        #13 0x55e075d8b6a0 in cmd_main git.c:905
        #14 0x55e075e81f07 in main common-main.c:60
        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360
        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)

    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).

This leak was addressed independently via 68b51172e3 (commit-reach: fix
memory leak in get_reachable_subset(), 2023-06-03), which makes t5583
leak-free.

But t5583 was not in the tree when 68b51172e3 was written, and the two
only met after the latter was merged back in via 693bde461c (Merge
branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).

At that point, t5583 was leak-free. Let's mark it as such accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5583-push-branches.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5583-push-branches.sh b/t/t5583-push-branches.sh
index e7e1b6dab6..320f49c753 100755
--- a/t/t5583-push-branches.sh
+++ b/t/t5583-push-branches.sh
@@ -5,6 +5,7 @@ test_description='check the consisitency of behavior of --all and --branches'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 delete_refs() {
-- 
2.42.0.3.g4011eb6a8b

Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> This series marks all leak-free tests as such, meaning that the above
> "make test" invocation will pass after this series. The bulk of the
> tests which are marked here in the first patch were always
> leak-free[^1]. The remaining two patches address a couple of special
> cases of tests which are also leak-free.
>
> Thanks in advance for your review!
>
> [^1]: At least as far back as v2.38.0, when the "check" mode of
>   GIT_TEST_PASSING_SANITIZE_LEAK was first introduced.

Nice to see "bugs" gone without any code changes ;-) It would have
taken a lot of patience from your part, though, and your effort is
very much appreciated.



> Taylor Blau (3):
>   leak tests: mark a handful of tests as leak-free
>   leak tests: mark t3321-notes-stripspace.sh as leak-free
>   leak tests: mark t5583-push-branches.sh as leak-free
>
>  t/t3321-notes-stripspace.sh | 1 +
>  t/t5571-pre-push-hook.sh    | 1 +
>  t/t5583-push-branches.sh    | 1 +
>  t/t7516-commit-races.sh     | 2 ++
>  4 files changed, 5 insertions(+)

Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Jeff King]

On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:

> While working on another topic that cleared up some leaks, I wanted to
> see if any new tests became leak-free, so I ran:
> 
>     $ make SANITIZE=leak
>     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test

Is that exactly what you ran? Because I'd expect the second "make"
invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.
(Though I would have then expected most of the scripts to complain
loudly about the mismatch; did you "cd t" in between the two?).

>  t/t3321-notes-stripspace.sh | 1 +
>  t/t5571-pre-push-hook.sh    | 1 +
>  t/t5583-push-branches.sh    | 1 +
>  t/t7516-commit-races.sh     | 2 ++
>  4 files changed, 5 insertions(+)

If I run a single:

  make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test

on v2.42.0, I get many hits. All of the ones you mentioned, plus:

  t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016

If I run a few by hand, I _do_ see leaks in them, but the exit codes are
hidden from the test suite (they are sub-programs of scripts, etc). I
guess you also have:

  GIT_TEST_SANITIZE_LEAK_LOG=true

set, which should find those (and which you mention in your first
commit). Turning that on eliminates some of them, but I'm left with:

  t5614 t5317 t5503

not in your list. Which is super weird, because t5614 is marked with
TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a
_different_ set (t5614 again, along with your 4, but also t5303, t7701,
and t4050). I wonder if we have a race in the leak-log code or
something (I'm running under prove with -j32, naturally).

> This series marks all leak-free tests as such, meaning that the above
> "make test" invocation will pass after this series. The bulk of the
> tests which are marked here in the first patch were always
> leak-free[^1]. The remaining two patches address a couple of special
> cases of tests which are also leak-free.

Hmm. If I check t5571, for example, by bisecting on:

  make SANITIZE=leak && (cd t && ./t5571-pre-push-hook.sh -v -i)

it shows that it was fixed by 861c56f6f9 (branch: fix a leak in
setup_tracking, 2023-06-11), which make sense. There are a bunch of leak
fixes in the same series, which makes me wonder if they're responsible
for most of these.

If the leaks are gone, I am happy that we are marking them. But it is
weird to me that we are getting different results.

-Peff

Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Jeff King]

On Thu, Aug 24, 2023 at 04:50:10PM -0400, Jeff King wrote:

> If I run a few by hand, I _do_ see leaks in them, but the exit codes are
> hidden from the test suite (they are sub-programs of scripts, etc). I
> guess you also have:
> 
>   GIT_TEST_SANITIZE_LEAK_LOG=true
> 
> set, which should find those (and which you mention in your first
> commit). Turning that on eliminates some of them, but I'm left with:
> 
>   t5614 t5317 t5503
> 
> not in your list. Which is super weird, because t5614 is marked with
> TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a
> _different_ set (t5614 again, along with your 4, but also t5303, t7701,
> and t4050). I wonder if we have a race in the leak-log code or
> something (I'm running under prove with -j32, naturally).

Argh. It is this again:

  https://lore.kernel.org/git/Yxl62zODF4oy1QL9@coredump.intra.peff.net/

Can we revisit that patch? Included again below for reference.

-- >8 --
Subject: [PATCH] test-lib: ignore uninteresting LSan output

When I run the tests in leak-checking mode the same way our CI job does,
like:

  make SANITIZE=leak \
       GIT_TEST_PASSING_SANITIZE_LEAK=true \
       GIT_TEST_SANITIZE_LEAK_LOG=true \
       test

then LSan can racily produce useless entries in the log files that look
like this:

  ==git==3034393==Unable to get registers from thread 3034307.

I think they're mostly harmless based on the source here:

  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414

which reads:

    PtraceRegistersStatus have_registers =
        suspended_threads.GetRegistersAndSP(i, &registers, &sp);
    if (have_registers != REGISTERS_AVAILABLE) {
      Report("Unable to get registers from thread %llu.\n", os_id);
      // If unable to get SP, consider the entire stack to be reachable unless
      // GetRegistersAndSP failed with ESRCH.
      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
        continue;
      sp = stack_begin;
    }

The program itself still runs fine and LSan doesn't cause us to abort.
But test-lib.sh looks for any non-empty LSan logs and marks the test as
a failure anyway, under the assumption that we simply missed the failing
exit code somehow.

I don't think I've ever seen this happen in the CI job, but running
locally using clang-14 on an 8-core machine, I can't seem to make it
through a full run of the test suite without having at least one
failure. And it's a different one every time (though they do seem to
often be related to packing tests, which makes sense, since that is one
of our biggest users of threaded code).

We can hack around this by only counting LSan log files that contain a
line that doesn't match our known-uninteresting pattern.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 293caf0f20..5ea5d1d62a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {
 	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+	xargs grep -lv "Unable to get registers from thread" |
 	wc -l
 }
 
-- 
2.42.0.448.g0caf9a9e14

Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Taylor Blau]

On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote:
> On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:
>
> > While working on another topic that cleared up some leaks, I wanted to
> > see if any new tests became leak-free, so I ran:
> >
> >     $ make SANITIZE=leak
> >     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test
>
> Is that exactly what you ran? Because I'd expect the second "make"
> invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.
> (Though I would have then expected most of the scripts to complain
> loudly about the mismatch; did you "cd t" in between the two?).

Argh. No, I wrote instead:

  make SANITIZE=leak
  make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test

> >  t/t3321-notes-stripspace.sh | 1 +
> >  t/t5571-pre-push-hook.sh    | 1 +
> >  t/t5583-push-branches.sh    | 1 +
> >  t/t7516-commit-races.sh     | 2 ++
> >  4 files changed, 5 insertions(+)
>
> If I run a single:
>
>   make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test
>
> on v2.42.0, I get many hits. All of the ones you mentioned, plus:
>
>   t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016
>
> If I run a few by hand, I _do_ see leaks in them, but the exit codes are
> hidden from the test suite (they are sub-programs of scripts, etc). I
> guess you also have:
>
>   GIT_TEST_SANITIZE_LEAK_LOG=true

Yep, that is in the patch message, and definitely necessary (as you
found ;-)) to get accurate results here.

Thanks,
Taylor

Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

[Jeff King]

On Fri, Aug 25, 2023 at 03:08:39PM -0400, Taylor Blau wrote:

> On Thu, Aug 24, 2023 at 04:50:09PM -0400, Jeff King wrote:
> > On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:
> >
> > > While working on another topic that cleared up some leaks, I wanted to
> > > see if any new tests became leak-free, so I ran:
> > >
> > >     $ make SANITIZE=leak
> > >     $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test
> >
> > Is that exactly what you ran? Because I'd expect the second "make"
> > invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.
> > (Though I would have then expected most of the scripts to complain
> > loudly about the mismatch; did you "cd t" in between the two?).
> 
> Argh. No, I wrote instead:
> 
>   make SANITIZE=leak
>   make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check ... test

Ah, that makes sense. The sample command in the commit message of the
first patch has the same situation, I think.

(I don't usually run "make test" from "t" myself because I prefer
"prove", and an explicit "make test" skips the DEFAULT_TEST_TARGET
magic).

-Peff

[PATCH v2 0/4] leak tests: mark remaining tests leak-free as such

[Taylor Blau]

Here is a reroll of my (I guess now mine and Peff's!) series to update
our test scripts to accurately mark which ones are leak-free.

This is mostly unchanged from the previous round, modulo cleaning up the
first (now second) patch's message, and inserting a new patch from Peff
at the beginning to ignore noisy LSan output.

Thanks in advance for your review!

Jeff King (1):
  test-lib: ignore uninteresting LSan output

Taylor Blau (3):
  leak tests: mark a handful of tests as leak-free
  leak tests: mark t3321-notes-stripspace.sh as leak-free
  leak tests: mark t5583-push-branches.sh as leak-free

 t/t3321-notes-stripspace.sh | 1 +
 t/t5571-pre-push-hook.sh    | 1 +
 t/t5583-push-branches.sh    | 1 +
 t/t7516-commit-races.sh     | 2 ++
 t/test-lib.sh               | 1 +
 5 files changed, 6 insertions(+)

Range-diff against v1:
-:  ---------- > 1:  7dd42212c0 test-lib: ignore uninteresting LSan output
1:  b1711c4c81 ! 2:  164f37cade leak tests: mark a handful of tests as leak-free
    @@ Commit message
         'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
         were marked as leak-free.
     
    -    As far as I can tell, each patch from that series ran tests from a
    -    handful of subject areas, such as "some ls-files tests", or "all trace2
    -    tests". This left some gaps in which tests had and hadn't been audited
    -    to be leak-free.
    +    Since then, a handful of tests have become leak-free due to changes like
     
    -    This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
    -    before sourcing t/test-lib.sh on most remaining leak-free tests. This
    -    list was compiled by doing:
    +      - 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), and
    +      - 866b43e644 (do_read_index(): always mark index as initialized unless
    +        erroring out, 2023-06-29)
    +
    +    , but weren't updated at the time to mark themselves as such. This leads
    +    to test "failures" when running:
     
             $ make SANITIZE=leak
    -        $ make \
    +        $ make -C t \
                 GIT_TEST_PASSING_SANITIZE_LEAK=check \
                 GIT_TEST_SANITIZE_LEAK_LOG=true \
                 GIT_TEST_OPTS=-vi test
     
    -    and looking through the list of failing tests in the output.
    +    This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
    +    before sourcing t/test-lib.sh on most remaining leak-free tests.
     
         There are a couple of other tests which are similarly leak-free, but not
         included in the list of tests touched by this patch. The remaining tests
         will be addressed in the subsequent two patches.
     
    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## t/t5571-pre-push-hook.sh ##
2:  cfeca88942 = 3:  116555fc02 leak tests: mark t3321-notes-stripspace.sh as leak-free
3:  4011eb6a8b = 4:  a16a0b2cac leak tests: mark t5583-push-branches.sh as leak-free
-- 
2.42.0.49.g03c54e21ee

[PATCH v2 1/4] test-lib: ignore uninteresting LSan output

[Taylor Blau]

From: Jeff King <peff@peff.net>

When I run the tests in leak-checking mode the same way our CI job does,
like:

  make SANITIZE=leak \
       GIT_TEST_PASSING_SANITIZE_LEAK=true \
       GIT_TEST_SANITIZE_LEAK_LOG=true \
       test

then LSan can racily produce useless entries in the log files that look
like this:

  ==git==3034393==Unable to get registers from thread 3034307.

I think they're mostly harmless based on the source here:

  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414

which reads:

    PtraceRegistersStatus have_registers =
        suspended_threads.GetRegistersAndSP(i, &registers, &sp);
    if (have_registers != REGISTERS_AVAILABLE) {
      Report("Unable to get registers from thread %llu.\n", os_id);
      // If unable to get SP, consider the entire stack to be reachable unless
      // GetRegistersAndSP failed with ESRCH.
      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
        continue;
      sp = stack_begin;
    }

The program itself still runs fine and LSan doesn't cause us to abort.
But test-lib.sh looks for any non-empty LSan logs and marks the test as
a failure anyway, under the assumption that we simply missed the failing
exit code somehow.

I don't think I've ever seen this happen in the CI job, but running
locally using clang-14 on an 8-core machine, I can't seem to make it
through a full run of the test suite without having at least one
failure. And it's a different one every time (though they do seem to
often be related to packing tests, which makes sense, since that is one
of our biggest users of threaded code).

We can hack around this by only counting LSan log files that contain a
line that doesn't match our known-uninteresting pattern.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 293caf0f20..5ea5d1d62a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -334,6 +334,7 @@ nr_san_dir_leaks_ () {
 	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+	xargs grep -lv "Unable to get registers from thread" |
 	wc -l
 }
 
-- 
2.42.0.49.g03c54e21ee

[PATCH v2 2/4] leak tests: mark a handful of tests as leak-free

[Taylor Blau]

In the topic merged via 5a4f8381b6 (Merge branch
'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
were marked as leak-free.

Since then, a handful of tests have become leak-free due to changes like

  - 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), and
  - 866b43e644 (do_read_index(): always mark index as initialized unless
    erroring out, 2023-06-29)

, but weren't updated at the time to mark themselves as such. This leads
to test "failures" when running:

    $ make SANITIZE=leak
    $ make -C t \
        GIT_TEST_PASSING_SANITIZE_LEAK=check \
        GIT_TEST_SANITIZE_LEAK_LOG=true \
        GIT_TEST_OPTS=-vi test

This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
before sourcing t/test-lib.sh on most remaining leak-free tests.

There are a couple of other tests which are similarly leak-free, but not
included in the list of tests touched by this patch. The remaining tests
will be addressed in the subsequent two patches.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5571-pre-push-hook.sh | 1 +
 t/t7516-commit-races.sh  | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index a11b20e378..448134c4bf 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -4,6 +4,7 @@ test_description='check pre-push hooks'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
index 2d38a16480..bb95f09810 100755
--- a/t/t7516-commit-races.sh
+++ b/t/t7516-commit-races.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git commit races'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'race to create orphan commit' '
-- 
2.42.0.49.g03c54e21ee

[PATCH v2 3/4] leak tests: mark t3321-notes-stripspace.sh as leak-free

[Taylor Blau]

This test was leak-free when t3321 was originally introduced, but never
marked as such:

    $ rev="$(git log --format='%H' --reverse -1 HEAD^ -- t/t3321-notes-stripspace.sh)"
    $ git checkout $rev

    $ make SANITIZE=leak
    [...]

    $ make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=--immediate t3321-notes-stripspace.sh
    [...]
    # passed all 27 test(s)
    1..27
    # faking up non-zero exit with --invert-exit-code
    make: *** [Makefile:66: t3321-notes-stripspace.sh] Error 1
    make: Leaving directory '/home/ttaylorr/src/git/t'

Mark this test as leak-free accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t3321-notes-stripspace.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh
index 028d825e8f..36abdca5ee 100755
--- a/t/t3321-notes-stripspace.sh
+++ b/t/t3321-notes-stripspace.sh
@@ -5,6 +5,7 @@
 
 test_description='Test commit notes with stripspace behavior'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 MULTI_LF="$LF$LF$LF"
-- 
2.42.0.49.g03c54e21ee

[PATCH v2 4/4] leak tests: mark t5583-push-branches.sh as leak-free

[Taylor Blau]

When t5583-push-branches.sh was originally introduced via 425b4d7f47
(push: introduce '--branches' option, 2023-05-06), it was not leak-free.
In fact, the test did not even run correctly until 022fbb655d (t5583:
fix shebang line, 2023-05-12), but after applying that patch, we see a
failure at t5583.8:

    ==2529087==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 384 byte(s) in 1 object(s) allocated from:
        #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
        #1 0x55e07606cbf9 in xrealloc wrapper.c:140
        #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42
        #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917
        #4 0x55e075fe9cce in add_missing_tags remote.c:1518
        #5 0x55e075fea1e4 in match_push_refs remote.c:1665
        #6 0x55e076050a8e in transport_push transport.c:1378
        #7 0x55e075e2eb74 in push_with_options builtin/push.c:401
        #8 0x55e075e2edb0 in do_push builtin/push.c:458
        #9 0x55e075e2ff7a in cmd_push builtin/push.c:702
        #10 0x55e075d8aaf0 in run_builtin git.c:452
        #11 0x55e075d8af08 in handle_builtin git.c:706
        #12 0x55e075d8b12c in run_argv git.c:770
        #13 0x55e075d8b6a0 in cmd_main git.c:905
        #14 0x55e075e81f07 in main common-main.c:60
        #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360
        #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)

    SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).

This leak was addressed independently via 68b51172e3 (commit-reach: fix
memory leak in get_reachable_subset(), 2023-06-03), which makes t5583
leak-free.

But t5583 was not in the tree when 68b51172e3 was written, and the two
only met after the latter was merged back in via 693bde461c (Merge
branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).

At that point, t5583 was leak-free. Let's mark it as such accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5583-push-branches.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5583-push-branches.sh b/t/t5583-push-branches.sh
index e7e1b6dab6..320f49c753 100755
--- a/t/t5583-push-branches.sh
+++ b/t/t5583-push-branches.sh
@@ -5,6 +5,7 @@ test_description='check the consisitency of behavior of --all and --branches'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 delete_refs() {
-- 
2.42.0.49.g03c54e21ee

Re: [PATCH v2 0/4] leak tests: mark remaining tests leak-free as such

[Jeff King]

On Mon, Aug 28, 2023 at 06:52:52PM -0400, Taylor Blau wrote:

> Here is a reroll of my (I guess now mine and Peff's!) series to update
> our test scripts to accurately mark which ones are leak-free.
> 
> This is mostly unchanged from the previous round, modulo cleaning up the
> first (now second) patch's message, and inserting a new patch from Peff
> at the beginning to ignore noisy LSan output.

Perhaps needless to say, but this all looks good to me. :)

-Peff

Re: [PATCH v2 0/4] leak tests: mark remaining tests leak-free as such

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 28, 2023 at 06:52:52PM -0400, Taylor Blau wrote:
>
>> Here is a reroll of my (I guess now mine and Peff's!) series to update
>> our test scripts to accurately mark which ones are leak-free.
>> 
>> This is mostly unchanged from the previous round, modulo cleaning up the
>> first (now second) patch's message, and inserting a new patch from Peff
>> at the beginning to ignore noisy LSan output.
>
> Perhaps needless to say, but this all looks good to me. :)

Thanks, both of you.  These look good to me, too ;-).

[PATCH] test-lib: ignore uninteresting LSan output

[Jeff King]

When I run the tests in leak-checking mode the same way our CI job does,
like:

  make SANITIZE=leak \
       GIT_TEST_PASSING_SANITIZE_LEAK=true \
       GIT_TEST_SANITIZE_LEAK_LOG=true \
       test

then LSan can racily produce useless entries in the log files that look
like this:

  ==git==3034393==Unable to get registers from thread 3034307.

I think they're mostly harmless based on the source here:

  https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414

which reads:

    PtraceRegistersStatus have_registers =
        suspended_threads.GetRegistersAndSP(i, &registers, &sp);
    if (have_registers != REGISTERS_AVAILABLE) {
      Report("Unable to get registers from thread %llu.\n", os_id);
      // If unable to get SP, consider the entire stack to be reachable unless
      // GetRegistersAndSP failed with ESRCH.
      if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
        continue;
      sp = stack_begin;
    }

The program itself still runs fine and LSan doesn't cause us to abort.
But test-lib.sh looks for any non-empty LSan logs and marks the test as
a failure anyway, under the assumption that we simply missed the failing
exit code somehow.

I don't think I've ever seen this happen in the CI job, but running
locally using clang-14 on an 8-core machine, I can't seem to make it
through a full run of the test suite without having at least one
failure. And it's a different one every time (though they do seem to
often be related to packing tests, which makes sense, since that is one
of our biggest users of threaded code).

We can hack around this by only counting LSan log files that contain a
line that doesn't match our known-uninteresting pattern.

Signed-off-by: Jeff King <peff@peff.net>
---
This feels awfully hacky, and maybe we should just throw it away unless
the CI job starts having problems. But I literally couldn't get through
a run without false positives, so at the very least I wanted to make
people aware of the problem and get that string into the list archive. ;)

 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 377cc1c120..3951214cd5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -324,6 +324,7 @@ nr_san_dir_leaks_ () {
 	find "$TEST_RESULTS_SAN_DIR" \
 		-type f \
 		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+	xargs grep -lv "Unable to get registers from thread" |
 	wc -l
 }
 
-- 
2.37.3.1139.g47294c03c7