Re: [PATCH v2 1/4] tests: add a test mode for SANITIZE=leak, run it in CI

On Wed, Jul 14 2021, Andrzej Hunt wrote:

On 14/07/2021 19:23, Ævar Arnfjörð Bjarmason wrote:

quoted

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/87czsv2idy.fsf@evledraar.gmail.com/ (local)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
  .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(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 73856bafc9..752fe187f9 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml

@@ -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

Is there any advantage to running leak checking with both gcc and
clang? My understanding is that you end up using the same sanitiser 
implementation under the hood - I can't remember if using a different
compiler actually helps find different leaks though.

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

My other question is: if we are adding a new job - should it really be
just a leak checking job? Leak checking is just a subset of ASAN 
(Address Sanitizer). And as discussed at [1] it's possible to run ASAN
and UBSAN (Undefined Behaviour Sanitizer) in the same build. I feel
like it's much more useful to first add a combined ASAN+UBSAN job,
followed by enabling leak-checking as part of ASAN in those jobs for
known leak-free tests - as opposed to only adding leak checking. We
currently disable Leak checking for ASAN here [2], but that could be
made conditional on the test ID (i.e. check an allowlist to enable
leak checking for some tests)?

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.

I think it's worth focusing on ASAN+UBSAN first because they tend to
find more impactful issues (e.g. buffer overflows, and other real
bugs) - whereas leaks... are ugly, but leaks in git don't actually
have much user impact?

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.

[1]
https://lore.kernel.org/git/YMI%2Fg1sHxJgb8%2FYD@coredump.intra.peff.net/ (local)

[2] https://git.kernel.org/pub/scm/git/git.git/tree/t/test-lib.sh#n44

quoted

      env:
        CC: ${{matrix.vector.cc}}
        jobname: ${{matrix.vector.jobname}}

diff --git a/Makefile b/Makefile
index 502e0c9a81..d4cad5136f 100644
--- a/Makefile
+++ b/Makefile

@@ -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)))'\' >>$@+

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 67852d0d37..8ac72d7246 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh

@@ -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)

How about `linux-clang*|linux-gcc*)` here and below?

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

quoted

  	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

diff --git a/ci/lib.sh b/ci/lib.sh
index 476c3f369f..bb02b5abf4 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh

@@ -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
+

Have you considered doing this in the yaml job configuration instead?
It's possible to set env-vars in yaml, although it will require some 
careful tweaking - here's an example where I'm setting different
values for SANITIZE depending on job (you'd probably just have to set
it to empty for the non leak-checking jobs):

https://github.com/ahunt/git/blob/master/.github/workflows/ahunt-sync-next2.yml#L51-L69

That does make the yaml more complex, but I think it's worth it to
reduce the amount of special-casing elsewhere (and is also worth it if 
we ever add other sanitisers)?

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.