UNLEAK(), leak checking in the default tests etc.

[In-Reply-To <a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>, but intentionally breaking threading for a new topic]

I ran into this when manually checking with valgrind and discovered that you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.

I wonder if that shouldn't be in DEVOPTS (or even a default under DEVELOPER=1). I.e. you don't need any other special compile flags, just a compiled git that you then run under valgrind to spot this.

    int repo_heap = 0;

    Then set "repo_heap = 1" in that absolute_pathdup(repo_name) branch,
    and...

    if (repo_heap)
        free(repo);

    GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind

I'm not familiar with git's development conventions/philosophy, but my 2c is that it's better not to enable it by default in order to minimise divergence from the code that users are running. OTOH it's not a major difference in behaviour so perhaps that's not a concern here.

More significantly: I get the impression it's easier to do leak checking using LSAN, which requires recompiling git anyway - at which point you get the flag for free - so how often will people actually perform leak checking with Valgrind in the first place?

Although this is possible, I don't think it's worth it: if UNLEAK already exists, we might as well use it here to make the code simpler. And UNLEAK is unlikely to go away anytime soon because... (continued below)

There are trickier examples where a cmd_* function has a complex struct on the stack, and correctly clearing all allocated memory pointed to by its members (or in turn further children with potentially multiple levels of indirection) is a lot of work - and that work doesn't actually benefit the user in any way. In other words, we either need to be able to use UNLEAK to suppress certain classes of uninteresting memory leaks - which allows us to focus on the interesting/real leaks - or someone has to spend a lot of time doing cleanup by hand (and/or someone has to implement a bunch of new cleanup functions)).

In your example above, the UNLEAK can be avoided at the cost of one additional tracking variable - but in many other cases avoiding an UNLEAK is much more expensive. It's certainly valid to debate the merits of the UNLEAK here, but that won't remove the need for UNLEAK's existence in general.

(The most common example that I remember is where cmd_* has a rev_info, and AFAICT there's no one-liner to clean that up. Using UNLEAK is honestly the best approach there. I don't think I've actually submitted any patches doing this, but I have a few in my local backlog.)

It's easy to perform leak-checking runs *if* you're OK recompiling with LSAN, instead of using valgrind. My usual recipe for running against a range of tests is something like:

make SANITIZE=address,leak
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" CFLAGS="-Og -g" T="\$(wildcard t00[0-9][0-9]-*.sh)" test

Additionally: I usually specify CC=clang, although gcc+LSAN has mostly been stable enough in my experience so you might be able to skip that.
(I've found ASAN+LSAN to be more stable than LSAN by itself, which is why I specify address+leak, but adding ASAN in turn requires overriding ASAN_OPTIONS to reenable leak checking.)

I don't know whether or not Valgrind is more/less effective at finding leaks, so being able to run the test suite under valgrind would be nice for comparison purposes though.

ATB,

Andrzej

It woudln't be on by default, you would need to turn it on with make DEVEOPER=1.

Felipe Contreras

Yeah, I'd rather not enable the option during normal builds. It carries a run-time penalty (it is actually building a pointless data structure that _does_ effectively leak the pointers, but backed by a global so they're "findable" by leak checkers). So it changes speed and possibly correctness of the final binary in a way that is different from what people would actually run in practice.

That might be worth it if there was some advantage to just turning it on (i.e., if by running with it all the time we might detect some bug). But by itself it does nothing useful.

If you really want to leak-check more thoroughly the normal binary, then IMHO you'd be better off to convert UNLEAK() sites to actual free calls.

And yeah, I'd very much agree here. It's definitely not wrong to run with Valgrind. But it's slower and much less thorough than ASan (probably not for leak detection, but definitely for bug-finding, since it can't look at stack variables).

If you do use it, and want to build with -DSUPPRESS_ANNOTATED_LEAKS all the time, that's OK, but I don't think it makes sense for it to the default even under DEVELOPER=1. I'm not opposed to a patch to make it easier to flip the switch, though (but I also find sticking a line in your config.mak to be pretty easy already).

-Peff

*Nod*, I didn't investigate the runtime penalty you and Jeff point out. In any case, it seems that can also be done with valgrind exclusion rules and/or manually ignoring these cases in the test wrapper.

The thing I was patching happened to be making rev_info * not leak. I probably didn't cover some more complex cases, but some simple cases seem relatively easy.

I.e. it just doesn't have a release() function, and at least the things I was looking at (bundle.c code) were relatively easy cases where we were just missing a loop to free() data from some struct.

But yes, I agree that free()-ing just before we exit() is rather useless in itself, the reason I wanted it is because it's a useful (although not perfect) proxy for checking if the APIs the command uses as a one-off leak when used as libraries, where we may be processing N items, later doing other work etc.

We should probably eventually have a s/free/end_free()/g and imitate perl(1)'s PERL_DESTRUCT_LEVEL option. I.e. you can globally configure perl to run in a mode that assumes a one-off command, in that case you'll just let the OS handle the cleanup, or one where you care about memory leaks because you're using it e.g. as an embedded library.

But maybe it's not even worth it. In Perl the main benefit is that it's a programming language with DESTROY handlers etc., so destruction can often be expensive; turning it off entirely can also be buggy, imagine relying on destructors to free temporary files etc.

We have that issue in theory with the interaction of atexit() handlers and e.g. things that would behave differently at a distance if certain thing were free()'d already, but in practice we probably don't.

But maybe it's not even worth pursuing. Have you (or anyone else) tried e.g. benchmarking git's tests or t/perf tests where free() is defined to be some noop stub? I'd expect it not to matter, but maybe I'm wrong...

I thought valgrind would be a better approach since we might rely on it just being there, so we could run some known-good commands that don't leak even in a "normal" test run, but...

I didn't know how to set that up, that seems easy enough.

This works for me:

make CC=clang SANITIZE=address,leak CFLAGS="-00 -g" (cd t && make ASAN_OPTIONS="<what you said>" [...])

I.e. it's just SANITIZE & flags that's important at compile-time. You doubtless knew that, mainly for my own notes & others following along.

I ran it, noted the failing tests, produced a giant GIT_SKIP_TESTS list and hacked ci/ to run that as a new linux-clang-SANITIZE job. That messy WIP code is currently running at:
https://github.com/avar/git/runs/2793150092

Wouldn't it be a good idea to have such a job and slowly work on the exclusion list?

E.g. I saw that t0004 failed, which was trivially fixed with a single strbuf_release(), and we could guard against regressions.

Anyway, I can submit some cleaned-up patches for that. I was just fishing for whether there was some good reason not to do it, since there seemed to have been interest in leak fixes, but it hadn't made it into CI / some "blessed" GIT_TEST_* mode or whatever. I.e. maybe the reports were unstable or unreliable...

I had trouble using valgrind's exclusions; there's more discussion in 0e5bba53af (add UNLEAK annotation for reducing leak false positives, 2017-09-08), but the gist of it is that it's awkward to annotate the point of leak, rather than the point of allocation (so you have to provide the complete callstack to the allocation, which is a maintenance headache).

Of course, if you find ways to make useful annotations with valgrind, I'm all for it. We have a few in t/valgrind already.

I haven't. Even though I originated UNLEAK(), I'm not really all that concerned about the cost of free() in general. My motivation for introducing it (versus adding free() calls) was mostly about convenience (complex data structures that don't have an easy free/release function, but also the fact that you can still access data after marking it with unleak).

The fact that it also preempts any arguments about the performance of calling free() was just a bonus. ;)

To be clear, I could easily be convinced by real numbers that the cost of free() at program end matters. I am just saying I am not one of the people who is going to argue that position in the meantime.

It should Just Work with:

make SANITIZE=leak test

for both gcc and clang. You do need ASAN_OPTIONS if you're asking ASan to do leak-checking (since we usually suppress that for the obvious reason that almost every test fails). I'm not sure if using both ASan and LSan together confuses LSan there (if so, it may be reasonable for test-lib.sh to modify its ASAN_OPTIONS setting if LSan is enabled).

I don't mind that. My intent was to get the whole suite clean eventually, and then start worrying about regressions. But that may take a while.

I do think it would be worth splitting out ASan from leak-checking. The whole suite should run clean with regular ASan already, and we'd want to find regressions there even in the tests that aren't leak-clean. I do periodic ASan runs already; the main argument against doing it for every CI run is just that's a lot more CPU. But maybe not enough to be prohibitive? It's probably still way cheaper than running the test suite on Windows.

-Peff

I've been running tests with ASAN in the Github Actions environment, and a single run takes just over 30 minutes [1] - which I believe is similar to the normal test jobs (they do run the test suite twice in that time I think).

I've been doing the same with UBSAN, and that's even faster at 15-20 minutes [2]. However I get the impression that ASAN issues are both more common (at least on seen), and more impactful - so I would argue that ASAN should be prioritised if there's spare capacity. (I have no idea if ASAN+UBSAN can be combined, but I suspect that doing so would make the tests slower?)

I'm also running LSAN tests in CI to try and catch regressions, but I've only enabled a handful of tests so far. My much simpler approach was to specify the range of tests to run as 0-X, and as we make progress on fixing leaks, X will slowly approach 9999 (currently we're at something like X~=5, although I'm not too far off sending out some patches to boost that to 99). The skip-tests approach seems much more useful!

ATB,

Andrzej

[1] https://github.com/ahunt/git/runs/2789921851?check_suite_focus=true
[2] https://github.com/ahunt/git/runs/2760632000?check_suite_focus=true

I routinely do SANITIZE=address,undefined since they are both useful (and we do not trigger either in the current test suite). I never measured the time of their combined use versus just one, but surely it's faster the two-at-once approach is faster than running the test suite twice.

Depending how fine-grained you get with skip-tests, it can create a hassle as individual tests are removed or reordered (and now somebody has to maintain the skip list). Doing it with whole scripts (whether saying "these ones are OK" or "these ones are known bad") seems like less maintenance overall. The results aren't as fine-grained, but for something that is meant to be a transitional step, I'm not sure it's worth the trouble to get more specific.

-Peff

I'm seeing 33 minutes for SANITIZE=address,undefined - which is no slower than SANITIZE=address by itself (disclaimer: it's only one measurement):
https://github.com/ahunt/git/runs/2795642716?check_suite_focus=true
(The job's name is wrong but if you look in the logs you can confirm that it's using address+undefined.)

The usual linux and mac test-jobs are actually from 24 to 30 minutes (the numbers seem a bit variable) - with the exception of one faster 10 minute job:
https://github.com/git/git/actions/runs/925771097
vs
https://github.com/git/git/actions/runs/927729395

So to summarise: adding an ASAN+UBSAN job would make things a bit slower, but not a huge amount slower.

'--valgrind' doesn't imply '--verbose' if '--verbose-log' was given, and that works with the test harness just fine; see 88c6e9d31c (test-lib: --valgrind should not override --verbose-log, 2017-09-05).

As a follow-up to my recent thread asking if we had some test mode or CI to test for memory leak regression (we don't), add such a test mode, and run it in CI.

Currently the two new CI targets take ~2-3 minutes to run in GitHub CI, whereas the normal test targets take 20-30 minutes. The tests run slower, but we have a small whitelist of test scripts that are OK.

1. https://lore.kernel.org/git/[email protected]/

Ævar Arnfjörð Bjarmason (4):
tests: add a test mode for SANITIZE=leak, run it in CI SANITIZE tests: fix memory leaks in t13*config*, add to whitelist SANITIZE tests: fix memory leaks in t5701*, add to whitelist SANITIZE tests: fix leak in mailmap.c

 .github/workflows/main.yml  |  6 ++++
 Makefile                    |  5 +++
 ci/install-dependencies.sh  |  4 +--
 ci/lib.sh                   | 18 ++++++++---
 ci/run-build-and-tests.sh   |  4 +--
 config.c                    | 17 ++++++++---
 mailmap.c                   |  2 ++
 protocol-caps.c             |  5 +--
 t/README                    | 16 ++++++++++
 t/t0500-progress-display.sh |  3 +-
 t/t1300-config.sh           | 16 ++++++----
 t/t4203-mailmap.sh          |  6 ++++
 t/t5701-git-serve.sh        |  3 +-
 t/test-lib.sh               | 61 +++++++++++++++++++++++++++++++++++++
 14 files changed, 142 insertions(+), 24 deletions(-)

2.32.0-dev

While git can be compiled with SANITIZE=leak there has been no corresponding GIT_TEST_* mode for it, i.e. memory leaks have been fixed as one-offs without structured regression testing.

This change add such a mode, we now have new linux-{clang,gcc}-sanitize-leak CI targets, these targets run the same tests as linux-{clang,gcc}, except that almost all of them are skipped.

There is a whitelist of some tests that are OK in test-lib.sh, and individual tests can be opted-in by setting GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those individual test can be skipped with the "!SANITIZE_LEAK" prerequisite. See the updated t/README for more details.

I'm using the GIT_TEST_SANITIZE_LEAK=true and !SANITIZE_LEAK pattern in a couple of tests whose memory leaks I'll fix in subsequent commits.

I'm not being aggressive about opting in tests, it's not all tests that currently pass under SANITIZE=leak, just a small number of known-good tests. We can add more later as we fix leaks and grow more confident in this test mode.

See the recent discussion at [1] about the lack of this sort of test mode, and 0e5bba53af (add UNLEAK annotation for reducing leak false positives, 2017-09-08) for the initial addition of SANITIZE=leak.

See also 09595ab381 (Merge branch 'jk/leak-checkers', 2017-09-19), 7782066f67 (Merge branch 'jk/apache-lsan', 2019-05-19) and the recent 936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the past history of "one-off" SANITIZE=leak (and more) fixes.

When calling maybe_skip_all_sanitize_leak matching against "$TEST_NAME" instead of "$this_test" as other "match_pattern_list()" users do is intentional. I'd like to match things like "t13*config*" in subsequent commits. This part of the API isn't public, so we can freely change it in the future.

1. https://lore.kernel.org/git/[email protected]/

---
 .github/workflows/main.yml  |  6 ++++
 Makefile                    |  5 ++++
 ci/install-dependencies.sh  |  4 +--
 ci/lib.sh                   | 18 +++++++----
 ci/run-build-and-tests.sh   |  4 +--
 t/README                    | 16 ++++++++++
 t/t0500-progress-display.sh |  3 +-
 t/t5701-git-serve.sh        |  2 +-
 t/test-lib.sh               | 60 +++++++++++++++++++++++++++++++++++++
 9 files changed, 107 insertions(+), 11 deletions(-)

index 73856bafc9..b81ec34959 100644
@@ -297,6 +297,12 @@ jobs:
           - jobname: linux-gcc-default
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-clang-sanitize-leak
+            cc: clang
+            pool: ubuntu-latest
+          - jobname: linux-gcc-sanitize-leak
+            cc: clang
+            pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}

index 502e0c9a81..d4cad5136f 100644
@@ -1216,6 +1216,9 @@ PTHREAD_CFLAGS =
 SPARSE_FLAGS ?=
 SP_EXTRA_FLAGS = -Wno-universal-initializer
 
+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
+SANITIZE_LEAK =
+
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.
 # Setting it to 0 will feed all files in a single spatch invocation.

@@ -1260,6 +1263,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 ifneq ($(filter leak,$(SANITIZERS)),)
 BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
+SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN

@@ -2793,6 +2797,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
 	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+

