threads / patch / 58527

patchtest-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK

Subject: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK

## tl;dr

4 messages between Sep 28, 2022 and Sep 29, 2022. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Sep 28, 2022, 10:01 UTC · lore

Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with SANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK method used before glibc 2.34 seems to have been (mostly?) compatible with it, but after 131b94a10a7 e.g. running:

	TEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh
Would report a leak in builtin/commit.c, but this would not:
	TEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh

Since the interaction is clearly breaking the SANITIZE=leak mode, let's mark them as explicitly incompatible.

A related regression for SANITIZE=address was fixed in 067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK, 2022-04-09).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Junio: I think this is worth considering for v2.38.0. We've had this
check since v2.36.0

But 2.34 just recently got migrated to Debian testing (just a few days ago), I suspect other distros are either upgrading to it now, or will soon: https://tracker.debian.org/pkg/glibc;

When I upgraded to it I discovered that all of our tests pass with SANITIZE=leak, i.e. unless TEST_NO_MALLOC_CHECK=1 is provided we completely disable the LeakSanitizer in favor of glibc.

 t/test-lib.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/test-lib.sh +3 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index a65df2fd220..02f438d47ec 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -563,8 +563,10 @@ case $GIT_TEST_FSYNC in
 esac
 
 # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
-# the test with valgrind and have not compiled with SANITIZE=address.
+# the test with valgrind and have not compiled with conflict SANITIZE
+# options.
 if test -n "$valgrind" ||
+   test -n "$SANITIZE_LEAK" ||
    test -n "$SANITIZE_ADDRESS" ||
    test -n "$TEST_NO_MALLOC_CHECK"
 then
-- 
2.38.0.rc1.925.gb61c5ccd7da
Rubén Justo· Sep 28, 2022, 23:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK

On 28/9/22 12:01, Ævar Arnfjörð Bjarmason wrote:
Show 53 quoted lines
> Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of
> MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with
> SANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK
> method used before glibc 2.34 seems to have been (mostly?) compatible
> with it, but after 131b94a10a7 e.g. running:
> 
> 	TEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh
> 
> Would report a leak in builtin/commit.c, but this would not:
> 
> 	TEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh
> 
> Since the interaction is clearly breaking the SANITIZE=leak mode,
> let's mark them as explicitly incompatible.
> 
> A related regression for SANITIZE=address was fixed in
> 067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK,
> 2022-04-09).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> Junio: I think this is worth considering for v2.38.0. We've had this
> check since v2.36.0
> 
> But 2.34 just recently got migrated to Debian testing (just a few days
> ago), I suspect other distros are either upgrading to it now, or will
> soon: https://tracker.debian.org/pkg/glibc;
> 
> When I upgraded to it I discovered that all of our tests pass with
> SANITIZE=leak, i.e. unless TEST_NO_MALLOC_CHECK=1 is provided we
> completely disable the LeakSanitizer in favor of glibc.
> 
>  t/test-lib.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index a65df2fd220..02f438d47ec 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -563,8 +563,10 @@ case $GIT_TEST_FSYNC in
>  esac
>  
>  # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
> -# the test with valgrind and have not compiled with SANITIZE=address.
> +# the test with valgrind and have not compiled with conflict SANITIZE
> +# options.
>  if test -n "$valgrind" ||
> +   test -n "$SANITIZE_LEAK" ||
>     test -n "$SANITIZE_ADDRESS" ||
>     test -n "$TEST_NO_MALLOC_CHECK"
>  then
> 

Thank you! A quick test with this on master shows clearly the leak in ref-filter.c we discussed recently. No need to dig with valgrind. I also found that other case you pointed out, from checkout. I'll reroll with that if you don't mind.

It is nice to have this working.
Thanks.
Un saludo.
Phillip Wood· Sep 29, 2022, 09:09 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK

On 28/09/2022 11:01, Ævar Arnfjörð Bjarmason wrote:
Show 18 quoted lines
> Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of
> MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with
> SANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK
> method used before glibc 2.34 seems to have been (mostly?) compatible
> with it, but after 131b94a10a7 e.g. running:
> 
> 	TEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh
> 
> Would report a leak in builtin/commit.c, but this would not:
> 
> 	TEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh
> 
> Since the interaction is clearly breaking the SANITIZE=leak mode,
> let's mark them as explicitly incompatible.
> 
> A related regression for SANITIZE=address was fixed in
> 067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK,
> 2022-04-09).
Oh so the LD_PRELOAD breaks both sanitizers but only one of them complains
Show 8 quoted lines
>   # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
> -# the test with valgrind and have not compiled with SANITIZE=address.
> +# the test with valgrind and have not compiled with conflict SANITIZE
> +# options.
>   if test -n "$valgrind" ||
> +   test -n "$SANITIZE_LEAK" ||
>      test -n "$SANITIZE_ADDRESS" ||
>      test -n "$TEST_NO_MALLOC_CHECK"

The indentation is dodgy, also it would be nice to keep these in alphabetical order. Other than that this looks like a sensible fix.

Best Wishes
Phillip
Junio C Hamano· Sep 29, 2022, 15:29 UTC · re: Phillip Wood · lore

Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 13 quoted lines
> Oh so the LD_PRELOAD breaks both sanitizers but only one of them complains
>
>>   # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
>> -# the test with valgrind and have not compiled with SANITIZE=address.
>> +# the test with valgrind and have not compiled with conflict SANITIZE
>> +# options.
>>   if test -n "$valgrind" ||
>> +   test -n "$SANITIZE_LEAK" ||
>>      test -n "$SANITIZE_ADDRESS" ||
>>      test -n "$TEST_NO_MALLOC_CHECK"
>
> The indentation is dodgy, also it would be nice to keep these in
> alphabetical order. Other than that this looks like a sensible fix.
Thanks, both.

Will re-queue with a local fix-up for the indentation. As to the ordering, I usually prefer to have new ones appended to the last unless there are other reasons, and "keep them sorted" is such a reason, so I may do so as well while at it.

← back to recent threads