[PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
    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(-)

diff --git a/Makefile b/Makefile
index 91738485626..76d187991d2 100644
--- a/Makefile
+++ b/Makefile
@@ -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)))'\' >>$@+
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 531cef097db..f09e8f3efce 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.

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

Re: [PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 09 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> ---
>     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.

> 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(-)
>
> diff --git a/Makefile b/Makefile
> index 91738485626..76d187991d2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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)))'\' >>$@+

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:

> -# 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.

Re: [PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Re: [PATCH] tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 11 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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?

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.