index 67852d0d37..31e519cde9 100755
@@ -12,13 +12,13 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 case "$jobname" in
-linux-clang|linux-gcc)
+linux-clang*|linux-gcc*)
 	sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test"
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
 		$UBUNTU_COMMON_PKGS
 	case "$jobname" in
-	linux-gcc)
+	linux-gcc*)
 		sudo apt-get -q -y install gcc-8
 		;;
 	esac

index 476c3f369f..34fd914438 100755
@@ -183,14 +183,16 @@ export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
 case "$jobname" in
-linux-clang|linux-gcc)
-	if [ "$jobname" = linux-gcc ]
-	then
+linux-clang*|linux-gcc*)
+	case "$jobname" in
+	linux-gcc*)
 		export CC=gcc-8
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
-	else
+		;;
+	*)
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
-	fi
+		;;
+	esac
 
 	export GIT_TEST_HTTPD=true
 

@@ -233,4 +235,10 @@ linux-musl)
 	;;
 esac
 
+case "$jobname" in
+linux-*-sanitize-leak)
+	export SANITIZE=leak
+	;;
+esac
+
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

index 3ce81ffee9..07b9c09f45 100755
@@ -12,7 +12,7 @@ esac
 
 make
 case "$jobname" in
-linux-gcc)
+linux-gcc*)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	make test
 	export GIT_TEST_SPLIT_INDEX=yes

@@ -29,7 +29,7 @@ linux-gcc)
 	export GIT_TEST_CHECKOUT_WORKERS=2
 	make test
 	;;
-linux-clang)
+linux-clang*)
 	export GIT_TEST_DEFAULT_HASH=sha1
 	make test
 	export GIT_TEST_DEFAULT_HASH=sha256

index 1a2072b2c8..303d0be817 100644
@@ -448,6 +448,22 @@ GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting
 to <n> and 'checkout.thresholdForParallelism' to 0, forcing the
 execution of the parallel-checkout code.
 
+GIT_TEST_SANITIZE_LEAK=<boolean> will force the tests to run when git
+is compiled with SANITIZE=leak (we pick it up via
+../GIT-BUILD-OPTIONS).
+
+By default all tests are skipped when compiled with SANITIZE=leak, and
+individual test scripts opt themselves in to leak testing by setting
+GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those
+tests use the SANITIZE_LEAK prerequisite to skip individiual tests
+(i.e. test_expect_success !SANITIZE_LEAK [...]).
+
+So the GIT_TEST_SANITIZE_LEAK setting is different in behavior from
+both other GIT_TEST_*=[true|false] settings, but more useful given how
+SANITIZE=leak works & the state of the test suite. Manually setting
+GIT_TEST_SANITIZE_LEAK=true is only useful during development when
+finding and fixing memory leaks.
+
 Naming Tests
 ------------
 

index 22058b503a..7afb9abb1f 100755
@@ -2,6 +2,7 @@
 
 test_description='progress display'
 
