tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

PATCH In review Phillip Wood via GitGitGadget 3 participants 5 messages Apr 9, 2022 – Apr 12, 2022 Kbuild

lore

Share & tools

lore

Phillip Wood via GitGitGadget

author

2022-04-09 12:28 UTC #0 lore raw

PATCH 1/1 tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

From: Phillip Wood <[email protected]>

As the address sanitizer checks for a superset of the issues detected by setting MALLOC_CHECK_ (which tries to detect things like double frees and off-by-one errors) there is no need to set the latter when compiling with -fsanitize=address.

This fixes a regression introduced by 131b94a10a ("test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34", 2022-03-04) which causes all the tests to fail with the message

ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

when git is compiled with SANITIZE=address on systems with glibc >= 2.34. I have tested SANITIZE=leak and SANITIZE=undefined and they do not suffer from this regression so the fix in this patch should be sufficient.

Signed-off-by Phillip Wood <[email protected]>

---
tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

I'm submitting this now as it fixes a regression introduced in the current cycle. Having said that there is an easy workaround (once one has discovered GIT_TEST_NO_MALLOC_CHECK) so I'd be happy to wait until the start of the next cycle given I've just missed -rc1.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1210%2Fphillipwood%2Fwip%2Ftest-malloc-asan-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1210/phillipwood/wip/test-malloc-asan-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1210

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

Makefile +4 -1

index 91738485626..76d187991d2 100644
@@ -1267,8 +1267,9 @@ PTHREAD_CFLAGS =
 SPARSE_FLAGS ?= -std=gnu99
 SP_EXTRA_FLAGS = -Wno-universal-initializer
 
-# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets
 SANITIZE_LEAK =
+SANITIZE_ADDRESS =
 
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.

@@ -1314,6 +1315,7 @@ SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN
+SANITIZE_ADDRESS = YesCompiledWithIt
 endif
 endif

@@ -2861,6 +2863,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
 	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
+	@echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+
 	@echo X=\'$(X)\' >>$@+
 ifdef FSMONITOR_DAEMON_BACKEND
 	@echo FSMONITOR_DAEMON_BACKEND=\''$(subst ','\'',$(subst ','\'',$(FSMONITOR_DAEMON_BACKEND)))'\' >>$@+

t/test-lib.sh +3 -2

index 531cef097db..f09e8f3efce 100644
@@ -535,9 +535,10 @@ case $GIT_TEST_FSYNC in
 	;;
 esac
 
-# Add libc MALLOC and MALLOC_PERTURB test
-# only if we are not executing the test with valgrind
+# Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
+# the test with valgrind and have not compiled with SANITIZE=address.
 if test -n "$valgrind" ||
+   test -n "$SANITIZE_ADDRESS" ||
    test -n "$TEST_NO_MALLOC_CHECK"
 then
 	setup_malloc_check () {

base-commit: faa21c10d44184f616d391c158dcbb13b9c72ef3

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-04-11 19:11 UTC #1 lore raw

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>
As the address sanitizer checks for a superset of the issues detected by setting MALLOC_CHECK_ (which tries to detect things like double frees and off-by-one errors) there is no need to set the latter when compiling with -fsanitize=address.

Very good idea.

Quoted 4 lines

I'm submitting this now as it fixes a regression introduced in the current cycle. Having said that there is an easy workaround (once one has discovered GIT_TEST_NO_MALLOC_CHECK) so I'd be happy to wait until the start of the next cycle given I've just missed -rc1.

Yeah, if this patch were broken, we'd be in worse place than we currently are, so I'd rather not fast-track it. I will queue it in 'seen' and possibly merge to 'next' as it is a good idea to avoid using both at the same time, though.

Thanks.

Ævar Arnfjörð Bjarmason

2022-04-11 21:57 UTC #2 lore raw

In reply to Phillip Wood via GitGitGadget

Quoted 20 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>
As the address sanitizer checks for a superset of the issues detected by setting MALLOC_CHECK_ (which tries to detect things like double frees and off-by-one errors) there is no need to set the latter when compiling with -fsanitize=address.
This fixes a regression introduced by 131b94a10a ("test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34", 2022-03-04) which causes all the tests to fail with the message
ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.
when git is compiled with SANITIZE=address on systems with glibc >= 2.34. I have tested SANITIZE=leak and SANITIZE=undefined and they do not suffer from this regression so the fix in this patch should be sufficient.

Quoted 8 lines

Signed-off-by Phillip Wood <[email protected]>
---
tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK
I'm submitting this now as it fixes a regression introduced in the current cycle. Having said that there is an easy workaround (once one has discovered GIT_TEST_NO_MALLOC_CHECK) so I'd be happy to wait until the start of the next cycle given I've just missed -rc1.

I wonder why we have to justify that we'll only turn on TEST_NO_MALLOC_CHECK if it's SANITIZE=address.

I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof to just say that these analysis options are mutually exclusive by default?

That would have the bonus of e.g. making SANITIZE=leak faster, it's already slow enough without the extra help of glibc's instrumentation.

Quoted 32 lines

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

Makefile +3 -1

index 91738485626..76d187991d2 100644
@@ -1267,8 +1267,9 @@ PTHREAD_CFLAGS =
 SPARSE_FLAGS ?= -std=gnu99
 SP_EXTRA_FLAGS = -Wno-universal-initializer
 
-# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
+# For informing GIT-BUILD-OPTIONS of the SANITIZE=leak,address targets
 SANITIZE_LEAK =
+SANITIZE_ADDRESS =
 
 # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
 # usually result in less CPU usage at the cost of higher peak memory.

@@ -1314,6 +1315,7 @@ SANITIZE_LEAK = YesCompiledWithIt
 endif
 ifneq ($(filter address,$(SANITIZERS)),)
 NO_REGEX = NeededForASAN
+SANITIZE_ADDRESS = YesCompiledWithIt
 endif
 endif

Makefile GIT-BUILD-OPTIONS: FORCE · line 2866

Quoted 4 lines

Makefile +1 -0

@@ -2861,6 +2863,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
 	@echo SANITIZE_LEAK=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_LEAK)))'\' >>$@+
