test-lib: check malloc debug LD_PRELOAD before using

Commit Message

Jeff King Nov. 11, 2024, 7:01 a.m. UTC

On Mon, Nov 11, 2024 at 12:11:46PM +0900, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > I don't offhand know of a good portable way to ask the system about
> > available libraries. But I guess just doing something like:
> >
> >   err=$(LD_PRELOAD=libc_malloc.so.0 git version 2>&1 >/dev/null)
> >   if test -z "$err"
> >   then
> > 	...seemed to work...
> >   fi
> >
> > would do it?
> 
> I do not necessarily view it as "asking the system about available
> libraries"; we are checking if we can sensibly run things with this
> set to LD_PRELOAD.  And presumably the answer was "no" in the
> original report, so it is a very direct way to ensure that we are
> setting it to a sensible value.  I like it.

Yeah, I agree that is a better way to think about it; it is more
directly asking what we want to know. So here it is as an actual patch.

> The above did not work for me until I did "s/malloc/&_debug/" on the
> command line.

Oops, yes. I should have said "not tested". ;) On the other hand,
writing the wrong name is an easy way to test the failure mode. I pulled
it out into a variable in the patch below so we only have to write it
once.

I tested before and after with a typo'd version of the library name and
it seems to work. But it would be great to get confirmation from Usman
that this fixes the problem.

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

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 <[e-mail]>
---
 t/test-lib.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Toon Claes Nov. 13, 2024, 10:19 a.m. UTC | #1

Jeff King <[e-mail]> 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 <[e-mail]>
> ---
>  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

Jeff King Nov. 14, 2024, 1:27 a.m. UTC | #2

On Wed, Nov 13, 2024 at 11:19:13AM +0100, Toon Claes wrote:

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

Hmm, good question. This is after the "you do not seem to have built
git" check, so I thought we were OK. But of course that one is using:

  "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X"

So the check is probably running some system "git" and not the built
one. That mostly works out anyway since the real variable there is the
LD_PRELOAD, not the specific version of git. And that's why testing it
worked for me. But on a system without an existing git in the $PATH at
all, it would disable the preload, even though it would work fine.

So some possible fixes are:

  - use a more complete path, as the earlier check does

  - delay the malloc-debug checking until after we've set up the $PATH

  - use a different program. We care about the preload working, so:

      LD_PRELOAD=$_USE_GLIBC_PRELOAD ls

    would mostly work the same. Though I suppose if you want to get
    really crazy, it's possible that "ls" might not be linked in the
    same way as our built git (e.g., it could be statically linked
    against a different libc).

So I guess just moving it is probably the least-bad option.

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

I'm not sure. It is optional, and many systems will happily run without
it. Just identifying glibc ones that happen not to have the debug
library installed seems weird when, say, Windows or FreeBSD similarly
run without it. And we'd be forcing the user to set TEST_NO_MALLOC_CHECK
unless they want to be spammed with the warning from every single test
script that is run.

At that point we should almost just revert my patch and let it fail when
the preload doesn't work (the only advantage is that we could produce a
more useful message).

-Peff

Patch

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=libc_malloc_debug.so.0
 	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) &&
+	   test -z "$stderr"
 	then
 		_USE_GLIBC_TUNABLES=YesPlease
 	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