+GIT_TEST_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 show_cr () {

@@ -283,7 +284,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
 	test_cmp expect out
 '
 
-test_expect_success 'progress generates traces' '
+test_expect_success !SANITIZE_LEAK 'progress generates traces' '
 	cat >in <<-\EOF &&
 	throughput 102400 1000
 	update

index 930721f053..d58efb0aa9 100755
@@ -243,7 +243,7 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '
 
 # Test the basics of object-info
 #
-test_expect_success 'basics of object-info' '
+test_expect_success !SANITIZE_LEAK 'basics of object-info' '
 	test-tool pkt-line pack >in <<-EOF &&
 	command=object-info
 	object-format=$(test_oid algo)

2.32.0-dev

Fix a couple of trivial memory leaks introduced in 3efd0bedc6 (config:
add conditional include, 2017-03-01) and my own 867ad08a26 (hooks:
allow customizing where the hook directory is, 2016-05-04).

In the latter case the "fix" is UNLEAK() on the global variable. This allows us to run all t13*config* tests under SANITIZE=leak.

With this change we can now run almost the whole set of config.c tests (t13*config) under SANITIZE=leak, so let's do so, with a few exceptions:

* The test added in ce81b1da23 (config: add new way to pass config via --config-env, 2021-01-12), it fails in GitHub CI, but passes for me locally. Let's just skip it for now.

* Ditto the split_cmdline and "aliases of builtins" tests, the former required splitting up an existing test, there an issue with the test that would have also been revealed by skipping it.

---
 config.c          | 17 ++++++++++++-----
 t/t1300-config.sh | 16 ++++++++++------
 t/test-lib.sh     |  1 +
 3 files changed, 23 insertions(+), 11 deletions(-)

index f9c400ad30..38e132c0e2 100644
@@ -138,8 +138,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		return config_error_nonbool("include.path");
 
 	expanded = expand_user_path(path, 0);
-	if (!expanded)
-		return error(_("could not expand include path '%s'"), path);
+	if (!expanded) {
+		ret = error(_("could not expand include path '%s'"), path);
+		goto cleanup;
+	}
 	path = expanded;
 
 	/*

@@ -149,8 +151,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	if (!is_absolute_path(path)) {
 		char *slash;
 
-		if (!cf || !cf->path)
-			return error(_("relative config includes must come from files"));
+		if (!cf || !cf->path) {
+			ret = error(_("relative config includes must come from files"));
+			goto cleanup;
+		}
 
 		slash = find_last_dir_sep(cf->path);
 		if (slash)

@@ -168,6 +172,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
+cleanup:
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;

@@ -1331,8 +1336,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "core.attributesfile"))
 		return git_config_pathname(&git_attributes_file, var, value);
 
-	if (!strcmp(var, "core.hookspath"))
+	if (!strcmp(var, "core.hookspath")) {
+		UNLEAK(git_hooks_path);
 		return git_config_pathname(&git_hooks_path, var, value);
+	}
 
 	if (!strcmp(var, "core.bare")) {
 		is_bare_repository_cfg = git_config_bool(var, value);

index 9ff46f3b04..93ad0f4887 100755
@@ -1050,12 +1050,16 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	test_must_fail git config --file=linktolinktonada --list
 '
 
-test_expect_success 'check split_cmdline return' "
-	git config alias.split-cmdline-fix 'echo \"' &&
-	test_must_fail git split-cmdline-fix &&
+test_expect_success 'setup check split_cmdline return' "
 	echo foo > foo &&
 	git add foo &&
-	git commit -m 'initial commit' &&
+	git commit -m 'initial commit'
+"
+
+test_expect_success !SANITIZE_LEAK 'check split_cmdline return' "
+	git config alias.split-cmdline-fix 'echo \"' &&
+	test_must_fail git split-cmdline-fix &&
+
 	git config branch.main.mergeoptions 'echo \"' &&
 	test_must_fail git merge main
 "

@@ -1101,7 +1105,7 @@ test_expect_success 'key sanity-checking' '
 	git config foo."ba =z".bar false
 '
 
-test_expect_success 'git -c works with aliases of builtins' '
+test_expect_success !SANITIZE_LEAK 'git -c works with aliases of builtins' '
 	git config alias.checkconfig "-c foo.check=bar config foo.check" &&
 	echo bar >expect &&
 	git checkconfig >actual &&

@@ -1397,7 +1401,7 @@ test_expect_success 'git --config-env with missing value' '
 	grep "invalid config format: config" error
 '
 
-test_expect_success 'git --config-env fails with invalid parameters' '
+test_expect_success !SANITIZE_LEAK 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&
 	test_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error &&

index 9201510e16..98e20950c3 100644
@@ -1370,6 +1370,7 @@ maybe_skip_all_sanitize_leak () {
 	add_sanitize_leak_true 't000*'
 	add_sanitize_leak_true 't001*'
 	add_sanitize_leak_true 't006*'
+	add_sanitize_leak_true 't13*config*'
 
 	# Blacklist patterns (overrides whitelist)
 	add_sanitize_leak_false 't000[469]*'

2.32.0-dev

Fix a memory leak in a2ba162cda (object-info: support for retrieving object info, 2021-04-20) which appears to have been based on a misunderstanding of how the pkt-line.c API works, there is no need to strdup() input to, it's just a printf()-like format function.

This fixes a potentially large memory leak, since the number of OID lines the "object-info" call can be arbitrarily large (or a small one if the request is small).

---
 protocol-caps.c      | 5 +++--
 t/t5701-git-serve.sh | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

index 13a9e63a04..901b6795e4 100644
@@ -69,9 +69,10 @@ static void send_info(struct repository *r, struct packet_writer *writer,
 			}
 		}
 
-		packet_writer_write(writer, "%s",
-				    strbuf_detach(&send_buffer, NULL));
+		packet_writer_write(writer, "%s", send_buffer.buf);
+		strbuf_reset(&send_buffer);
 	}
+	strbuf_release(&send_buffer);
 }
 
 int cap_object_info(struct repository *r, struct strvec *keys,

index d58efb0aa9..e2f4832adf 100755
@@ -5,6 +5,7 @@ test_description='test protocol v2 server commands'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+GIT_TEST_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'test capability advertisement' '

2.32.0-dev

Get closer to being able to run t4203-mailmap.sh by fixing a couple of memory leak in mailmap.c.

In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to clear the "me" structure, but while we freed parts of the mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

---
 mailmap.c          | 2 ++
 t/t4203-mailmap.sh | 6 ++++++
 2 files changed, 8 insertions(+)

index d1f7c0d272..e1c8736093 100644
@@ -36,6 +36,7 @@ static void free_mailmap_info(void *p, const char *s)
 		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
+	free(mi);
 }
 
 static void free_mailmap_entry(void *p, const char *s)

@@ -51,6 +52,7 @@ static void free_mailmap_entry(void *p, const char *s)
 
 	me->namemap.strdup_strings = 1;
 	string_list_clear_func(&me->namemap, free_mailmap_info);
+	free(me);
 }
 
 /*

index 0b2d21ec55..c7de4299cf 100755
@@ -79,6 +79,12 @@ test_expect_success 'check-mailmap bogus contact --stdin' '
 	test_must_fail git check-mailmap --stdin bogus </dev/null
 '
 
+if test_have_prereq SANITIZE_LEAK
+then
+	skip_all='skipping the rest of mailmap tests under SANITIZE_LEAK'
+	test_done
+fi
+
 test_expect_success 'No mailmap' '
 	cat >expect <<-EOF &&
 	$GIT_AUTHOR_NAME (1):

2.32.0-dev

Missing sign-off.

As a follow-up to my recent thread asking if we had some test mode or CI to test for memory leak regression (we don't), add such a test mode, and run it in CI.

Currently the two new CI targets take ~2-3 minutes to run in GitHub CI, whereas the normal test targets take 20-30 minutes. The tests run slower, but we have a small whitelist of test scripts that are OK.

v2:

* Fixes issues spotted by Đoàn Trần Công Danh and Eric Sunshine, thanks both!

* I got rid of the change to t0500, I saw it being flaky in GitHub CI, and looks like there'll be other concurrent edits to that file, so leaving it be.

v1: http://lore.kernel.org/git/[email protected]

Ævar Arnfjörð Bjarmason (4):
tests: add a test mode for SANITIZE=leak, run it in CI SANITIZE tests: fix memory leaks in t13*config*, add to whitelist SANITIZE tests: fix memory leaks in t5701*, add to whitelist SANITIZE tests: fix leak in mailmap.c

 .github/workflows/main.yml |  6 ++++
 Makefile                   |  5 ++++
 ci/install-dependencies.sh |  4 +--
 ci/lib.sh                  | 18 +++++++----
 ci/run-build-and-tests.sh  |  4 +--
 config.c                   | 17 +++++++----
 mailmap.c                  |  2 ++
 protocol-caps.c            |  5 ++--
 t/README                   | 16 ++++++++++
 t/t1300-config.sh          | 16 ++++++----
 t/t4203-mailmap.sh         |  6 ++++
 t/t5701-git-serve.sh       |  3 +-
 t/test-lib.sh              | 61 ++++++++++++++++++++++++++++++++++++++
 13 files changed, 140 insertions(+), 23 deletions(-)

Range-diff against v1:
1: b7948c408d ! 1: 0795436a24 tests: add a test mode for SANITIZE=leak, run it in CI
@@ .github/workflows/main.yml: jobs:
+ cc: clang
+ pool: ubuntu-latest
+ - jobname: linux-gcc-sanitize-leak
-+ cc: clang
++ cc: gcc
+ pool: ubuntu-latest
env:
CC: ${{matrix.vector.cc}}
@@ ci/install-dependencies.sh: UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl

case "$jobname" in
-linux-clang|linux-gcc)
-+linux-clang*|linux-gcc*) ++linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test" sudo apt-get -q update
sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
$UBUNTU_COMMON_PKGS
case "$jobname" in
- linux-gcc)
-+ linux-gcc*)
++ linux-gcc|linux-gcc-sanitize-leak)
sudo apt-get -q -y install gcc-8
;;
esac
@@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true
-linux-clang|linux-gcc)
- if [ "$jobname" = linux-gcc ]
- then
-+linux-clang*|linux-gcc*) ++linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
+ case "$jobname" in
-+ linux-gcc*)
++ linux-gcc|linux-gcc-sanitize-leak)
export CC=gcc-8
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
- else
@@ ci/lib.sh: linux-musl)
esac

+case "$jobname" in
-+linux-*-sanitize-leak) ++linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
+ export SANITIZE=leak
+ ;;
+esac
@@ ci/run-build-and-tests.sh: esac
make
case "$jobname" in
-linux-gcc)
-+linux-gcc*)
++linux-gcc|linux-gcc-sanitize-leak)
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
make test
export GIT_TEST_SPLIT_INDEX=yes
@@ ci/run-build-and-tests.sh: linux-gcc)
make test
;;
-linux-clang)
-+linux-clang*)
++linux-clang|linux-clang-sanitize-leak)
export GIT_TEST_DEFAULT_HASH=sha1
make test
export GIT_TEST_DEFAULT_HASH=sha256
@@ t/README: GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting
------------

- ## t/t0500-progress-display.sh ##
-@@
-
- test_description='progress display'
-
-+GIT_TEST_SANITIZE_LEAK=true
- . ./test-lib.sh
-
- show_cr () {
-@@ t/t0500-progress-display.sh: test_expect_success 'cover up after throughput shortens a lot' '
- test_cmp expect out
- '
-
--test_expect_success 'progress generates traces' '
-+test_expect_success !SANITIZE_LEAK 'progress generates traces' '
- cat >in <<-\EOF &&
- throughput 102400 1000
- update
-
## t/t5701-git-serve.sh ##
@@ t/t5701-git-serve.sh: test_expect_success 'unexpected lines are not allowed in fetch request' '

2:  babcb1c289 = 2:  867e8e9a6c SANITIZE tests: fix memory leaks in t13*config*, add to whitelist
3:  11aa2f3bb5 = 3:  b7fb5d5a56 SANITIZE tests: fix memory leaks in t5701*, add to whitelist
4:  7f4e433559 ! 4:  ad8680f529 SANITIZE tests: fix leak in mailmap.c

@@ Commit message
mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

+ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
+
## mailmap.c ##
@@ mailmap.c: static void free_mailmap_info(void *p, const char *s)
s, debug_str(mi->name), debug_str(mi->email));

2.32.0.853.g5a570c9bf9

While git can be compiled with SANITIZE=leak there has been no corresponding GIT_TEST_* mode for it, i.e. memory leaks have been fixed as one-offs without structured regression testing.

This change add such a mode, we now have new linux-{clang,gcc}-sanitize-leak CI targets, these targets run the same tests as linux-{clang,gcc}, except that almost all of them are skipped.

There is a whitelist of some tests that are OK in test-lib.sh, and individual tests can be opted-in by setting GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those individual test can be skipped with the "!SANITIZE_LEAK" prerequisite. See the updated t/README for more details.

I'm using the GIT_TEST_SANITIZE_LEAK=true and !SANITIZE_LEAK pattern in a couple of tests whose memory leaks I'll fix in subsequent commits.

I'm not being aggressive about opting in tests, it's not all tests that currently pass under SANITIZE=leak, just a small number of known-good tests. We can add more later as we fix leaks and grow more confident in this test mode.

See the recent discussion at [1] about the lack of this sort of test mode, and 0e5bba53af (add UNLEAK annotation for reducing leak false positives, 2017-09-08) for the initial addition of SANITIZE=leak.

See also 09595ab381 (Merge branch 'jk/leak-checkers', 2017-09-19), 7782066f67 (Merge branch 'jk/apache-lsan', 2019-05-19) and the recent 936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the past history of "one-off" SANITIZE=leak (and more) fixes.

When calling maybe_skip_all_sanitize_leak matching against "$TEST_NAME" instead of "$this_test" as other "match_pattern_list()" users do is intentional. I'd like to match things like "t13*config*" in subsequent commits. This part of the API isn't public, so we can freely change it in the future.

1. https://lore.kernel.org/git/[email protected]/

---
 .github/workflows/main.yml |  6 ++++
 Makefile                   |  5 ++++
 ci/install-dependencies.sh |  4 +--
 ci/lib.sh                  | 18 ++++++++----
 ci/run-build-and-tests.sh  |  4 +--
 t/README                   | 16 ++++++++++
 t/t5701-git-serve.sh       |  2 +-
 t/test-lib.sh              | 60 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 105 insertions(+), 10 deletions(-)

index 73856bafc9..752fe187f9 100644
@@ -297,6 +297,12 @@ jobs:
           - jobname: linux-gcc-default
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-clang-sanitize-leak
+            cc: clang
+            pool: ubuntu-latest
+          - jobname: linux-gcc-sanitize-leak
+            cc: gcc
+            pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}

index 502e0c9a81..d4cad5136f 100644
@@ -1216,6 +1216,9 @@ PTHREAD_CFLAGS =
 SPARSE_FLAGS ?=
 SP_EXTRA_FLAGS = -Wno-universal-initializer
 
+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
+SANITIZE_LEAK =
+
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.
 # Setting it to 0 will feed all files in a single spatch invocation.

@@ -1260,6 +1263,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 ifneq ($(filter leak,$(SANITIZERS)),)
 BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
+SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN

@@ -2793,6 +2797,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
 	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+

index 67852d0d37..8ac72d7246 100755
@@ -12,13 +12,13 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 case "$jobname" in
-linux-clang|linux-gcc)
+linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
 	sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test"
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
 		$UBUNTU_COMMON_PKGS
 	case "$jobname" in
-	linux-gcc)
+	linux-gcc|linux-gcc-sanitize-leak)
 		sudo apt-get -q -y install gcc-8
 		;;
 	esac

index 476c3f369f..bb02b5abf4 100755
@@ -183,14 +183,16 @@ export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
 case "$jobname" in
-linux-clang|linux-gcc)
-	if [ "$jobname" = linux-gcc ]
-	then
+linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
+	case "$jobname" in
+	linux-gcc|linux-gcc-sanitize-leak)
 		export CC=gcc-8
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
-	else
+		;;
+	*)
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
-	fi
+		;;
+	esac
 
 	export GIT_TEST_HTTPD=true
 

@@ -233,4 +235,10 @@ linux-musl)
 	;;
 esac
 
+case "$jobname" in
+linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
+	export SANITIZE=leak
+	;;
+esac
+
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

index 3ce81ffee9..5fe047b5c6 100755
@@ -12,7 +12,7 @@ esac
 
 make
 case "$jobname" in
-linux-gcc)
+linux-gcc|linux-gcc-sanitize-leak)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	make test
 	export GIT_TEST_SPLIT_INDEX=yes

@@ -29,7 +29,7 @@ linux-gcc)
 	export GIT_TEST_CHECKOUT_WORKERS=2
 	make test
 	;;
-linux-clang)
+linux-clang|linux-clang-sanitize-leak)
 	export GIT_TEST_DEFAULT_HASH=sha1
 	make test
 	export GIT_TEST_DEFAULT_HASH=sha256

index 1a2072b2c8..303d0be817 100644
@@ -448,6 +448,22 @@ GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting
 to <n> and 'checkout.thresholdForParallelism' to 0, forcing the
 execution of the parallel-checkout code.
 
+GIT_TEST_SANITIZE_LEAK=<boolean> will force the tests to run when git
+is compiled with SANITIZE=leak (we pick it up via
+../GIT-BUILD-OPTIONS).
+
+By default all tests are skipped when compiled with SANITIZE=leak, and
+individual test scripts opt themselves in to leak testing by setting
+GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those
+tests use the SANITIZE_LEAK prerequisite to skip individiual tests
+(i.e. test_expect_success !SANITIZE_LEAK [...]).
+
+So the GIT_TEST_SANITIZE_LEAK setting is different in behavior from
+both other GIT_TEST_*=[true|false] settings, but more useful given how
+SANITIZE=leak works & the state of the test suite. Manually setting
+GIT_TEST_SANITIZE_LEAK=true is only useful during development when
+finding and fixing memory leaks.
+
 Naming Tests
 ------------
 

index 930721f053..d58efb0aa9 100755
@@ -243,7 +243,7 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '
 
 # Test the basics of object-info
 #
-test_expect_success 'basics of object-info' '
+test_expect_success !SANITIZE_LEAK 'basics of object-info' '
 	test-tool pkt-line pack >in <<-EOF &&
 	command=object-info
 	object-format=$(test_oid algo)

2.32.0.853.g5a570c9bf9

I didn't know that, makes sense. I'll make it one job and have it use whatever CC is.

It sounds good to support that, but at least right now I've got the itch of finding leaks during development, and I think in any case being able to do a full run with just sanitizing, leak checking (or combined) makes sense, i.e. to make GIT_TEST_SANITIZE_LEAK=* the top-level interface.

I haven't checked how noisy ASAN is, is it like the leak checking where we fail almost all tests now?

Anyway, once we have some test mode like this it'll be trivial to extend it. I mainly want us to get this into CI so we can have an expanding line in the sand with regressions.

We have one-off commands, but also long-lived things like "git cat-file --batch", it's useful if we don't leak in those.

The entry point to those tends to be one-off commands in tests, so checking leaks for all commands in a test (if you can get there) is a useful indicator for how the underlying API performs.

I think in the git.git codebase we don't have much of an issue with buffer overflows etc, because we tend to consistently use APIs like strbuf that avoid those issues, but then again I haven't run the tests with that, maybe I'll be unpleasantly surprised.

I also find leak checking to be useful during development to spot faulty assumptions, i.e. the leak itself may not be a big deal, but it's usually an early sign that I'm structuring something incorrectly.

I did that in v1, as Đoàn Trần Công Danh pointed out we have other jobs that would match that.

I'm not too familiar with git.git's ci/* dir, but I think it's like that in general because we don't just run in GitHub CI, but want to support Azure, Travis etc.

So almost all of the logic is in those shellscripts, right now this target just runs in the GitHub CI, but it would be useful to make it drop-in enabled elsewhere.

I think given that that doing anything overly clever in the YAML would probably be counter-productive.

I do think it's worth having an ASan+UBSan job. In the CI we use for our custom fork of Git at GitHub, we run it for every pull request (and I do bring upstream any applicable fixes). It's kind of expensive compared to a regular "make test", but probably not nearly as bad as just running the regular test suite on Windows.

And it's true that ASan can do leak-checking, too. In the long run, when we are leak-free, I think it may make sense to combine the jobs. But in the interim state where we can run the whole suite with ASan/UBSan, but not with LSan, I think it's simpler to just keep them separate. That lets us just entirely skip tests or scripts in the leak-checking run. I haven't measured, but I also expect that LSan is not much more expensive than a regular run, so combining the two isn't that big a win).

So I do like your suggestion, but I think it just be orthogonal further to leak-checking.

-Peff

This opening puzzled me. I'm not sure I understand why we need a special GIT_TEST_* mode for it. If you do "make SANITIZE=leak test", then your binaries will leak-check while running the tests.

I.e., there is nothing that test-lib.sh itself needs to do differently to enable it.

What we _do_ need is some mechanism of annotating to tests to say "this is known to leak", so that we can skip them for normal integration runs.

And that is part of what's going on in this patch, but I'm not sure it is the simplest way to do it. The first question is: how do we want to annotate the tests. By marking individual scripts or tests in the test-files themselves? Or by using a separate list of "these scripts or tests are known to pass"?

IMHO the latter is preferable. It keeps the annotations out of the way of normal work (they are a temporary thing until we eventually pass the whole suite leak free, but I expect they'll be with us for a while). The downside is that the annotations may get out of sync with test numbers. But if we are primarily annotating whole scripts (and not individual tests), then that is generally pretty stable.

And with that in mind, can we just use an existing mechanism for picking which tests to run, and drive it externally from the CI job?

We already have GIT_SKIP_TESTS and --run. Those are perhaps a bit awkward for feeding huge lists to, and there is no environment equivalent for --run (so you can't trigger it easily from "make test"). But what if we could do something like:

GIT_TEST_RUN_FROM=t/leak-free make SANITIZE=leak test

and then t/leak-free contained the usual patterns like:

t000*
t1234.5

and so on. That requires two new features in test-lib.sh:

- making a GIT_TEST_RUN variable that is the opposite of GIT_TEST_SKIP (instead of just the command-line --run).

- adding GIT_TEST_{RUN,SKIP}_FROM variables to read the values from a file rather than the environment (I suppose the caller could just stuff the contents into the variable, but I expect that test-lib.sh may want to pare down the entries that do not even apply to the current script for the sake of efficiency in checking each test).

That infrastructure would then be applicable to other cases, too. Or even just useful for using another list (or no list at all) when you are looking at whether other tests are leak-free or not.

I'm not clear on what we expect to get out of running it with both clang and gcc. They should be producing identical results.

-Peff

I've included a mechanism for whitelisting specific globs, the idea was not to have that be too detailed, but we'd e.g. get to the point of t00* or whatever passing.

Anything that's a lot more granular than that is doing to suck, e.g. exposing teh GIT_TEST_SKIP and --run features. of specific test numbers, now you need to count your tests if you add one in the middle of one of those, and more likely you won't test under the mode and just see it in CI.

The marking at a distance I've done also has that problem in theory, but I think in practice we'll use it carefully for globs of tests unlikely to break.

This whole thing is much more with the GIT_TEST_SANITIZE_LEAK mode, it's a really common case that we e.g. leak in some revision.c API user, we should fix that, but holding up marking the rest of at test whose entire tests otherwise pass is bad, it means you can't do any testing of a given API or subsystem without getting the entire file to pass.

Whereas while we're fixing very common leaks in the codabase it's likely that any given test file will have a few such tests.

It also means everything works by default, you get an appropriate notice from prove(1), and even if you run one test manually it'll skip, but emit a message saying you can set the env var to force its run.

Indeed, addressed elsewhere, i.e. it's just a thinko of mine.

I think you can do the same level of skipping with GIT_TEST_SKIP, though. My argument was just that adding a new mechanism does not make sense when we already have one. I.e., running:

GIT_SKIP_TESTS='

    t[123456789]*
    t0[^0]*
    t00[^016]*
    t000[469]
    t001[2459]
    t006[0248]

' make SANITIZE=leak test

works already to do the same thing. The only thing we might want is a nicer syntax (e.g., to allow positive and negative patterns, or to read from a file). But that would benefit all users of GIT_SKIP_TESTS, not just people interested in leaks.

With GIT_SKIP_TESTS you obviously don't get a message saying "try skipping this test" when it fails. :) But IMHO that is not that big a deal. You'll get a test failure with good LSan output. If you are working on expanding leak-checker coverage, you already know about your options for skipping. If you're adding a new test that leaks, you might consider fixing the leak (though not always, if it's far from code you're touching).

-Peff

I cheated a little here; an unrelated bug does cause a failure in t0000 with this pattern. I've just sent:

https://lore.kernel.org/git/[email protected]/

to fix it.

-Peff

A glob in this series is t13*config*, you can't do that with GIT_SKIP_TESTS because it only includes the numeric part of the test, i.e. t1300, not t1300-config, or t1306-xdg-files.

But sure, it could happen via some other mechanism than the exact one I picked, or we could add GIT_SKIP_TESTS2 or whatever.

I would like to be able to compile with it and run "make test" without a wall of failures by default, i.e. we should be able to tell regressions from known-OK to get anywhere with it, but that's orthagonal to the exact mechanism.

I do think it makes sense as a test mode test-lib.sh is aware of, e.g. on obvious next step is to not fail everything right away, but just let the test run and log all failures to a file, then e.g. fail one test at the end, or if we're running in that mode collate all the callstacks and emit a summary for the whole test run.

But yes, the message it emits now isn't such a big deal.

That seems like a feature that GIT_SKIP_TESTS could learn (though IMHO just using the test number in your patterns is sufficient).

Right, I definitely agree on the goal. I just don't see the need to add a new, very-specific mechanism. The skip-list above is gross and obviously not something you'd want to type. Driving it from a ci script is not too bad, but I agree people who want to leak-check locally would want an easy way to use it, too. That's why I suggested extending it to a file that could be easily specified (and possibly even auto-triggered in the Makefile by SANITIZE=leak).

That's a more compelling reason, if we did implement that feature. My hope was that all of this would be a temporary state, though, and we'd get to a point where you can simply run "make SANITIZE=leak test" and actually run all of the tests. And then such a feature would not be that interesting, because failures would be rare and cause for immediate human attention.

-Peff

Fix a couple of trivial memory leaks introduced in 3efd0bedc6 (config:
add conditional include, 2017-03-01) and my own 867ad08a26 (hooks:
allow customizing where the hook directory is, 2016-05-04).

In the latter case the "fix" is UNLEAK() on the global variable. This allows us to run all t13*config* tests under SANITIZE=leak.

With this change we can now run almost the whole set of config.c tests (t13*config) under SANITIZE=leak, so let's do so, with a few exceptions:

* The test added in ce81b1da23 (config: add new way to pass config via --config-env, 2021-01-12), it fails in GitHub CI, but passes for me locally. Let's just skip it for now.

* Ditto the split_cmdline and "aliases of builtins" tests, the former required splitting up an existing test, there an issue with the test that would have also been revealed by skipping it.

---
 config.c          | 17 ++++++++++++-----
 t/t1300-config.sh | 16 ++++++++++------
 t/test-lib.sh     |  1 +
 3 files changed, 23 insertions(+), 11 deletions(-)

index f9c400ad30..38e132c0e2 100644
@@ -138,8 +138,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		return config_error_nonbool("include.path");
 
 	expanded = expand_user_path(path, 0);
-	if (!expanded)
-		return error(_("could not expand include path '%s'"), path);
+	if (!expanded) {
+		ret = error(_("could not expand include path '%s'"), path);
+		goto cleanup;
+	}
 	path = expanded;
 
 	/*

@@ -149,8 +151,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	if (!is_absolute_path(path)) {
 		char *slash;
 
-		if (!cf || !cf->path)
-			return error(_("relative config includes must come from files"));
+		if (!cf || !cf->path) {
+			ret = error(_("relative config includes must come from files"));
+			goto cleanup;
+		}
 
 		slash = find_last_dir_sep(cf->path);
 		if (slash)

@@ -168,6 +172,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
+cleanup:
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;

@@ -1331,8 +1336,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "core.attributesfile"))
 		return git_config_pathname(&git_attributes_file, var, value);
 
-	if (!strcmp(var, "core.hookspath"))
+	if (!strcmp(var, "core.hookspath")) {
+		UNLEAK(git_hooks_path);
 		return git_config_pathname(&git_hooks_path, var, value);
+	}
 
 	if (!strcmp(var, "core.bare")) {
 		is_bare_repository_cfg = git_config_bool(var, value);

index 9ff46f3b04..93ad0f4887 100755
@@ -1050,12 +1050,16 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	test_must_fail git config --file=linktolinktonada --list
 '
 
-test_expect_success 'check split_cmdline return' "
-	git config alias.split-cmdline-fix 'echo \"' &&
-	test_must_fail git split-cmdline-fix &&
+test_expect_success 'setup check split_cmdline return' "
 	echo foo > foo &&
 	git add foo &&
-	git commit -m 'initial commit' &&
+	git commit -m 'initial commit'
+"
+
+test_expect_success !SANITIZE_LEAK 'check split_cmdline return' "
+	git config alias.split-cmdline-fix 'echo \"' &&
+	test_must_fail git split-cmdline-fix &&
+
 	git config branch.main.mergeoptions 'echo \"' &&
 	test_must_fail git merge main
 "

@@ -1101,7 +1105,7 @@ test_expect_success 'key sanity-checking' '
 	git config foo."ba =z".bar false
 '
 
-test_expect_success 'git -c works with aliases of builtins' '
+test_expect_success !SANITIZE_LEAK 'git -c works with aliases of builtins' '
 	git config alias.checkconfig "-c foo.check=bar config foo.check" &&
 	echo bar >expect &&
 	git checkconfig >actual &&

@@ -1397,7 +1401,7 @@ test_expect_success 'git --config-env with missing value' '
 	grep "invalid config format: config" error
 '
 
-test_expect_success 'git --config-env fails with invalid parameters' '
+test_expect_success !SANITIZE_LEAK 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&
 	test_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error &&

index 9201510e16..98e20950c3 100644
@@ -1370,6 +1370,7 @@ maybe_skip_all_sanitize_leak () {
 	add_sanitize_leak_true 't000*'
 	add_sanitize_leak_true 't001*'
 	add_sanitize_leak_true 't006*'
+	add_sanitize_leak_true 't13*config*'
 
 	# Blacklist patterns (overrides whitelist)
 	add_sanitize_leak_false 't000[469]*'

2.32.0.853.g5a570c9bf9

Good point, I'll fix that.

I was doing this rather blindly to see if I could get this larg batch of tests to pass with some a minimal fixes/whitelisting of some "known bad".

This is a common leak pattern in Git. We do something like:

static const char *foo = "default";
...
int config_cb(const char *var, const char *value, void *)

  {
          if (!strcmp(var, "core.foo"))
	          foo = xstrdup(value);

}

So we leak if the variable appears twice. But we can't just call "free(foo)" here. In the first call, it's pointing to a string literal!

In the case of git_hooks_path, it defaults to NULL, so this works out OK. But it's setting up a trap for somebody later on, who assigns it a default value (and the compiler won't help; it's a "const char *", so the assignment is fine, and the free() would already be casting away the constness).

I see a few possible solutions:

- instead of strdup'ing long-lived config values, strintern() them. This is really leaking them, but in a way that we hold on to the old values. This is actually more or less what UNLEAK() is doing under the hood (saving a reference to the old buffer, even the variable is overwritten).

- find a way to tell when a string comes from the heap versus a

    literal. I don't think you can do this portably without keeping your
    own separate flag. We could abstract away some of the pain with a
    struct like:

       struct def_string {
               /* might point to heap memory; const because you must
                * check flag before modifying */
               const char *value;
               int from_heap;
       }

       /* regular static initialization is OK if you don't want a default */
       #define DEF_STRING_INIT(str) { .value = str }

       static void def_string_set(struct def_string *ds, const char *value)
       {
               if (ds->from_heap)
                       free(ds->value);
               ds->value = xstrdup(value);
               ds->from_heap = 1;
       }

    The annoying thing is all of the users need to refer to
    git_hook_path.value instead of just git_hook_path. If you don't mind
    a little macro hackery, we could get around that by declaring pairs
    of variables. Like:

      #define DEF_STRING_DECLARE(name, value) \
      const char *name = value; \
      int name##_from_heap

      #define DEF_STRING_SET(name, value) do { \
              if (name##_from_heap) \
                      free(name); \
              name = xstrdup(value); \
              name##_from_heap = 1; \
      } while(0)

