{"thread":{"id":"58527","subject":"[PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK","startedAt":"2022-09-28T10:03:01Z","lastAt":"2022-09-29T15:29:29Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Rubén Justo","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463839","messageId":"patch-1.1-e31681731b7-20220928T095041Z-avarab@gmail.com","threadId":"58527","inReplyTo":null,"subject":"[PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T10:01:54Z","receivedAt":"2022-09-28T10:03:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of\nMALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with\nSANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK\nmethod used before glibc 2.34 seems to have been (mostly?) compatible\nwith it, but after 131b94a10a7 e.g. running:\n\n\tTEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh\n\nWould report a leak in builtin/commit.c, but this would not:\n\n\tTEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh\n\nSince the interaction is clearly breaking the SANITIZE=leak mode,\nlet's mark them as explicitly incompatible.\n\nA related regression for SANITIZE=address was fixed in\n067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK,\n2022-04-09).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nJunio: I think this is worth considering for v2.38.0. We've had this\ncheck since v2.36.0\n\nBut 2.34 just recently got migrated to Debian testing (just a few days\nago), I suspect other distros are either upgrading to it now, or will\nsoon: https://tracker.debian.org/pkg/glibc;\n\nWhen I upgraded to it I discovered that all of our tests pass with\nSANITIZE=leak, i.e. unless TEST_NO_MALLOC_CHECK=1 is provided we\ncompletely disable the LeakSanitizer in favor of glibc.\n\n t/test-lib.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex a65df2fd220..02f438d47ec 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -563,8 +563,10 @@ case $GIT_TEST_FSYNC in\n esac\n \n # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing\n-# the test with valgrind and have not compiled with SANITIZE=address.\n+# the test with valgrind and have not compiled with conflict SANITIZE\n+# options.\n if test -n \"$valgrind\" ||\n+   test -n \"$SANITIZE_LEAK\" ||\n    test -n \"$SANITIZE_ADDRESS\" ||\n    test -n \"$TEST_NO_MALLOC_CHECK\"\n then\n-- \n2.38.0.rc1.925.gb61c5ccd7da\n\n"},{"id":"463887","messageId":"7730c25c-5f10-b1a8-f1c3-cdeaa712ef05@gmail.com","threadId":"58527","inReplyTo":"patch-1.1-e31681731b7-20220928T095041Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-28T23:20:41Z","receivedAt":"2022-09-28T23:21:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 28/9/22 12:01, Ævar Arnfjörð Bjarmason wrote:\n> Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of\n> MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with\n> SANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK\n> method used before glibc 2.34 seems to have been (mostly?) compatible\n> with it, but after 131b94a10a7 e.g. running:\n> \n> \tTEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh\n> \n> Would report a leak in builtin/commit.c, but this would not:\n> \n> \tTEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh\n> \n> Since the interaction is clearly breaking the SANITIZE=leak mode,\n> let's mark them as explicitly incompatible.\n> \n> A related regression for SANITIZE=address was fixed in\n> 067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK,\n> 2022-04-09).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> Junio: I think this is worth considering for v2.38.0. We've had this\n> check since v2.36.0\n> \n> But 2.34 just recently got migrated to Debian testing (just a few days\n> ago), I suspect other distros are either upgrading to it now, or will\n> soon: https://tracker.debian.org/pkg/glibc;\n> \n> When I upgraded to it I discovered that all of our tests pass with\n> SANITIZE=leak, i.e. unless TEST_NO_MALLOC_CHECK=1 is provided we\n> completely disable the LeakSanitizer in favor of glibc.\n> \n>  t/test-lib.sh | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index a65df2fd220..02f438d47ec 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -563,8 +563,10 @@ case $GIT_TEST_FSYNC in\n>  esac\n>  \n>  # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing\n> -# the test with valgrind and have not compiled with SANITIZE=address.\n> +# the test with valgrind and have not compiled with conflict SANITIZE\n> +# options.\n>  if test -n \"$valgrind\" ||\n> +   test -n \"$SANITIZE_LEAK\" ||\n>     test -n \"$SANITIZE_ADDRESS\" ||\n>     test -n \"$TEST_NO_MALLOC_CHECK\"\n>  then\n> \n\nThank you! A quick test with this on master shows clearly the leak in ref-filter.c\nwe discussed recently.  No need to dig with valgrind.  I also found that other case\nyou pointed out, from checkout. I'll reroll with that if you don't mind.\n\nIt is nice to have this working.\n\nThanks.\n\nUn saludo.\n"},{"id":"463898","messageId":"a013246a-777c-acf8-d2c1-3bf9b78aa9e8@dunelm.org.uk","threadId":"58527","inReplyTo":"patch-1.1-e31681731b7-20220928T095041Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-29T09:09:58Z","receivedAt":"2022-09-29T09:11:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/09/2022 11:01, Ævar Arnfjörð Bjarmason wrote:\n> Since 131b94a10a7 (test-lib.sh: Use GLIBC_TUNABLES instead of\n> MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04) compiling with\n> SANITIZE=leak has missed reporting some leaks. The old MALLOC_CHECK\n> method used before glibc 2.34 seems to have been (mostly?) compatible\n> with it, but after 131b94a10a7 e.g. running:\n> \n> \tTEST_NO_MALLOC_CHECK=1 make SANITIZE=leak test T=t6437-submodule-merge.sh\n> \n> Would report a leak in builtin/commit.c, but this would not:\n> \n> \tTEST_NO_MALLOC_CHECK= make SANITIZE=leak test T=t6437-submodule-merge.sh\n> \n> Since the interaction is clearly breaking the SANITIZE=leak mode,\n> let's mark them as explicitly incompatible.\n> \n> A related regression for SANITIZE=address was fixed in\n> 067109a5e7d (tests: make SANITIZE=address imply TEST_NO_MALLOC_CHECK,\n> 2022-04-09).\n\nOh so the LD_PRELOAD breaks both sanitizers but only one of them complains\n\n>   # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing\n> -# the test with valgrind and have not compiled with SANITIZE=address.\n> +# the test with valgrind and have not compiled with conflict SANITIZE\n> +# options.\n>   if test -n \"$valgrind\" ||\n> +   test -n \"$SANITIZE_LEAK\" ||\n>      test -n \"$SANITIZE_ADDRESS\" ||\n>      test -n \"$TEST_NO_MALLOC_CHECK\"\n\nThe indentation is dodgy, also it would be nice to keep these in \nalphabetical order. Other than that this looks like a sensible fix.\n\nBest Wishes\n\nPhillip\n"},{"id":"463901","messageId":"xmqqill6p5bq.fsf@gitster.g","threadId":"58527","inReplyTo":"a013246a-777c-acf8-d2c1-3bf9b78aa9e8@dunelm.org.uk","subject":"Re: [PATCH] test-lib: have SANITIZE=leak imply TEST_NO_MALLOC_CHECK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T15:29:13Z","receivedAt":"2022-09-29T15:29:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Oh so the LD_PRELOAD breaks both sanitizers but only one of them complains\n>\n>>   # Add libc MALLOC and MALLOC_PERTURB test only if we are not executing\n>> -# the test with valgrind and have not compiled with SANITIZE=address.\n>> +# the test with valgrind and have not compiled with conflict SANITIZE\n>> +# options.\n>>   if test -n \"$valgrind\" ||\n>> +   test -n \"$SANITIZE_LEAK\" ||\n>>      test -n \"$SANITIZE_ADDRESS\" ||\n>>      test -n \"$TEST_NO_MALLOC_CHECK\"\n>\n> The indentation is dodgy, also it would be nice to keep these in\n> alphabetical order. Other than that this looks like a sensible fix.\n\nThanks, both.\n\nWill re-queue with a local fix-up for the indentation.  As to the\nordering, I usually prefer to have new ones appended to the last\nunless there are other reasons, and \"keep them sorted\" is such a\nreason, so I may do so as well while at it.\n\n\n\n"}]}