+	@echo SANITIZE_ADDRESS=\''$(subst ','\'',$(subst ','\'',$(SANITIZE_ADDRESS)))'\' >>$@+

Then this could just add SANITIZERS=$(SANITIZERS), we still need SANITIZE_LEAK as we care about that specifically, but This mostly sounds sensible, but for this:

Makefile GIT-BUILD-OPTIONS: FORCE

Quoted 9 lines

Makefile +3 -2

-# Add libc MALLOC and MALLOC_PERTURB test
-# only if we are not executing the test with valgrind
+# Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
+# the test with valgrind and have not compiled with SANITIZE=address.
 if test -n "$valgrind" ||
+   test -n "$SANITIZE_ADDRESS" ||
    test -n "$TEST_NO_MALLOC_CHECK"
 then
 	setup_malloc_check () {

We could check $SANITIZERS here instead.

Junio C Hamano

2022-04-11 23:07 UTC #3 lore raw

Quoted 6 lines · Ævar Arnfjörð Bjarmason

I wonder why we have to justify that we'll only turn on TEST_NO_MALLOC_CHECK if it's SANITIZE=address.
I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof to just say that these analysis options are mutually exclusive by default?

Given that the SANITIZE mechanism itself allows more than one to be requested at the same time, it is unclear to me why other checks like undefined needs to exclude checks done by other mechanisms like MALLOC_CHECK_ by default. If I correctly read under-the-three-dash commentary Phillip wrote, it's not like that use of MALLOC_CHECK_ inherently interferes with the way SANITIZE=undefined wants to work, no?

Ævar Arnfjörð Bjarmason

2022-04-12 10:01 UTC #4 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Ævar Arnfjörð Bjarmason
I wonder why we have to justify that we'll only turn on TEST_NO_MALLOC_CHECK if it's SANITIZE=address.
I.e. we also have SANITIZE=undefined, wouldn't it be more future-proof to just say that these analysis options are mutually exclusive by default?

Quoted 7 lines

Given that the SANITIZE mechanism itself allows more than one to be requested at the same time, it is unclear to me why other checks like undefined needs to exclude checks done by other mechanisms like MALLOC_CHECK_ by default. If I correctly read under-the-three-dash commentary Phillip wrote, it's not like that use of MALLOC_CHECK_ inherently interferes with the way SANITIZE=undefined wants to work, no?

Because:

* It makes it slower, and part of the utility of these checks is that they run in a timely fashion.

* We add these glibc checks because we'd like to catch malloc()/free() issues, and run the test suite with them by default.

Someone using the SANITIZE=* feature is almost certain to be also doing a "normal" test run, so I don't think we're getting anything extra by combining the two, except needlessly slowing it down.

* Even though SANITIZE=leak,address & valgrind are strictly speaking incompatible with the glibc check, having inject itself into other sanitize modes is surely going to make debugging harder until you discover that we're also injecting the custom malloc.