I can't say I _love_ any of that, but I think it would work (and probably we'd adapt our helpers like git_config_pathname() to take a def_string. Or I guess just have a def_string_free() which can be called before writing into them).

But maybe there's a better solution I'm missing.

-Peff

Ah, right. I didn't think about the risk of future breakages.

Is it worth sidestepping the whole globals issue by migrating core.hookspath (and other string config values) to be fetched via git_config_get_pathname() and equivalents at the point of use instead?

I looked at the commit below which introduced git_config_get* which suggests that these methods were indeed intended to be an improvement over the callback based API, and IIUC switching over should have a bunch of advantages:
- Removes some potential bugs that can happen if git_config() was never
called with the right callback.
- Potentially reduces the number of times we have to iterate over the
config in the first place (assuming we migrate *all* config access
and not just strings).
- Fewer globals - which reduces potential for such leaks (and probably
makes it easier to read the code in the first place).
OTOH I'm not familiar enough with this code to know what the disadvantages of such a migration might be (it's definitely going to be a lot of work... but that's going to apply to any of the approaches we can choose to fix these leaks).

git_config_get* were introduced in:
3c8687a73e (add config_set API for caching config-like files,
2014-07-28)

Yeah, I almost suggested that. It probably does work OK in this instance, but it's a much bigger change to convert all cases.

I'm also not sure if you'd run into tricky details. For instance, calling git_config_* is doing an expensive-ish lookup in the cached config (well, expensive to accessing a single pointer). So in cases where we're going to access the string many times (say, in a loop), we'd want our own variable. That might end up being easy by calling it once outside the loop. Or it might not be, for cases where the variable is used under the hood by a helper function.

-Peff

Instead of: "int from_heap" in your "def_string" I think we should just use "struct string_list_item". I.e. you want a void* here. Why?

<Digression>

I have an unsent series for handling some more common cases in the string-list API. I started writing it due to a very related problem, i.e. that we conflate "string init dup/nodup" with "do we want to free?".

We (ab)use the "strdup_strings" in a few places to free that sort of thing at the end if we have heap-allocated strings, but ones we did not strdup ourselves, e.g. this in merge-ort.c (not picking on Elijah (CC'd) here, it's common in lots of places, and this one was pretty much lifted from merge-recursive).

        opti->paths_to_free.strdup_strings = 1;
        string_list_clear(&opti->paths_to_free, 0);
        opti->paths_to_free.strdup_strings = 0;

So I improved the string-list and strmap free functions so you can instead do:

    string_list_clear_strings((&opti->paths_to_free, 0);

And that along with some other changes allows you to clear (or not) any combination of the string, util, or have a callback function of your own run (but be ensured to run all of those before we get to any of the other freeing).

</Digression>

You must be thinking what any of this has to do with heap strings in C, well one common case you've not discussed is that we sometimes do the equivalent of, with string-list.h or not (somewhat pseudocode);

	void add_to_list(struct string_list *list, char *on_heap_now_we_own_it)
	{
		char *ptr = on_heap_now_we_own_it;
		char *mydup = xstrdup("foo");

	        ptr++; /* skip first byte */
		string_list_append(list, ptr);
		string_list_append(list, mydup);
	}

And:

        struct string_list list = STRING_LIST_INIT_NODUP;
        /* other stuff here, we get strings from somewhere etc. */
        add_to_list(list, some_string);

So now you're left with needing to free both at the end, but we since we did ptr++ there we can't free() that (we'd need to free(ptr - 1), but how to keep track of that?).

Well, tying this back to my clear() improvements for string-list.h I thought a really neat solution to this was:

    string_list_append(list, ptr)->util = on_heap_now_we_own_it;
    string_list_append(list, mydup)->util = mydup;

I.e. by convention we store the pointer we need to free (if any) in the "util" field.

And then if you get a string not from the heap you just leave the "util" as NULL, and at the end you just free() all your "util" fields, and it just so happens that some of them are the same as the "string" field.

We're not in the habit of passing loose "string_list_item" around now, but I don't see why we wouldn't (possibly with a change to extract that bit out, so we could use it in other places).

The neat thing about doing this is also that you're not left with every API boundary needing to deal with your new "def_string", a lot of them use string_list already, and hardly need to change anything, to the extent that we do need to change anything having a "void *util" is a lot more generally usable. You end up getting memory management for free as you gain a feature to pass arbitrary data along with your items.

Yes, an equivalent way to write it is with a separate to_free buffer. But why would we want it to be void? And why would we want to use a string_list_item, which is otherwise unrelated?

That works, but now "util" is not available for all the _other_ uses for which it was intended. And if we're not using it for those other uses, then why does it need to exist at all? If we are only using it to hold the allocated string pointer, then shouldn't it be "char *to_free"?

It seems unnecessarily confusing to me. It sounds like you have a struct which just _happens_ to have a "void *" in it you can re-use, so you start using it in lots of other places that are not in fact string lists at all. That is confusing to me on the face, but what happens when string_list needs a feature which requires adding more fields to it?

If the point is to have a maybe-allocated string, why not make that a type itself? And then if we want string_list to use it, it can.

I don't think most interfaces take a string_list_item now, so wouldn't they similarly need to be changed? Though the point is that all of these degrade to a regular C-string, so when you are just passing the value (and not ownership), you would just dereference at that point.

-Peff

On Fri, Jul 16 2021, Jeff King wrote:

[Very late reply, just getting back to this thread]

We could factor "string_list_item" out into a "string_pair" or whatever.

Sorry, I didn't mean to get into the naming/code location aspect of the discussion, just the sensibility of using a "char */void * pair" for these common memory management cases, v.s. your suggestion of having a "char *, int from_heap" pair.

Because having it be "char *" doesn't cover the common case of e.g. getting an already allocated "struct something *" which contains your string, setting the "string" in "struct string_list_item" to some string in that struct, and the "util" to the struct itself, as we now own it and want to free() it later in its entirety.

That and the even more common case I mentioned upthread of wanting to ferry around the truncated version of some char *, but still wanting to account for the original for an eventual free().

But yes, if you want to account for freeing that data *and* have util set to something else you'll need to have e.g. your own wrapper struct and your own string_list_clear_func() callback.

I'm not suggesting that this handles every possible scenario, just that having look at a lot of the code involved recently this seemed like a neat solution for the common cases.

*nod*, covered above. My examples were unnecessarily confusing...

Sure, just like things would need to be changed to handle your proposed "struct def_string".

By piggy-backing on an already used struct in our codebase we can get a lot of that memory management pretty much for free without much churn.

If you squint and pretend that "struct string_list_item" isn't called something to do with that particular collections API (but it would make use of it) then we've already set up most of the scaffolding and management for this.

OK. I buy that storing a void pointer makes it more flexible. I'm not altogether convinced this pattern is especially common, but it's not any harder to work with than a "need_to_free" flag, so there's no reason not to do that (and to be fair, I didn't look around for possible uses of the pattern; it's just not one I think of as common off the top of my head).

But stuffing it into the util field of string_list really feels like a stretch, and something that would make existing string_list use painful. There are tons of cases where util points to some totally unrelated (in terms of memory ownership) item. I'd venture to say most cases where string_list_clear() is called without free_util would count here.

It's that squinting that bothers me. Sure, it's _kinda_ similar. And I don't have any problem with some kind of struct that says "this is a string, and when you are done with it, this is how you free it". And I don't have any problem with building the "dup" version of string_list with that struct as a primitive. But it seems to me to be orthogonal from the "util" pointer of a string_list, which is about creating a mapping from the string to some other thing (which may or may not contain the string, and may or may not be owned).

TBH, I have always found the "util" field of string_list a bit ugly (and really most of string_list). I think most cases would be better off with a different data structure (a set or a hash table), but we didn't have convenient versions of those for a long time. I don't mind seeing conversions of string_list to other data structures. But that seems to be working against using string_list's string struct in more places.

-Peff

For what it's worth I've got some WIP code that's part of my daily build where I did end up going through all those callers, as part of general string_list_clear() improvements mentioned offhand in
https://lore.kernel.org/git/[email protected]/

This is just from fuzzy memory & I can't recall the specifics (and haven't combed through that WIP code now), but it's something like that in the ~100 uses of string_list in our codebase 60-70% are the simple case where the "strdup_strings" and string_list_clear() is enough, maybe another 10-20% have a "util" field they manage or not, 5%-ish have a simple string_list_clear_func().

It was just 2-3 cases that leaked memory due to skipping a prefix and sticking it in the list, and maybe another 1-2 where the void* to a struct containing the string stuck into the string slot was something we could use.

So it's not "common" in the sense of absolute numbers, but I did run into a handful of them, and having them handled by having the string_list take an arbitrary "util" was something I found neat.

I should probably have said "well known" (as in "well known technique"), "idiomatic" or something...

The "util" is whatever the user makes it. We could add a "pointer_to_free" to every container type to solve this more cleanly/generally at the API level, but just handing the problem to the user seems better to me. I.e. an API like string_list has convenience functions for freeing all the "util", if you only need it for memory tracking use it as-is, if you need a "real util" *and* such tracking just create a 2-member wrapper struct yourself & use that.

If we followed my idle musings we'd be using string_list_item in more places, not necessarily string_list, and would rename s/string_list_item/string_and_util/ or something.

One way to look at this problem is that we're pretty close to just re-inventing the sort of generalized refcounted container type that some programming languages carry around. E.g. Perl has a "struct SV*" that a $string maps to, but also hash and array values etc.

Those languages usually have a "refcount" or whatever, but since we're using this in native C and it's usually (or at least should be) clear who owns the memory just having something to point free() at will do.

I'm just saying that if we're going halfway there it would be unfortunate if we'd end up with a "struct def_string" which wouldn't handle this "borrowing a string from a struct" case.

Or maybe we should just use "struct strbuf" and do copying in even more places...

Fix a memory leak in a2ba162cda (object-info: support for retrieving object info, 2021-04-20) which appears to have been based on a misunderstanding of how the pkt-line.c API works, there is no need to strdup() input to, it's just a printf()-like format function.

This fixes a potentially large memory leak, since the number of OID lines the "object-info" call can be arbitrarily large (or a small one if the request is small).

---
 protocol-caps.c      | 5 +++--
 t/t5701-git-serve.sh | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

index 13a9e63a04..901b6795e4 100644
@@ -69,9 +69,10 @@ static void send_info(struct repository *r, struct packet_writer *writer,
 			}
 		}
 
-		packet_writer_write(writer, "%s",
-				    strbuf_detach(&send_buffer, NULL));
+		packet_writer_write(writer, "%s", send_buffer.buf);
+		strbuf_reset(&send_buffer);
 	}
