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

From: Jeff King <peff@peff.net>
Date: Nov 14, 2024, 01:27 UTC
Message-ID: <20241114012729.GA1148710@coredump.intra.peff.net>
In-Reply-To: <87zfm3iggu.fsf@iotcl.com>

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

Show 17 quoted lines

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

Show 7 quoted lines

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