Re: [PATCH] test-lib: check malloc debug LD_PRELOAD before using

[Toon Claes <toon@iotcl.com>]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] test-lib: check malloc debug LD_PRELOAD before using
>
> This fixes test failures across the suite on glibc platforms that don't
> have libc_malloc_debug.so.0.

As I ran into this issue not so long as well, I'm really supportive of
adding a fix for this.

> We added support for glibc's malloc checking routines long ago in
> a731fa916e (Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test
> suite for detecting heap corruption, 2012-09-14). Back then we didn't
> need to do any checks to see if the platform supported it. We were just
> setting some environment variables which would either enable it or not.
>
> That changed in 131b94a10a (test-lib.sh: Use GLIBC_TUNABLES instead of
> MALLOC_CHECK_ on glibc >= 2.34, 2022-03-04). Now that glibc split this
> out into libc_malloc_debug.so, we have to add it to LD_PRELOAD. We only
> do that when we detect glibc, but it's possible to have glibc but not
> the malloc debug library. In that case LD_PRELOAD will complain to
> stderr, and tests which check for an empty stderr will fail.
>
> You can work around this by setting TEST_NO_MALLOC_CHECK, which disables
> the feature entirely. But it's not obvious to know you need to do that.
> Instead, since this malloc checking is best-effort anyway, let's just
> automatically disable it when the LD_PRELOAD appears not to work. We can
> check it by running something simple that should work (and produce
> nothing on stderr) like "git version".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/test-lib.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index a278181a05..4fe757fe9a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -593,9 +593,12 @@ then
>  	}
>  else
>  	_USE_GLIBC_TUNABLES=
> +	_USE_GLIBC_PRELOAD=
>  	if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
>  	   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
> -	   expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null
> +	   expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null &&
> +	   stderr=$(LD_PRELOAD=$_USE_GLIBC_PRELOAD git version 2>&1 >/dev/null) &&

Can we assume some version of git is in the $PATH here? I see $PATH and
$GIT_EXEC_PATH are only determined at line 1440 and further.

> +	   test -z "$stderr"
>  	then
>  		_USE_GLIBC_TUNABLES=YesPlease

Shall we include a warning in a else clause to inform the user the tests
were started with malloc check, but libc_malloc_debug.so.0 was not found
and they should either install it or run with TEST_NO_MALLOC_CHECK?

>  	fi
> @@ -607,7 +610,7 @@ else
>  		if test -n "$_USE_GLIBC_TUNABLES"
>  		then
>  			g=
> -			LD_PRELOAD="libc_malloc_debug.so.0"
> +			LD_PRELOAD=$_USE_GLIBC_PRELOAD
>  			for t in \
>  				glibc.malloc.check=1 \
>  				glibc.malloc.perturb=165
> -- 
> 2.47.0.495.g1253739cc1

I've tested this patch with and without having glibc-utils installed, in
combination of having TEST_NO_MALLOC_CHECK set/unset and seems to work
like a charm.


--
Toon