+	strbuf_release(&send_buffer);
 }
 
 int cap_object_info(struct repository *r, struct strvec *keys,

index d58efb0aa9..e2f4832adf 100755
@@ -5,6 +5,7 @@ test_description='test protocol v2 server commands'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+GIT_TEST_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'test capability advertisement' '

2.32.0.853.g5a570c9bf9

Very nice. This will also be much more efficient, since we get to reuse the same buffer in each run through the loop.

-Peff

Fix a memory leak in a2ba162cda (object-info: support for retrieving object info, 2021-04-20) which appears to have been based on a misunderstanding of how the pkt-line.c API works. There is no need to strdup() input to packet_writer_write(), it's just a printf()-like format function.

This fixes a potentially large memory leak, since the number of OID lines the "object-info" call can be arbitrarily large (or a small one if the request is small).

This makes t5701-git-serve.sh pass again under SANITIZE=leak, as it did before a2ba162cda2.

---

This was originally submitted as part of the SANITIZE=leak series as
https://lore.kernel.org/git/[email protected]/

In its v3 I stopped doing these leak fixes & test changes, let's just consider this separately. We'll eventually want to add SANITIZE=leak whitelisting to the relevant test if and when my SANITIZE=leak series goes in, but we can just do that then along with adding various other tests.

Range-diff:
1: 720852eee0b ! 1: 9acbc21cdd3 SANITIZE tests: fix memory leaks in t5701*, add to whitelist
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- SANITIZE tests: fix memory leaks in t5701*, add to whitelist
+ protocol-caps.c: fix memory leak in send_info()

Fix a memory leak in a2ba162cda (object-info: support for retrieving object info, 2021-04-20) which appears to have been based on a
- misunderstanding of how the pkt-line.c API works, there is no need to
- strdup() input to, it's just a printf()-like format function.
+ misunderstanding of how the pkt-line.c API works. There is no need to
+ strdup() input to packet_writer_write(), it's just a printf()-like
+ format function.

This fixes a potentially large memory leak, since the number of OID lines the "object-info" call can be arbitrarily large (or a small one if the request is small).

+ This makes t5701-git-serve.sh pass again under SANITIZE=leak, as it
+ did before a2ba162cda2.
+

## protocol-caps.c ##
@@ protocol-caps.c: static void send_info(struct repository *r, struct packet_write
}

int cap_object_info(struct repository *r, struct strvec *keys,
-
- ## t/t5701-git-serve.sh ##
-@@ t/t5701-git-serve.sh: test_description='test protocol v2 server commands'
- GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
- export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
-+GIT_TEST_SANITIZE_LEAK=true
- . ./test-lib.sh
-
- test_expect_success 'test capability advertisement' '

 protocol-caps.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 13a9e63a04a..901b6795e42 100644
@@ -69,9 +69,10 @@ static void send_info(struct repository *r, struct packet_writer *writer,
 			}
 		}
 
-		packet_writer_write(writer, "%s",
-				    strbuf_detach(&send_buffer, NULL));
+		packet_writer_write(writer, "%s", send_buffer.buf);
+		strbuf_reset(&send_buffer);
 	}
+	strbuf_release(&send_buffer);
 }
 
 int cap_object_info(struct repository *r, struct strvec *keys,

2.33.0.805.g739b16c2189

On Tue, Aug 31, 2021 at 6:46 AM Ævar Arnfjörð Bjarmason <[email protected]> wrote:

[Replying again as I used HTML mail by mistake. Sorry.]

Thanks for cleaning up after me. Yes, this was my lack of knowledge on how the internals of Git works. I was also not aware of SANITIZE=leak so thanks for the heads up. This looks good to me.

Thanks, both.

Will apply.

Thanks, for what it's worth the series I submitted in parallel to this to add a SANITIZE=leak CI mode at
https://lore.kernel.org/git/[email protected]
could use reviewers :)

I.e. having some real tests for this sort of thing and running them in CI will help to catch any such issues earlier.

Get closer to being able to run t4203-mailmap.sh by fixing a couple of memory leak in mailmap.c.

In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to clear the "me" structure, but while we freed parts of the mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

---
 mailmap.c          | 2 ++
 t/t4203-mailmap.sh | 6 ++++++
 2 files changed, 8 insertions(+)

index d1f7c0d272..e1c8736093 100644
@@ -36,6 +36,7 @@ static void free_mailmap_info(void *p, const char *s)
 		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
+	free(mi);
 }
 
 static void free_mailmap_entry(void *p, const char *s)

@@ -51,6 +52,7 @@ static void free_mailmap_entry(void *p, const char *s)
 
 	me->namemap.strdup_strings = 1;
 	string_list_clear_func(&me->namemap, free_mailmap_info);
+	free(me);
 }
 
 /*

index 0b2d21ec55..c7de4299cf 100755
@@ -79,6 +79,12 @@ test_expect_success 'check-mailmap bogus contact --stdin' '
 	test_must_fail git check-mailmap --stdin bogus </dev/null
 '
 
+if test_have_prereq SANITIZE_LEAK
+then
+	skip_all='skipping the rest of mailmap tests under SANITIZE_LEAK'
+	test_done
+fi
+
 test_expect_success 'No mailmap' '
 	cat >expect <<-EOF &&
 	$GIT_AUTHOR_NAME (1):

2.32.0.853.g5a570c9bf9

In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to clear the "me" structure, but while we freed parts of the mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

This brings us from 50 failures when running t4203-mailmap.sh to
49. Not really progress as far as the number of failures is concerned, but as far as I can tell this fixes all leaks in mailmap.c itself. There's still users of it such as builtin/log.c that call read_mailmap() without a clear_mailmap(), but that's on them.

---

This was originally submitted as part of the SANITIZE=leak series as
https://lore.kernel.org/git/[email protected]/

In its v3 I stopped doing these leak fixes & test changes, let's just consider this separately. We'll eventually want to add SANITIZE=leak whitelisting to the relevant test if and when my SANITIZE=leak series goes in, but we can just do that then along with adding various other tests.

Range-diff:
1: 80edda308c9 ! 1: f11eb44e4c5 SANITIZE tests: fix leak in mailmap.c
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <[email protected]>

## Commit message ##
- SANITIZE tests: fix leak in mailmap.c
-
- Get closer to being able to run t4203-mailmap.sh by fixing a couple of
- memory leak in mailmap.c.
+ mailmap.c: fix a memory leak in free_mailap_{info,entry}()

In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to
@@ Commit message
mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

+ This brings us from 50 failures when running t4203-mailmap.sh to
+ 49. Not really progress as far as the number of failures is concerned,
+ but as far as I can tell this fixes all leaks in mailmap.c
+ itself. There's still users of it such as builtin/log.c that call
+ read_mailmap() without a clear_mailmap(), but that's on them.
+

## mailmap.c ##
@@ mailmap.c: static void free_mailmap_entry(void *p, const char *s)
}

/*
-
- ## t/t4203-mailmap.sh ##
-@@ t/t4203-mailmap.sh: test_expect_success 'check-mailmap bogus contact --stdin' '
- test_must_fail git check-mailmap --stdin bogus </dev/null
- '
-
-+if test_have_prereq SANITIZE_LEAK
-+then
-+ skip_all='skipping the rest of mailmap tests under SANITIZE_LEAK'
-+ test_done
-+fi
-+
- test_expect_success 'No mailmap' '
- cat >expect <<-EOF &&
- $GIT_AUTHOR_NAME (1):

 mailmap.c | 2 ++
 1 file changed, 2 insertions(+)

index 462b3956340..40ce152024d 100644
@@ -37,6 +37,7 @@ static void free_mailmap_info(void *p, const char *s)
 		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
+	free(mi);
 }
 
 static void free_mailmap_entry(void *p, const char *s)

@@ -52,6 +53,7 @@ static void free_mailmap_entry(void *p, const char *s)
 
 	me->namemap.strdup_strings = 1;
 	string_list_clear_func(&me->namemap, free_mailmap_info);
+	free(me);
 }
 
 /*

2.33.0.805.g739b16c2189

As a standalone patch, the "50 failures" is confusing and sounds quite alarming. Adding even a tiny bit of context:

s/50 failure/50 SANITIZE failures/

would help reduce the confusion. Alternatively, just dropping the second paragraph altogether would clear up any misunderstanding since the first paragraph and the patch body stand well on their own without any additional explanation.

Thanks, the patch looks good to me. I agree with Eric that mentioning "leak failures" in the second paragraph would make it less confusing. :)

-Peff

Here is what I queued.

Thanks, all.

From ccdd5d1eb14a6735c34428e856c0de33f1055520 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
<[email protected]>
Date: Tue, 31 Aug 2021 15:42:52 +0200
Subject: [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to clear the "me" structure, but while we freed parts of the mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure.

This brings the number of SANITIZE=leak failures in t4203-mailmap.sh down from 50 to 49. Not really progress as far as the number of failures is concerned, but as far as I can tell this fixes all leaks in mailmap.c itself. There's still users of it such as builtin/log.c that call read_mailmap() without a clear_mailmap(), but that's on them.

---
 mailmap.c | 2 ++
 1 file changed, 2 insertions(+)

index d1f7c0d272..e1c8736093 100644
@@ -36,6 +36,7 @@ static void free_mailmap_info(void *p, const char *s)
 		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
+	free(mi);
 }
 
 static void free_mailmap_entry(void *p, const char *s)

@@ -51,6 +52,7 @@ static void free_mailmap_entry(void *p, const char *s)
 
 	me->namemap.strdup_strings = 1;
 	string_list_clear_func(&me->namemap, free_mailmap_info);
+	free(me);
 }
 
 /*

2.33.0-323-g897a01baa9

The leak fixes look good to me, modulo the UNLEAK as already commented on in patch 2/4 - thank you!

I don't feel qualified to review the test and CI related scripting, hopefully someone else will be able to look at those changes :).

ATB,

Andrzej

[...snip...]

We can compile git with SANITIZE=leak, and have had various efforts in the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) to plug memory leaks, but have had no CI testing of it to ensure that we don't get regressions. This series adds a GIT_TEST_* mode for checking those regressions, and runs it in CI.

Since I submitted v2 the delta between origin/master..origin/seen broke even t0001-init.sh when run under SANITIZE=leak, so this series will cause test smoke on "seen". It's a prblom with another topic[1] though, which I and Emily will fix.

Changes since v2:

* In v2 compiling with SANITIZE=leak would change things so only known-good passing tests were run by default, everything else would pass as a dummy. Now the default running of tests is unchanged, but if we run with GIT_TEST_PASSING_SANITIZE_LEAK=true only those tests are run which set and export TEST_PASSES_SANITIZE_LEAK=true.

* The facility for declaring known-good tests in test-lib.sh based on wildcards is gone, instead individual tests need to declare if they're OK under SANITIZE=leak. This is done via "export TEST_PASSES_SANITIZE_LEAK=true", there's a handy import of "./test-pragma-SANITIZE=leak-ok.sh" before sourcing "./test-lib.sh" itself to set this.

* The various leak fixes are gone entirely. I'll submit some of those independently or as follow-ups.

* We now mark 57 tests in the test suite as OK under SANITIZE=leak. This is far from all of them, but gives us a decent set to start out with. The largest chunk of these is in t0*.sh.

* The CI job is not run under both GCC and Clang, but just whatever with one default compiler (which happens to be GCC). I'd missed that running under both was pointless.

It would be meaningful to run this under e.g. OSX & Windows too, as we take different codepaths there, but that can be left to a follow-up series.

1. https://lore.kernel.org/git/[email protected]/

Ævar Arnfjörð Bjarmason (8):
Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
CI: refactor "if" to "case" statement
tests: add a test mode for SANITIZE=leak, run it in CI
tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true
tests: annotate t001*.sh with TEST_PASSES_SANITIZE_LEAK=true
tests: annotate t002*.sh with TEST_PASSES_SANITIZE_LEAK=true
tests: annotate select t0*.sh with TEST_PASSES_SANITIZE_LEAK=true
tests: annotate select t*.sh with TEST_PASSES_SANITIZE_LEAK=true

 .github/workflows/main.yml              |  2 ++
 Makefile                                |  5 +++++
 ci/install-dependencies.sh              |  4 ++--
 ci/lib.sh                               | 29 +++++++++++++++++--------
 ci/run-build-and-tests.sh               |  4 ++--
 t/README                                |  7 ++++++
 t/t0000-basic.sh                        |  1 +
 t/t0001-init.sh                         |  1 +
 t/t0002-gitfile.sh                      |  1 +
 t/t0003-attributes.sh                   |  1 +
 t/t0004-unwritable.sh                   |  3 ++-
 t/t0005-signals.sh                      |  2 ++
 t/t0007-git-var.sh                      |  2 ++
 t/t0008-ignores.sh                      |  1 +
 t/t0010-racy-git.sh                     |  1 +
 t/t0011-hashmap.sh                      |  1 +
 t/t0013-sha1dc.sh                       |  1 +
 t/t0016-oidmap.sh                       |  1 +
 t/t0017-env-helper.sh                   |  1 +
 t/t0018-advice.sh                       |  1 +
 t/t0022-crlf-rename.sh                  |  1 +
 t/t0024-crlf-archive.sh                 |  1 +
 t/t0025-crlf-renormalize.sh             |  1 +
 t/t0026-eol-config.sh                   |  1 +
 t/t0029-core-unsetenvvars.sh            |  1 +
 t/t0030-stripspace.sh                   |  1 +
 t/t0052-simple-ipc.sh                   |  1 +
 t/t0061-run-command.sh                  |  1 +
 t/t0063-string-list.sh                  |  1 +
 t/t0066-dir-iterator.sh                 |  1 +
 t/t0067-parse_pathspec_file.sh          |  1 +
 t/t0091-bugreport.sh                    |  1 +
 t/t1010-mktree.sh                       |  1 +
 t/t1100-commit-tree-options.sh          |  1 +
 t/t1308-config-set.sh                   |  1 +
 t/t1309-early-config.sh                 |  1 +
 t/t1420-lost-found.sh                   |  1 +
 t/t1430-bad-ref-name.sh                 |  1 +
 t/t1509-root-work-tree.sh               |  1 +
 t/t2002-checkout-cache-u.sh             |  1 +
 t/t2050-git-dir-relative.sh             |  1 +
 t/t2081-parallel-checkout-collisions.sh |  1 +
 t/t2100-update-cache-badpath.sh         |  1 +
 t/t2200-add-update.sh                   |  1 +
 t/t2201-add-update-typechange.sh        |  1 +
 t/t2202-add-addremove.sh                |  1 +
 t/t2204-add-ignored.sh                  |  1 +
 t/t2300-cd-to-toplevel.sh               |  1 +
 t/t3000-ls-files-others.sh              |  1 +
 t/t3004-ls-files-basic.sh               |  1 +
 t/t3006-ls-files-long.sh                |  1 +
 t/t3008-ls-files-lazy-init-name-hash.sh |  1 +
 t/t3100-ls-tree-restrict.sh             |  1 +
 t/t3101-ls-tree-dirname.sh              |  1 +
 t/t3102-ls-tree-wildcards.sh            |  1 +
 t/t3103-ls-tree-misc.sh                 |  1 +
 t/t3205-branch-color.sh                 |  1 +
 t/t3211-peel-ref.sh                     |  1 +
 t/t3300-funny-names.sh                  |  1 +
 t/t3902-quoted.sh                       |  1 +
 t/t4002-diff-basic.sh                   |  1 +
 t/t4026-color.sh                        |  1 +
 t/t4300-merge-tree.sh                   |  1 +
 t/test-lib.sh                           | 22 +++++++++++++++++++
 t/test-pragma-SANITIZE=leak-ok.sh       |  8 +++++++
 65 files changed, 128 insertions(+), 14 deletions(-)
 create mode 100644 t/test-pragma-SANITIZE=leak-ok.sh

Range-diff against v2:

-:  ----------- > 1:  85619728d41 Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
-:  ----------- > 2:  91c36b94eaa CI: refactor "if" to "case" statement
1:  df5a44e70b5 ! 3:  7e3577e4e3c tests: add a test mode for SANITIZE=leak, run it in CI

@@ Commit message
corresponding GIT_TEST_* mode for it, i.e. memory leaks have been fixed as one-offs without structured regression testing.

- This change add such a mode, we now have new
- linux-{clang,gcc}-sanitize-leak CI targets, these targets run the same
- tests as linux-{clang,gcc}, except that almost all of them are
- skipped.
+ This change add such a mode, and a new linux-SANITIZE=leak CI
+ target. The test mode and CI target only runs a whitelist of
+ known-good tests using a mechanism discussed below, to ensure that we
+ won't add regressions to code that's had its memory leaks fixed.

- There is a whitelist of some tests that are OK in test-lib.sh, and
- individual tests can be opted-in by setting
- GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those
- individual test can be skipped with the "!SANITIZE_LEAK"
- prerequisite. See the updated t/README for more details.
+ The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test
+ mode. When running in that mode all tests except those that have opted
+ themselves in to running by setting and exporting
+ TEST_PASSES_SANITIZE_LEAK=true before sourcing test-lib.sh.

- I'm using the GIT_TEST_SANITIZE_LEAK=true and !SANITIZE_LEAK pattern
- in a couple of tests whose memory leaks I'll fix in subsequent
- commits.
+ I'm adding a "test-pragma-SANITIZE=leak-ok.sh" wrapper for setting and
+ exporting that variable, as the assignment/export boilerplate would
+ otherwise get quite verbose and repetitive in subsequent commits.

- I'm not being aggressive about opting in tests, it's not all tests
- that currently pass under SANITIZE=leak, just a small number of
- known-good tests. We can add more later as we fix leaks and grow more
- confident in this test mode.
+ The tests using the "test-pragma-SANITIZE=leak-ok.sh" pragma can in
+ turn make use of the "SANITIZE_LEAK" prerequisite added in a preceding
+ commit, should they wish to selectively skip tests even under
+ "GIT_TEST_PASSING_SANITIZE_LEAK=true".

- See the recent discussion at [1] about the lack of this sort of test
- mode, and 0e5bba53af (add UNLEAK annotation for reducing leak false
- positives, 2017-09-08) for the initial addition of SANITIZE=leak.
+ Now tests that don't set the "test-pragma-SANITIZE=leak-ok.sh" pragma
+ will be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:
+
+ $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0001-init.sh
+ 1..0 # SKIP skip all tests in t0001 under SANITIZE=leak, TEST_PASSES_SANITIZE_LEAK not set
+
+ In subsequents commit we'll conservatively add more
+ TEST_PASSES_SANITIZE_LEAK=true annotations. The idea is that as memory
+ leaks are fixed we can add more known-good tests to this CI target, to
+ ensure that we won't have regressions.
+
+ As of writing this we've got major regressions between master..seen,
+ i.e. the t000*.sh tests and more fixed since 31f9acf9ce2 (Merge branch
+ 'ah/plugleaks', 2021-08-04) have regressed recently.
+
+ See the discussion at <[email protected]> about the
+ lack of this sort of test mode, and 0e5bba53af (add UNLEAK annotation
+ for reducing leak false positives, 2017-09-08) for the initial
+ addition of SANITIZE=leak.

See also 09595ab381 (Merge branch 'jk/leak-checkers', 2017-09-19), 7782066f67 (Merge branch 'jk/apache-lsan', 2019-05-19) and the recent 936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the past history of "one-off" SANITIZE=leak (and more) fixes.

- When calling maybe_skip_all_sanitize_leak matching against
- "$TEST_NAME" instead of "$this_test" as other "match_pattern_list()"
- users do is intentional. I'd like to match things like "t13*config*"
- in subsequent commits. This part of the API isn't public, so we can
- freely change it in the future.
-
- 1. https://lore.kernel.org/git/[email protected]/
-

## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
- jobname: linux-gcc-default
cc: gcc
pool: ubuntu-latest
-+ - jobname: linux-clang-sanitize-leak
-+ cc: clang
-+ pool: ubuntu-latest
-+ - jobname: linux-gcc-sanitize-leak
-+ cc: gcc
++ - jobname: linux-SANITIZE=leak
+ pool: ubuntu-latest
env:
CC: ${{matrix.vector.cc}}
jobname: ${{matrix.vector.jobname}}

- ## Makefile ##
-@@ Makefile: PTHREAD_CFLAGS =
- SPARSE_FLAGS ?=
- SP_EXTRA_FLAGS = -Wno-universal-initializer
-
-+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
-+SANITIZE_LEAK =
-+
- # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
- # usually result in less CPU usage at the cost of higher peak memory.
- # Setting it to 0 will feed all files in a single spatch invocation.
-@@ Makefile: BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
- endif
- ifneq ($(filter leak,$(SANITIZERS)),)
- BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
-+SANITIZE_LEAK = YesCompiledWithIt
- endif
- ifneq ($(filter address,$(SANITIZERS)),)
- NO_REGEX = NeededForASAN
-@@ Makefile: GIT-BUILD-OPTIONS: FORCE
- @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
- @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
- @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
-+ @echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
- @echo X=\'$(X)\' >>$@+
- ifdef TEST_OUTPUT_DIRECTORY
- @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
-
## ci/install-dependencies.sh ##
@@ ci/install-dependencies.sh: UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"

case "$jobname" in
-linux-clang|linux-gcc)
-+linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak) ++linux-clang|linux-gcc|linux-SANITIZE=leak)
sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test" sudo apt-get -q update
sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
$UBUNTU_COMMON_PKGS
case "$jobname" in
- linux-gcc)
-+ linux-gcc|linux-gcc-sanitize-leak)
++ linux-gcc|linux-SANITIZE=leak)
sudo apt-get -q -y install gcc-8
;;
esac
@@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true

case "$jobname" in
-linux-clang|linux-gcc)
-- if [ "$jobname" = linux-gcc ]
-- then
-+linux-clang|linux-gcc|linux-clang-sanitize-leak|linux-gcc-sanitize-leak)
-+ case "$jobname" in
-+ linux-gcc|linux-gcc-sanitize-leak)
++linux-clang|linux-gcc|linux-SANITIZE=leak)
+ case "$jobname" in
+- linux-gcc)
++ linux-gcc|linux-SANITIZE=leak)
export CC=gcc-8
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
-- else
-+ ;;
-+ *)
- MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
-- fi
-+ ;;
-+ esac
-
- export GIT_TEST_HTTPD=true
-
+ ;;
@@ ci/lib.sh: linux-musl)
;;
esac

+case "$jobname" in
-+linux-clang-sanitize-leak|linux-gcc-sanitize-leak) ++linux-SANITIZE=leak)
+ export SANITIZE=leak
++ export GIT_TEST_PASSING_SANITIZE_LEAK=true
+ ;;
+esac
+
@@ ci/run-build-and-tests.sh: esac
make
case "$jobname" in
-linux-gcc)
-+linux-gcc|linux-gcc-sanitize-leak)
++linux-gcc|linux-SANITIZE=leak)
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
make test
export GIT_TEST_SPLIT_INDEX=yes
@@ ci/run-build-and-tests.sh: linux-gcc)
make test
;;
-linux-clang)
-+linux-clang|linux-clang-sanitize-leak) ++linux-clang|linux-SANITIZE=leak)
export GIT_TEST_DEFAULT_HASH=sha1
make test
export GIT_TEST_DEFAULT_HASH=sha256
@@ t/README: GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting
to <n> and 'checkout.thresholdForParallelism' to 0, forcing the execution of the parallel-checkout code.

-+GIT_TEST_SANITIZE_LEAK=<boolean> will force the tests to run when git -+is compiled with SANITIZE=leak (we pick it up via -+../GIT-BUILD-OPTIONS).
-+
-+By default all tests are skipped when compiled with SANITIZE=leak, and -+individual test scripts opt themselves in to leak testing by setting -+GIT_TEST_SANITIZE_LEAK=true before sourcing test-lib.sh. Within those -+tests use the SANITIZE_LEAK prerequisite to skip individiual tests -+(i.e. test_expect_success !SANITIZE_LEAK [...]).
-+
-+So the GIT_TEST_SANITIZE_LEAK setting is different in behavior from -+both other GIT_TEST_*=[true|false] settings, but more useful given how -+SANITIZE=leak works & the state of the test suite. Manually setting -+GIT_TEST_SANITIZE_LEAK=true is only useful during development when -+finding and fixing memory leaks. ++GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> when compiled with ++SANITIZE=leak will run only those tests that have whitelisted ++themselves as passing with no memory leaks. Do this by sourcing ++"test-pragma-SANITIZE=leak-ok.sh" before sourcing "test-lib.sh" itself ++at the top of the test script. This test mode is used by the ++"linux-SANITIZE=leak" CI target.
+
Naming Tests
------------

- ## t/t5701-git-serve.sh ##
-@@ t/t5701-git-serve.sh: test_expect_success 'unexpected lines are not allowed in fetch request' '
+ ## t/t0000-basic.sh ##
+@@ t/t0000-basic.sh: swapping compression and hashing order, the person who is making the
+ modification *should* take notice and update the test vectors here.
+ '
+
++. ./test-pragma-SANITIZE=leak-ok.sh
+ . ./test-lib.sh

- # Test the basics of object-info
- #
--test_expect_success 'basics of object-info' ' -+test_expect_success !SANITIZE_LEAK 'basics of object-info' '
- test-tool pkt-line pack >in <<-EOF &&
- command=object-info
- object-format=$(test_oid algo)
+ try_local_xy () {

## t/test-lib.sh ##
-@@ t/test-lib.sh: then
- exit 1
- fi
-
-+# SANITIZE=leak test mode
-+sanitize_leak_true=
-+add_sanitize_leak_true () {
-+ sanitize_leak_true="$sanitize_leak_true$1 "
-+}
-+
-+sanitize_leak_false=
-+add_sanitize_leak_false () {
-+ sanitize_leak_false="$sanitize_leak_false$1 "
-+}
-+
-+sanitize_leak_opt_in_msg="opt-in with GIT_TEST_SANITIZE_LEAK=true" -+maybe_skip_all_sanitize_leak () {
-+ # Whitelist patterns
-+ add_sanitize_leak_true 't000*'
-+ add_sanitize_leak_true 't001*'
-+ add_sanitize_leak_true 't006*'
-+
-+ # Blacklist patterns (overrides whitelist)
-+ add_sanitize_leak_false 't000[469]*'
-+ add_sanitize_leak_false 't001[2459]*'
-+ add_sanitize_leak_false 't006[0248]*'
-+
-+ if match_pattern_list "$1" "$sanitize_leak_false"
-+ then
-+ skip_all="test $this_test on SANITIZE=leak blacklist, $sanitize_leak_opt_in_msg"
-+ test_done
-+ elif match_pattern_list "$1" "$sanitize_leak_true"
-+ then
-+ return 0
-+ fi
-+ return 1
-+}
-+
- # Are we running this test at all?
- remove_trash=
- this_test=${0##*/}
@@ t/test-lib.sh: then
test_done
fi
@@ t/test-lib.sh: then
+# SANITIZE=leak
+if test -n "$SANITIZE_LEAK"
+then
-+ if test -z "$GIT_TEST_SANITIZE_LEAK" &&
-+ maybe_skip_all_sanitize_leak "$TEST_NAME"
++ if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+ then
-+ say_color info >&3 "test $this_test on SANITIZE=leak whitelist"
-+ GIT_TEST_SANITIZE_LEAK=true
-+ fi
++ # We need to see it in "git env--helper" (via
++ # test_bool_env)
++ export TEST_PASSES_SANITIZE_LEAK
+
-+ # We need to see it in "git env--helper" (via
-+ # test_bool_env)
-+ export GIT_TEST_SANITIZE_LEAK
-+
-+ if ! test_bool_env GIT_TEST_SANITIZE_LEAK false
-+ then
-+ skip_all="skip all tests in $this_test under SANITIZE=leak, $sanitize_leak_opt_in_msg"
-+ test_done
++ if ! test_bool_env TEST_PASSES_SANITIZE_LEAK false
++ then
++ skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
++ test_done
++ fi
+ fi
-+elif test_bool_env GIT_TEST_SANITIZE_LEAK false
++elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+then
-+ error "GIT_TEST_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak"
++ error "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak"
+fi
+
# Last-minute variable setup
HOME="$TRASH_DIRECTORY" GNUPGHOME="$HOME/gnupg-home-not-used"
-@@ t/test-lib.sh: test -z "$NO_PYTHON" && test_set_prereq PYTHON
- test -n "$USE_LIBPCRE2" && test_set_prereq PCRE
- test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
- test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
-+test -n "$SANITIZE_LEAK" && test_set_prereq SANITIZE_LEAK -
- if test -z "$GIT_TEST_CHECK_CACHE_TREE"
- then
+
+ ## t/test-pragma-SANITIZE=leak-ok.sh (new) ##
+@@
++#!/bin/sh
++
++## This "pragma" (as in "perldoc perlpragma") declares that the test ++## will pass under GIT_TEST_PASSING_SANITIZE_LEAK=true. Source this ++## before sourcing test-lib.sh
++
++TEST_PASSES_SANITIZE_LEAK=true
++export TEST_PASSES_SANITIZE_LEAK

2:  51fd1c400da < -:  ----------- SANITIZE tests: fix memory leaks in t13*config*, add to whitelist
3:  720852eee0b < -:  ----------- SANITIZE tests: fix memory leaks in t5701*, add to whitelist
4:  80edda308c9 < -:  ----------- SANITIZE tests: fix leak in mailmap.c
-:  ----------- > 4:  0cd14d64165 tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true
-:  ----------- > 5:  ed5f5705755 tests: annotate t001*.sh with TEST_PASSES_SANITIZE_LEAK=true
-:  ----------- > 6:  2599016c4e7 tests: annotate t002*.sh with TEST_PASSES_SANITIZE_LEAK=true
-:  ----------- > 7:  ddc4d6d2cf1 tests: annotate select t0*.sh with TEST_PASSES_SANITIZE_LEAK=true
-:  ----------- > 8:  e611d2c23d9 tests: annotate select t*.sh with TEST_PASSES_SANITIZE_LEAK=true

2.33.0.805.g739b16c2189

Hmm. This still seems more complicated than we need. If we just want a flag in each script, then test-lib.sh can use that flag to tweak LSAN_OPTIONS. See the patch below.

That has two drawbacks:

- it doesn't have any way to switch the flag per-test. But IMHO it is a mistake to go in that direction. This is all temporary scaffolding while we have leaks, and the script-level of granularity is fine.

- it runs the tests not marked as LSAN-OK, just without leak checking, which is redundant in CI where we're already running them. But we could still be collecting leak stats (and just not failing the tests). See the patch below.

If we do care about not running them, then I think it makes more sense to extend the run/skip mechanisms and build on that.

(I also think I prefer the central list of "mark these scripts as OK for leak-checking", rather than annotating individuals. Because again, this is temporary, and it's nice to keep it in a sandbox that only people working on leak-checking would look at or touch).

I realize this is kind-of bikeshedding, and I'm not vehemently opposed to what you have here. It just seems like fewer moving parts would be less likely to confuse folks who want to poke at it.

I found the extra level of indirection added by this pragma confusing. We just need to set a variable, which is also a one-liner, and one that is more obvious about what it's doing. In your code you also export it, but that's not necessary for something that test-lib.sh is going to look at. Or if it's really necessary at some point, then test-lib.sh can do the export itself.

Sort of a meta-question, but what's the plan for folks who add a new test to say t0000, and it reveals a leak in code they didn't touch?

They'll get a CI failure (as will Junio if he picks up the patch), so somebody is going to have to deal with it. Do they fix it? Do they unset the "this script is OK" flag? Do they mark the individual test as non-lsan-ok?

I do like the idea of finding real regressions. But while the state of leak-checking is still so immature, I'm worried about this adding extra friction for developers. Especially if they get some spooky action at a distance caused by a leak in far-away code.

Anyway, here's LSAN_OPTIONS thing I was thinking of.

---

index df544bb321..b1da18955d 100755
@@ -2,6 +2,7 @@
 
 test_description='git init'
 
+TEST_LSAN_OK=1
 . ./test-lib.sh
 
 check_config () {

index abcfbed6d6..62627afeaf 100644
@@ -44,9 +44,30 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 : ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
 export ASAN_OPTIONS
 
-# If LSAN is in effect we _do_ want leak checking, but we still
-# want to abort so that we notice the problems.
-: ${LSAN_OPTIONS=abort_on_error=1}
+if test -n "$LSAN_OPTIONS"
+then
+	# Leave user-provided options alone.
+	:
+elif test -n "$TEST_LSAN_OK"
+then
+	# The test script has declared itself as LSAN-clean; turn on full leak
+	# checking.
+	LSAN_OPTIONS=abort_on_error=1
+else
+	# The test script has possible LSAN failures. Just disable
+	# leak-checking entirely. Another option would be to log the failures
+	# with:
+	#
+	#   LSAN_OPTIONS=exitcode=0:log_path=$TEST_DIRECTORY/lsan/out
+	#
+	# The results are rather confusing, though, as the logs are
+	# per-process; you have no idea which one came from which test script.
+	# Ideally we'd send them to descriptor 4 along with the rest of the
+	# script log, but there's no LSAN_OPTION for that (recent versions of
+	# libsanitizer do have a public function to do so, so we could hook it
+	# ourselves via common-main).
+	LSAN_OPTIONS=detect_leaks=0
+fi
 export LSAN_OPTIONS
 
 if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS

On the "pragma" include v.s. env var + export: I figured this would be easier to read as I thought the export was required (I don't think it is in most cases, but e.g. for t0000*.sh I think it is, but that's from memory...).

We have a lot of tests that do simple checking of the tool itself, and later in the script might be stressing trace2, or common sources of leaks like "git log" in combination with the tool (e.g. the commit-graph tests).

So being able to tweak this inside the script is useful, but that can of course also be done with this proposed TEST_LSAN_OK + prereq.

Sure, I'd prefer

The patch I have here is already nicely integrated with the skip mechanism. I.e. we use skip_all which shows a summary in any TAP consumer, and we can skip individual tests with prerequisites.

I can see that for the proposed v2 mechanism, but in this v3 nothing changes unless you opt-in to things via new GIT_TEST_* setting. So the chance for confusion seems minimal to nonexisting.

I was interested in doing some summaries of existing leaks eventually. It seems even with LSAN_OPTIONS=detect_leaks=0 compiling with SANITIZE=leak make things a bit slower, but not by much (but actual leak checking is much slower).

But I'd prefer to leave any "write out leak logs and summarize" step for some later change.

*nod*, will remove it per discussion above.

Then CI will fail on this job. We'd have those same failures now (e.g. the mentioned current delta between master..seen), we just don't see them. Having visibility on them seems like an improvement.

I'd think they'd fix it, or make marking the regression as OK part of their re-roll, just like failures on master..seen now.

If you're getting at that we should start out this job as an FYI job that doesn't impact the CI run's overall status if it fails I think that would be OK as a start.

Yeah, ultimately this series is an implicit endorsement of us caring more than we do now.

I think this friction point is going to be mitigated a lot by the ability I've added to not just skip entire test scripts, but allow prereq skipping of some tests, early bailing out etc.

It allows you to say add a "git log" test at the end of some test that otherwise just uses some core API or a test tool and not have to throw the baby out with the bathwater in terms of disabling all existing leak checks there to make forward progress (or split up the entire test script).

Thanks, that & your follow-up is very interesting. Can I assume this has your SOB? I'd like to add that redirect to fd 4 change to this series.

I admit that half of my complaint with the pragma is the weird filename with an "=" in it. :) But I do think just assigning the variable is the most readable thing. If t0000 needs to export for whatever reason, it can do so (preferably with a comment explaining why).

Getting rid of the "let's tell the tests that we were built with LSAN" was part of the simplicity I was going for (and obviously does preclude a prerequisite). I had hoped we wouldn't need to do per-test stuff, because this was all a temporary state. But maybe that's naive.

I meant here that we'd be driving the selection externally from the tests using the skip/run mechanisms (something along the lines of what I sketched out before).

But I admit that there isn't really a big difference between the two approaches. Since you've coded this one up already, let's go in that direction (i.e., this series).

OK, I can live with that (especially given how apparently difficult it is to convince LSAN to do it).

I think that would be OK, but I'm not quite sure of the best way to do it. Why don't we start it as a regular required job, and then we can see how often it is causing a headache. If once every few months somebody fixes a leak, I'd be happy. If new developers are getting tangled up constantly in unrelated leaks, then that's something we'd need to revisit.

I half-agree with your final paragraph. The biggest friction point I think will be for new folks when CI starts failing, and they don't understand why (or where the problem is, or how to debug it, etc). But like I said, let's see what happens.

Yes, go for it.

-Peff

We can compile git with SANITIZE=leak, and have had various efforts in the past such as 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) to plug memory leaks, but have had no CI testing of it to ensure that we don't get regressions. This series adds a GIT_TEST_* mode for checking those regressions, and runs it in CI.

Since I submitted v2 the delta between origin/master..origin/seen broke even t0001-init.sh when run under SANITIZE=leak, so this series will cause test smoke on "seen".

That failure is due to a bug in es/config-based-hooks [1] and the hn/reftable topic, i.e. these patches are legitimately catching regressions in "seen" from day 1.

Changes since v3:

* Much updated commit message

* Re-arranged the t/README change to avoid a conflict with "seen".

* Now testing OSX as well as Linux. Full CI passes on top of "master" on both: https://github.com/avar/git/runs/3535331215

* I ejected the previous 4-8/8 patches of adding SANITIZE=leak annotations to various tests, let's focus on the test mode itself here and not overly distracting ourselves with whatever other regressions on "seen" those annotations might cause, I can submit those annotations later.

* As noted in the updated commit message I didn't end up going with Jeff King's suggestion of supporting LSAN_OPTIONS directly, and fixing the "fd" the tests write to. All of those things can be extended or fixed later.

1. https://lore.kernel.org/git/[email protected]/ [1]

Ævar Arnfjörð Bjarmason (3):
Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
CI: refactor "if" to "case" statement
tests: add a test mode for SANITIZE=leak, run it in CI

 .github/workflows/main.yml |  6 ++++++
 Makefile                   |  5 +++++
 ci/install-dependencies.sh |  6 +++---
 ci/lib.sh                  | 31 +++++++++++++++++++++----------
 ci/run-build-and-tests.sh  |  2 +-
 t/README                   |  7 +++++++
 t/t0000-basic.sh           |  1 +
 t/t0004-unwritable.sh      |  3 ++-
 t/test-lib.sh              | 21 +++++++++++++++++++++
 9 files changed, 67 insertions(+), 15 deletions(-)

Range-diff against v3:

1:  85619728d41 = 1:  bdfe2279271 Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
2:  91c36b94eaa ! 2:  6aaa60e3759 CI: refactor "if" to "case" statement

@@ Metadata
## Commit message ##
CI: refactor "if" to "case" statement

- Refactor an "if" statement for "linux-gcc" to a "case" statement in
- preparation for another case being added to it, and do the same for
- the "osx-gcc" just below it for consistency.
+ Refactor an "if" statement for "linux-gcc" and "osx-gcc" to a "case"
+ statement in preparation for another case being added to them.

3: 7e3577e4e3c ! 3: fffbfc35c00 tests: add a test mode for SANITIZE=leak, run it in CI
@@ Metadata
## Commit message ##
tests: add a test mode for SANITIZE=leak, run it in CI

- While git can be compiled with SANITIZE=leak there has been no
- corresponding GIT_TEST_* mode for it, i.e. memory leaks have been
- fixed as one-offs without structured regression testing.
+ While git can be compiled with SANITIZE=leak we have not run
+ regression tests under that mode, memory leaks have only been fixed as
+ one-offs without structured regression testing.

- This change add such a mode, and a new linux-SANITIZE=leak CI
- target. The test mode and CI target only runs a whitelist of
- known-good tests using a mechanism discussed below, to ensure that we
- won't add regressions to code that's had its memory leaks fixed.
+ This change add CI testing for it. We'll now build with GCC under
+ Linux and test t000[04]*.sh with SANITIZE=leak, and likewise with GCC
+ on OSX. The new jobs are called "linux-SANITIZE=leak" and
+ "osx-SANITIZE=leak".

The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test
- mode. When running in that mode all tests except those that have opted
- themselves in to running by setting and exporting
- TEST_PASSES_SANITIZE_LEAK=true before sourcing test-lib.sh.
+ mode. When running in that mode, we'll assert that we were compiled
+ with SANITIZE=leak, and then skip all tests except those that we've
+ opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing
+ test-lib.sh (see discussion in t/README).

- I'm adding a "test-pragma-SANITIZE=leak-ok.sh" wrapper for setting and
- exporting that variable, as the assignment/export boilerplate would
- otherwise get quite verbose and repetitive in subsequent commits.
+ The tests using the "TEST_PASSES_SANITIZE_LEAK=true" setting can in
+ turn make use of the "SANITIZE_LEAK" prerequisite, should they wish to
+ selectively skip tests even under
+ "GIT_TEST_PASSING_SANITIZE_LEAK=true". In a preceding commit we
+ started doing this in "t0004-unwritable.sh" under SANITIZE=leak, now
+ it'll combine nicely with "GIT_TEST_PASSING_SANITIZE_LEAK=true".

- The tests using the "test-pragma-SANITIZE=leak-ok.sh" pragma can in
- turn make use of the "SANITIZE_LEAK" prerequisite added in a preceding
- commit, should they wish to selectively skip tests even under
- "GIT_TEST_PASSING_SANITIZE_LEAK=true".
-
- Now tests that don't set the "test-pragma-SANITIZE=leak-ok.sh" pragma
- will be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:
+ Now tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will be
+ skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:

$ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0001-init.sh
1..0 # SKIP skip all tests in t0001 under SANITIZE=leak, TEST_PASSES_SANITIZE_LEAK not set

- In subsequents commit we'll conservatively add more
- TEST_PASSES_SANITIZE_LEAK=true annotations. The idea is that as memory
- leaks are fixed we can add more known-good tests to this CI target, to
- ensure that we won't have regressions.
+ The intent is to add more TEST_PASSES_SANITIZE_LEAK=true annotations
+ as follow-up change, but let's start small to begin with.
+
+ It would also be possible to implement a more lightweight version of
+ this by only relying on setting "LSAN_OPTIONS". See
+ <YS9OT/[email protected]>[1] and
+ <[email protected]>[2] for a discussion of
+ that. I've opted for this approach of adding a GIT_TEST_* mode instead
+ because it's consistent with how we handle other special test modes.
+
+ Being able to add a "!SANITIZE_LEAK" prerequisite and calling
+ "test_done" early if it isn't satisfied also means that we can more
+ incrementally add regression tests without being forced to fix
+ widespread and hard-to-fix leaks at the same time.
+
+ We have tests that do simple checking of some tool we're interested
+ in, but later on in the script might be stressing trace2, or common
+ sources of leaks like "git log" in combination with the tool (e.g. the
+ commit-graph tests). To be clear having a prerequisite could also be
+ accomplished by using "LSAN_OPTIONS" directly.
+
+ On the topi of "LSAN_OPTIONS": It would be nice to have a mode to
+ aggregate all failures in our various scripts, see [2] for a start at
+ doing that which sets "log_path" in "LSAN_OPTIONS". I've punted on
+ that for now, it can be added later, and that proposed patch is also
+ hindered by us wanting to test e.g. test-tool leaks (and by proxy, any
+ API leaks they uncover), not just the "common-main.c" entry point.

As of writing this we've got major regressions between master..seen, i.e. the t000*.sh tests and more fixed since 31f9acf9ce2 (Merge branch
@@ Commit message
936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the past history of "one-off" SANITIZE=leak (and more) fixes.

+ The reason for using gcc on OSX over the clang default is because
+ it'll currently fail to build with:
+
+ clang: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin19.6.0'
+
+ If that's sorted out in the future we might want to run that job with
+ "clang" merely to make use of the default, and also to add some
+ compiler variance into the mix. Both use the
+ "AddressSanitizerLeakSanitizer" library[3], so in they shouldn't be
+ have differently under GCC or clang.
+
+ 1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
+ 2. https://lore.kernel.org/git/YS9OT%[email protected]/
+ 3. https://lore.kernel.org/git/[email protected]/
+

## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
cc: gcc
pool: ubuntu-latest
+ - jobname: linux-SANITIZE=leak
++ cc: gcc
+ pool: ubuntu-latest
++ - jobname: osx-SANITIZE=leak
++ cc: gcc
++ pool: macos-latest
env:
CC: ${{matrix.vector.cc}}
jobname: ${{matrix.vector.jobname}}
@@ ci/install-dependencies.sh: UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl
sudo apt-get -q -y install gcc-8
;;
esac
+@@ ci/install-dependencies.sh: linux-clang|linux-gcc)
+ cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
+ popd
+ ;;
+-osx-clang|osx-gcc)
++osx-clang|osx-gcc|osx-SANITIZE=leak)
+ export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
+ # Uncomment this if you want to run perf tests:
+ # brew install gnu-time

## ci/lib.sh ##
@@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true
@@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true
export CC=gcc-8
MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3" ;;
+@@ ci/lib.sh: linux-clang|linux-gcc)
+ GIT_LFS_PATH="$HOME/custom/git-lfs"
+ export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
+ ;;
+-osx-clang|osx-gcc) ++osx-clang|osx-gcc|osx-SANITIZE=leak)
+ case "$jobname" in
+- osx-gcc)
++ osx-gcc|osx-SANITIZE=leak)
+ export CC=gcc-9
+ MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
+ ;;
@@ ci/lib.sh: linux-musl)
;;
esac

+case "$jobname" in
-+linux-SANITIZE=leak) ++linux-SANITIZE=leak|osx-SANITIZE=leak)
+ export SANITIZE=leak
+ export GIT_TEST_PASSING_SANITIZE_LEAK=true
+ ;;
@@ ci/run-build-and-tests.sh: esac
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main make test
export GIT_TEST_SPLIT_INDEX=yes
-@@ ci/run-build-and-tests.sh: linux-gcc)
- export GIT_TEST_CHECKOUT_WORKERS=2
- make test
- ;;
--linux-clang) -+linux-clang|linux-SANITIZE=leak)
- export GIT_TEST_DEFAULT_HASH=sha1
- make test
- export GIT_TEST_DEFAULT_HASH=sha256

## t/README ##
-@@ t/README: GIT_TEST_CHECKOUT_WORKERS=<n> overrides the 'checkout.workers' setting
- to <n> and 'checkout.thresholdForParallelism' to 0, forcing the
- execution of the parallel-checkout code.
+@@ t/README: excluded as so much relies on it, but this might change in the future.
+ GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
+ test suite. Accept any boolean values that are accepted by git-config.

+GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> when compiled with +SANITIZE=leak will run only those tests that have whitelisted
-+themselves as passing with no memory leaks. Do this by sourcing -+"test-pragma-SANITIZE=leak-ok.sh" before sourcing "test-lib.sh" itself -+at the top of the test script. This test mode is used by the -+"linux-SANITIZE=leak" CI target.
++themselves as passing with no memory leaks. Tests can be whitelisted ++by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing ++"test-lib.sh" itself at the top of the test script. This test mode is ++used by the "linux-SANITIZE=leak" CI target.
+
- Naming Tests
- ------------
+ GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
+ default to n.

## t/t0000-basic.sh ##
@@ t/t0000-basic.sh: swapping compression and hashing order, the person who is maki
modification *should* take notice and update the test vectors here. '

-+. ./test-pragma-SANITIZE=leak-ok.sh
++TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh

try_local_xy () {

+ ## t/t0004-unwritable.sh ##
+@@
+
+ test_description='detect unwritable repository and fail correctly' +
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success setup '
+
## t/test-lib.sh ##
@@ t/test-lib.sh: then
test_done
fi

-+# Aggressively skip non-whitelisted tests when compiled with -+# SANITIZE=leak
++# skip non-whitelisted tests when compiled with SANITIZE=leak
+if test -n "$SANITIZE_LEAK"
+then
+ if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
@@ t/test-lib.sh: then
# Last-minute variable setup
HOME="$TRASH_DIRECTORY" GNUPGHOME="$HOME/gnupg-home-not-used"
-
- ## t/test-pragma-SANITIZE=leak-ok.sh (new) ##
-@@
-+#!/bin/sh
-+
-+## This "pragma" (as in "perldoc perlpragma") declares that the test -+## will pass under GIT_TEST_PASSING_SANITIZE_LEAK=true. Source this -+## before sourcing test-lib.sh
-+
-+TEST_PASSES_SANITIZE_LEAK=true
-+export TEST_PASSES_SANITIZE_LEAK

4:  0cd14d64165 < -:  ----------- tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true
5:  ed5f5705755 < -:  ----------- tests: annotate t001*.sh with TEST_PASSES_SANITIZE_LEAK=true
6:  2599016c4e7 < -:  ----------- tests: annotate t002*.sh with TEST_PASSES_SANITIZE_LEAK=true
7:  ddc4d6d2cf1 < -:  ----------- tests: annotate select t0*.sh with TEST_PASSES_SANITIZE_LEAK=true
8:  e611d2c23d9 < -:  ----------- tests: annotate select t*.sh with TEST_PASSES_SANITIZE_LEAK=true

2.33.0.818.gd2ef2916285

When SANITIZE=leak is specified we'll now add a SANITIZE_LEAK flag to GIT-BUILD-OPTIONS, this can then be picked up by the test-lib.sh, which sets a SANITIZE_LEAK prerequisite.

We can then skip specific tests that are known to fail under SANITIZE=leak, add one such annotation to t0004-unwritable.sh, which now passes under SANITIZE=leak.

---
 Makefile              | 5 +++++
 t/t0004-unwritable.sh | 2 +-
 t/test-lib.sh         | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

index 429c276058d..34c12ea6e6f 100644
@@ -1221,6 +1221,9 @@ PTHREAD_CFLAGS =
 SPARSE_FLAGS ?=
 SP_EXTRA_FLAGS = -Wno-universal-initializer
 
+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
+SANITIZE_LEAK =
+
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.
 # Setting it to 0 will feed all files in a single spatch invocation.

@@ -1265,6 +1268,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
 endif
 ifneq ($(filter leak,$(SANITIZERS)),)
 BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS
+SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN

@@ -2812,6 +2816,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
 	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+

index e3137d638ee..fbdcb926b3a 100755
@@ -21,7 +21,7 @@ test_expect_success POSIXPERM,SANITY 'write-tree should notice unwritable reposi
 	test_must_fail git write-tree
 '
 
-test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' '
 	test_when_finished "chmod 775 .git/objects .git/objects/??" &&
 	chmod a-w .git/objects .git/objects/?? &&
 	test_must_fail git commit -m second

index abcfbed6d61..4ab18914a3d 100644
@@ -1533,6 +1533,7 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON
 test -n "$USE_LIBPCRE2" && test_set_prereq PCRE
 test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
+test -n "$SANITIZE_LEAK" && test_set_prereq SANITIZE_LEAK
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
 then

2.33.0.818.gd2ef2916285

Refactor an "if" statement for "linux-gcc" and "osx-gcc" to a "case" statement in preparation for another case being added to them.

---
 ci/lib.sh | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

index 476c3f369f5..33b9777ab7e 100755
@@ -184,13 +184,15 @@ export SKIP_DASHED_BUILT_INS=YesPlease
 
 case "$jobname" in
 linux-clang|linux-gcc)
-	if [ "$jobname" = linux-gcc ]
-	then
+	case "$jobname" in
+	linux-gcc)
 		export CC=gcc-8
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
-	else
+		;;
+	*)
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
-	fi
+		;;
+	esac
 
 	export GIT_TEST_HTTPD=true
 

@@ -207,13 +209,15 @@ linux-clang|linux-gcc)
 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 osx-clang|osx-gcc)
-	if [ "$jobname" = osx-gcc ]
-	then
+	case "$jobname" in
+	osx-gcc)
 		export CC=gcc-9
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
-	else
+		;;
+	*)
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)"
-	fi
+		;;
+	esac
 
 	# t9810 occasionally fails on Travis CI OS X
 	# t9816 occasionally fails with "TAP out of sequence errors" on

2.33.0.818.gd2ef2916285

While git can be compiled with SANITIZE=leak we have not run regression tests under that mode, memory leaks have only been fixed as one-offs without structured regression testing.

This change add CI testing for it. We'll now build with GCC under Linux and test t000[04]*.sh with SANITIZE=leak, and likewise with GCC on OSX. The new jobs are called "linux-SANITIZE=leak" and "osx-SANITIZE=leak".

The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test mode. When running in that mode, we'll assert that we were compiled with SANITIZE=leak, and then skip all tests except those that we've opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing test-lib.sh (see discussion in t/README).

The tests using the "TEST_PASSES_SANITIZE_LEAK=true" setting can in turn make use of the "SANITIZE_LEAK" prerequisite, should they wish to selectively skip tests even under "GIT_TEST_PASSING_SANITIZE_LEAK=true". In a preceding commit we started doing this in "t0004-unwritable.sh" under SANITIZE=leak, now it'll combine nicely with "GIT_TEST_PASSING_SANITIZE_LEAK=true".

Now tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:

    $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0001-init.sh
    1..0 # SKIP skip all tests in t0001 under SANITIZE=leak, TEST_PASSES_SANITIZE_LEAK not set

The intent is to add more TEST_PASSES_SANITIZE_LEAK=true annotations as follow-up change, but let's start small to begin with.

It would also be possible to implement a more lightweight version of this by only relying on setting "LSAN_OPTIONS". See <YS9OT/[email protected]>[1] and <[email protected]>[2] for a discussion of that. I've opted for this approach of adding a GIT_TEST_* mode instead because it's consistent with how we handle other special test modes.

Being able to add a "!SANITIZE_LEAK" prerequisite and calling "test_done" early if it isn't satisfied also means that we can more incrementally add regression tests without being forced to fix widespread and hard-to-fix leaks at the same time.

We have tests that do simple checking of some tool we're interested in, but later on in the script might be stressing trace2, or common sources of leaks like "git log" in combination with the tool (e.g. the commit-graph tests). To be clear having a prerequisite could also be accomplished by using "LSAN_OPTIONS" directly.

On the topi of "LSAN_OPTIONS": It would be nice to have a mode to aggregate all failures in our various scripts, see [2] for a start at doing that which sets "log_path" in "LSAN_OPTIONS". I've punted on that for now, it can be added later, and that proposed patch is also hindered by us wanting to test e.g. test-tool leaks (and by proxy, any API leaks they uncover), not just the "common-main.c" entry point.

As of writing this we've got major regressions between master..seen, i.e. the t000*.sh tests and more fixed since 31f9acf9ce2 (Merge branch 'ah/plugleaks', 2021-08-04) have regressed recently.

See the discussion at <[email protected]> about the lack of this sort of test mode, and 0e5bba53af (add UNLEAK annotation for reducing leak false positives, 2017-09-08) for the initial addition of SANITIZE=leak.

See also 09595ab381 (Merge branch 'jk/leak-checkers', 2017-09-19), 7782066f67 (Merge branch 'jk/apache-lsan', 2019-05-19) and the recent 936e58851a (Merge branch 'ah/plugleaks', 2021-05-07) for some of the past history of "one-off" SANITIZE=leak (and more) fixes.

The reason for using gcc on OSX over the clang default is because it'll currently fail to build with:

clang: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin19.6.0'

If that's sorted out in the future we might want to run that job with "clang" merely to make use of the default, and also to add some compiler variance into the mix. Both use the "AddressSanitizerLeakSanitizer" library[3], so in they shouldn't be have differently under GCC or clang.

1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
2. https://lore.kernel.org/git/YS9OT%[email protected]/
3. https://lore.kernel.org/git/[email protected]/

---
 .github/workflows/main.yml |  6 ++++++
 ci/install-dependencies.sh |  6 +++---
 ci/lib.sh                  | 15 +++++++++++----
 ci/run-build-and-tests.sh  |  2 +-
 t/README                   |  7 +++++++
 t/t0000-basic.sh           |  1 +
 t/t0004-unwritable.sh      |  1 +
 t/test-lib.sh              | 20 ++++++++++++++++++++
 8 files changed, 50 insertions(+), 8 deletions(-)

index 68596f25927..b41572293c9 100644
@@ -232,6 +232,12 @@ jobs:
           - jobname: linux-gcc-default
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-SANITIZE=leak
+            cc: gcc
+            pool: ubuntu-latest
+          - jobname: osx-SANITIZE=leak
+            cc: gcc
+            pool: macos-latest
     env:
       CC: ${{matrix.vector.cc}}
       jobname: ${{matrix.vector.jobname}}

index 5772081b6e5..a89e72c1438 100755
@@ -12,13 +12,13 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 case "$jobname" in
-linux-clang|linux-gcc)
+linux-clang|linux-gcc|linux-SANITIZE=leak)
 	sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test"
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
 		$UBUNTU_COMMON_PKGS
 	case "$jobname" in
-	linux-gcc)
+	linux-gcc|linux-SANITIZE=leak)
 		sudo apt-get -q -y install gcc-8
 		;;
 	esac

@@ -37,7 +37,7 @@ linux-clang|linux-gcc)
 		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
 	popd
 	;;
-osx-clang|osx-gcc)
+osx-clang|osx-gcc|osx-SANITIZE=leak)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time

index 33b9777ab7e..36b7c0d3020 100755
@@ -183,9 +183,9 @@ export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
 case "$jobname" in
-linux-clang|linux-gcc)
+linux-clang|linux-gcc|linux-SANITIZE=leak)
 	case "$jobname" in
-	linux-gcc)
+	linux-gcc|linux-SANITIZE=leak)
 		export CC=gcc-8
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
 		;;

@@ -208,9 +208,9 @@ linux-clang|linux-gcc)
 	GIT_LFS_PATH="$HOME/custom/git-lfs"
 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
-osx-clang|osx-gcc)
+osx-clang|osx-gcc|osx-SANITIZE=leak)
 	case "$jobname" in
-	osx-gcc)
+	osx-gcc|osx-SANITIZE=leak)
 		export CC=gcc-9
 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
 		;;

@@ -237,4 +237,11 @@ linux-musl)
 	;;
 esac
 
+case "$jobname" in
+linux-SANITIZE=leak|osx-SANITIZE=leak)
+	export SANITIZE=leak
+	export GIT_TEST_PASSING_SANITIZE_LEAK=true
+	;;
+esac
+
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"

index 3ce81ffee94..4133239fc36 100755
@@ -12,7 +12,7 @@ esac
 
 make
 case "$jobname" in
-linux-gcc)
+linux-gcc|linux-SANITIZE=leak)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 	make test
 	export GIT_TEST_SPLIT_INDEX=yes

index 9e701223020..4864f208c8a 100644
@@ -366,6 +366,13 @@ excluded as so much relies on it, but this might change in the future.
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
 
+GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> when compiled with
+SANITIZE=leak will run only those tests that have whitelisted
+themselves as passing with no memory leaks. Tests can be whitelisted
+by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing
+"test-lib.sh" itself at the top of the test script. This test mode is
+used by the "linux-SANITIZE=leak" CI target.
+
 GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
 default to n.
 

index cb87768513c..54318af3861 100755
@@ -18,6 +18,7 @@ swapping compression and hashing order, the person who is making the
 modification *should* take notice and update the test vectors here.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 try_local_xy () {

index fbdcb926b3a..37d68ef03be 100755
@@ -2,6 +2,7 @@
 
 test_description='detect unwritable repository and fail correctly'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

index 4ab18914a3d..3b7acfec23b 100644
@@ -1379,6 +1379,26 @@ then
 	test_done
 fi
 
+# skip non-whitelisted tests when compiled with SANITIZE=leak
+if test -n "$SANITIZE_LEAK"
+then
+	if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+	then
+		# We need to see it in "git env--helper" (via
+		# test_bool_env)
+		export TEST_PASSES_SANITIZE_LEAK
+
+		if ! test_bool_env TEST_PASSES_SANITIZE_LEAK false
+		then
+			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+			test_done
+		fi
+	fi
+elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+then
+	error "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak"
+fi
+
 # Last-minute variable setup
 HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"

2.33.0.818.gd2ef2916285

s/topi/topic/

The patch looks OK to me. There are a bunch of typos/nits in the commit message which made it a little harder to read. I don't care _that_ much, but there's one inaccuracy I wanted to point out, and the others are along for the ride. :)

Funky comma placement. Maybe:

While git can be compiled with SANITIZE=leak, we have not run regression tests under that mode. Memory leaks have only been fixed as one-offs without structured regression testing.

s/add/adds/

A matter of taste, but I find the "linux-SANITIZE=leak" a little funny to read because of the mixed-caps and punctuation. Just linux-leaks or something is descriptive enough. Pure bikeshedding, of course.

I think test-tool does actually use common-main.c, so we'd be covered there, too. That said, I'm perfectly fine to leave this for now (or perhaps never; if we can get the whole suite passing with leak-checking on, then aggregating the many leak reports without having test failures will become a moot point).

OK, I think we should proceed with this series/approach, then. The question of friction when CI fails is an open one, but we won't know until we have more data. So let's see what happens. :)

The patches themselves look fine to me, though I had a few nits on the third commit message.

-Peff