[RFC]: Test Were failing on Fedora Linux.

[RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

Hello,

I was trying to build the Git project on Fedora Linux. I just
installed the Fedora.

I followed through the steps in the Submitting a Patch. About 42 tests
were failing.

I noticed that there are some directories created that did not get deleted.

*'trash directory.t0000-basic'
'trash directory.t0001-init'
'trash directory.t0003-attributes'
'trash directory.t0004-unwritable'
'trash directory.t0008-ignores'
'trash directory.t0012-help'
'trash directory.t0017-env-helper'
'trash directory.t0018-advice'
'trash directory.t0020-crlf'
'trash directory.t0021-conversion'
'trash directory.t0027-auto-crlf'
'trash directory.t0040-parse-options'
'trash directory.t0041-usage'
'trash directory.t0061-run-command'
'trash directory.t0070-fundamental'
'trash directory.t0090-cache-tree'
'trash directory.t0202-gettext-perl'
'trash directory.t0203-gettext-setlocale-sanity'
'trash directory.t0210-trace2-normal'
'trash directory.t0300-credentials'
'trash directory.t0301-credential-cache'
'trash directory.t0302-credential-store'
'trash directory.t0303-credential-external*

Any idea of what I am missing ?

Thank you.
Usman.

Re: [RFC]: Test Were failing on Fedora Linux.

[Christian Couder]

On Sat, Nov 9, 2024 at 7:02 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> Hello,
>
> I was trying to build the Git project on Fedora Linux. I just
> installed the Fedora.
>
> I followed through the steps in the Submitting a Patch. About 42 tests
> were failing.

Please tell us how exactly you ran the tests and what was the output
that showed some tests failed.

You can run each failing test with some options like -i and -v to get
more information, like for example:

$ ./t0000-basic.sh -i -v

> I noticed that there are some directories created that did not get deleted.
>
> *'trash directory.t0000-basic'
> 'trash directory.t0001-init'
> 'trash directory.t0003-attributes'
> 'trash directory.t0004-unwritable'
> 'trash directory.t0008-ignores'
> 'trash directory.t0012-help'
> 'trash directory.t0017-env-helper'
> 'trash directory.t0018-advice'
> 'trash directory.t0020-crlf'
> 'trash directory.t0021-conversion'
> 'trash directory.t0027-auto-crlf'
> 'trash directory.t0040-parse-options'
> 'trash directory.t0041-usage'
> 'trash directory.t0061-run-command'
> 'trash directory.t0070-fundamental'
> 'trash directory.t0090-cache-tree'
> 'trash directory.t0202-gettext-perl'
> 'trash directory.t0203-gettext-setlocale-sanity'
> 'trash directory.t0210-trace2-normal'
> 'trash directory.t0300-credentials'
> 'trash directory.t0301-credential-cache'
> 'trash directory.t0302-credential-store'
> 'trash directory.t0303-credential-external*
>
> Any idea of what I am missing ?

When a test fails, the directory where the test was run is not deleted
so that it can be inspected to see what went wrong in the test. So it
is to be expected that these directories were not deleted if the
corresponding tests failed.

Best,
Christian.

Re: [RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

On Sat, Nov 9, 2024 at 3:12 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sat, Nov 9, 2024 at 7:02 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> >
> > Hello,
> >
> > I was trying to build the Git project on Fedora Linux. I just
> > installed the Fedora.
> >
> > I followed through the steps in the Submitting a Patch. About 42 tests
> > were failing.
>
> Please tell us how exactly you ran the tests and what was the output
> that showed some tests failed.
>
> You can run each failing test with some options like -i and -v to get
> more information, like for example:
>
> $ ./t0000-basic.sh -i -v
Hello Christian,

I was using make before. I tried to run the single test as above, the error was
ERROR: ld.so: object 'libc_malloc_debug.so.0' from LD_PRELOAD cannot
be preloaded (cannot open shared object file): ignored
I tried to check this online, but, all the solutions I found were not
resolving it.

I look for libc_malloc_debug.so.0 and it is located in /usr/lib
uniqueusman@fedora:~/git/t$ sudo find / -name "libc_malloc_debug.so.0"
find: ‘/run/user/1000/gvfs’: Permission denied
find: ‘/run/user/1000/doc’: Permission denied
/usr/lib/libc_malloc_debug.so.0

Thank you.
Usman

>
> > I noticed that there are some directories created that did not get deleted.
> >
> > *'trash directory.t0000-basic'
> > 'trash directory.t0001-init'
> > 'trash directory.t0003-attributes'
> > 'trash directory.t0004-unwritable'
> > 'trash directory.t0008-ignores'
> > 'trash directory.t0012-help'
> > 'trash directory.t0017-env-helper'
> > 'trash directory.t0018-advice'
> > 'trash directory.t0020-crlf'
> > 'trash directory.t0021-conversion'
> > 'trash directory.t0027-auto-crlf'
> > 'trash directory.t0040-parse-options'
> > 'trash directory.t0041-usage'
> > 'trash directory.t0061-run-command'
> > 'trash directory.t0070-fundamental'
> > 'trash directory.t0090-cache-tree'
> > 'trash directory.t0202-gettext-perl'
> > 'trash directory.t0203-gettext-setlocale-sanity'
> > 'trash directory.t0210-trace2-normal'
> > 'trash directory.t0300-credentials'
> > 'trash directory.t0301-credential-cache'
> > 'trash directory.t0302-credential-store'
> > 'trash directory.t0303-credential-external*
> >
> > Any idea of what I am missing ?
>
> When a test fails, the directory where the test was run is not deleted
> so that it can be inspected to see what went wrong in the test. So it
> is to be expected that these directories were not deleted if the
> corresponding tests failed.
Ohh, thanks for this.
>
> Best,
> Christian.

Re: [RFC]: Test Were failing on Fedora Linux.

[Christian Couder]

Hi Usman,

On Sat, Nov 9, 2024 at 10:33 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> On Sat, Nov 9, 2024 at 3:12 AM Christian Couder
> <christian.couder@gmail.com> wrote:

> > You can run each failing test with some options like -i and -v to get
> > more information, like for example:
> >
> > $ ./t0000-basic.sh -i -v
>
> I was using make before. I tried to run the single test as above, the error was
> ERROR: ld.so: object 'libc_malloc_debug.so.0' from LD_PRELOAD cannot
> be preloaded (cannot open shared object file): ignored
> I tried to check this online, but, all the solutions I found were not
> resolving it.

First, as a workaround, maybe you can try:

$ TEST_NO_MALLOC_CHECK=1 ./t0000-basic.sh -i -v

to see if things work when disabling malloc checks.

Anyway the issue might be related to how your shared libraries are
installed, so using ldconfig might help:

$ sudo ldconfig

Alternatively you can try replacing the following line in t/test-lib.sh:

                 LD_PRELOAD="libc_malloc_debug.so.0"

with:

                 LD_PRELOAD="/usr/lib/libc_malloc_debug.so.0"

This is a hack but it might help understand what's going on if it works.

> I look for libc_malloc_debug.so.0 and it is located in /usr/lib
> uniqueusman@fedora:~/git/t$ sudo find / -name "libc_malloc_debug.so.0"
> find: ‘/run/user/1000/gvfs’: Permission denied
> find: ‘/run/user/1000/doc’: Permission denied
> /usr/lib/libc_malloc_debug.so.0

Yeah, not sure why it doesn't work while you have it.

Best,
Christian.

Re: [RFC]: Test Were failing on Fedora Linux.

[Andreas Schwab]

On Nov 09 2024, Christian Couder wrote:

> Yeah, not sure why it doesn't work while you have it.

It's probably of the wrong architecture.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

On Sat, Nov 9, 2024 at 9:01 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi Usman,
>
> On Sat, Nov 9, 2024 at 10:33 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> >
> > On Sat, Nov 9, 2024 at 3:12 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
>
> > > You can run each failing test with some options like -i and -v to get
> > > more information, like for example:
> > >
> > > $ ./t0000-basic.sh -i -v
> >
> > I was using make before. I tried to run the single test as above, the error was
> > ERROR: ld.so: object 'libc_malloc_debug.so.0' from LD_PRELOAD cannot
> > be preloaded (cannot open shared object file): ignored
> > I tried to check this online, but, all the solutions I found were not
> > resolving it.
>
> First, as a workaround, maybe you can try:
>
> $ TEST_NO_MALLOC_CHECK=1 ./t0000-basic.sh -i -v
>
> to see if things work when disabling malloc checks.
>
> Anyway the issue might be related to how your shared libraries are
> installed, so using ldconfig might help:
>
> $ sudo ldconfig
>
> Alternatively you can try replacing the following line in t/test-lib.sh:
>
>                  LD_PRELOAD="libc_malloc_debug.so.0"
>
> with:
>
>                  LD_PRELOAD="/usr/lib/libc_malloc_debug.so.0"
>
> This is a hack but it might help understand what's going on if it works.
Hi Christian.

Thanks for this, the problem was due to the architecture, the one
/usr/lib/libc_malloc_debug.so.0 was 32bit,
I had to download the 64bit by downloading glibc-utils.x86_64. I did
not face this when I was using Arch Linux.

Thank you.
Usman.
>
> > I look for libc_malloc_debug.so.0 and it is located in /usr/lib
> > uniqueusman@fedora:~/git/t$ sudo find / -name "libc_malloc_debug.so.0"
> > find: ‘/run/user/1000/gvfs’: Permission denied
> > find: ‘/run/user/1000/doc’: Permission denied
> > /usr/lib/libc_malloc_debug.so.0
>
> Yeah, not sure why it doesn't work while you have it.
>
> Best,
> Christian.

Re: [RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

On Sat, Nov 9, 2024 at 9:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Nov 09 2024, Christian Couder wrote:
>
> > Yeah, not sure why it doesn't work while you have it.
>
> It's probably of the wrong architecture.
Hi Andreas,
Thanks for responding.
It was actually the wrong Architecture. Thank you. Just curious, any
reason why the 32bit was present instead of the
64bit ?, I will normally think the operating system should ship 64bit
by default.

Usman Akinyemi.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Re: [RFC]: Test Were failing on Fedora Linux.

[Todd Zullinger]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

Usman Akinyemi wrote:
> On Sat, Nov 9, 2024 at 9:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Nov 09 2024, Christian Couder wrote:
>>
>>> Yeah, not sure why it doesn't work while you have it.
>>
>> It's probably of the wrong architecture.
> Hi Andreas,
> Thanks for responding.
> It was actually the wrong Architecture. Thank you. Just curious, any
> reason why the 32bit was present instead of the
> 64bit ?, I will normally think the operating system should ship 64bit
> by default.

The 64-bit libc_malloc_debug.so.0 is in /lib64 and was moved
to the glibc-utils package in Fedora 40, with 2c1b0f0 (Move
memory tracing libraries to glibc-utils, 2024-05-15)¹.  The
commit message notes:

    On x86_64, glibc-utils will now only contain the 64-bit
    version of these libraries but still need the 32-bit
    version (in order to support tracing i686 applications).
    Therefore, on i686 the libraries remain in the main
    glibc package.

If you're interested in installing the various dependencies
needed to run the test suite on Fedora, take a look at the
Fedora git package spec file².

The BuildRequires contain a substantial set of dependencies
which enable as many of the tests as practical to run when
building the packages (I believe more tests are run there
than are run in the git project's CI for most runs,
actually :).

See also the %check section of the test suite for some tests
which are skipped and other comments which might be useful.

¹ https://src.fedoraproject.org/rpms/glibc/c/2c1b0f0
² https://src.fedoraproject.org/rpms/git/blob/rawhide/f/git.spec

-- 
Todd

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC]: Test Were failing on Fedora Linux.

[Jeff King]

On Sat, Nov 09, 2024 at 11:05:55AM -0500, Todd Zullinger wrote:

> The 64-bit libc_malloc_debug.so.0 is in /lib64 and was moved
> to the glibc-utils package in Fedora 40, with 2c1b0f0 (Move
> memory tracing libraries to glibc-utils, 2024-05-15)¹.  The
> commit message notes:
> 
>     On x86_64, glibc-utils will now only contain the 64-bit
>     version of these libraries but still need the 32-bit
>     version (in order to support tracing i686 applications).
>     Therefore, on i686 the libraries remain in the main
>     glibc package.
> 
> If you're interested in installing the various dependencies
> needed to run the test suite on Fedora, take a look at the
> Fedora git package spec file².

Hmm. I wonder if our test scripts could be a little more forgiving here.

The glibc malloc debugging stuff has always been turned on by default
since a731fa916e (Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the
test suite for detecting heap corruption, 2012-09-14). Back then the
setup just involved setting some environment variables. If we were on a
system where it didn't exist, it was no big deal. We'd just run without
it.

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 sounds like it's possible to have
glibc but not the malloc debug library. In which case we'll produce
errors (at the very least it seems like ld.so will complain to stderr,
which perhaps is the source of the test failures here).

Can we do a better job of detecting that the library is available?

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 dunno. Maybe this is not a common enough thing to worry
about. It just seems bad for us to make life harder for people running
the tests for an optional thing.

  Side note: The glibc malloc stuff seems a bit more redundant these
  days, since we run with ASan in CI (which I'd expect to catch a
  superset of what the malloc debugging would). There is some value in
  catching things sooner, though (I don't usually do an ASan run on my
  workstation, and of course some people may not use CI at all).

-Peff

Re: [RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

On Sat, Nov 9, 2024 at 11:05 AM Todd Zullinger <tmz@pobox.com> wrote:
>
> Usman Akinyemi wrote:
> > On Sat, Nov 9, 2024 at 9:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >> On Nov 09 2024, Christian Couder wrote:
> >>
> >>> Yeah, not sure why it doesn't work while you have it.
> >>
> >> It's probably of the wrong architecture.
> > Hi Andreas,
> > Thanks for responding.
> > It was actually the wrong Architecture. Thank you. Just curious, any
> > reason why the 32bit was present instead of the
> > 64bit ?, I will normally think the operating system should ship 64bit
> > by default.
>
> The 64-bit libc_malloc_debug.so.0 is in /lib64 and was moved
> to the glibc-utils package in Fedora 40, with 2c1b0f0 (Move
> memory tracing libraries to glibc-utils, 2024-05-15)¹.  The
> commit message notes:
>
>     On x86_64, glibc-utils will now only contain the 64-bit
>     version of these libraries but still need the 32-bit
>     version (in order to support tracing i686 applications).
>     Therefore, on i686 the libraries remain in the main
>     glibc package.
>
> If you're interested in installing the various dependencies
> needed to run the test suite on Fedora, take a look at the
> Fedora git package spec file².
>
> The BuildRequires contain a substantial set of dependencies
> which enable as many of the tests as practical to run when
> building the packages (I believe more tests are run there
> than are run in the git project's CI for most runs,
> actually :).
>
Hi Todd,

Thanks for the explanation,

I really appreciate it.

Usman.
> See also the %check section of the test suite for some tests
> which are skipped and other comments which might be useful.
>
> ¹ https://src.fedoraproject.org/rpms/glibc/c/2c1b0f0
> ² https://src.fedoraproject.org/rpms/git/blob/rawhide/f/git.spec
>
> --
> Todd

Re: [RFC]: Test Were failing on Fedora Linux.

[Usman Akinyemi]

On Sat, Nov 9, 2024 at 2:00 PM Jeff King <peff@peff.net> wrote:
>
> On Sat, Nov 09, 2024 at 11:05:55AM -0500, Todd Zullinger wrote:
>
> > The 64-bit libc_malloc_debug.so.0 is in /lib64 and was moved
> > to the glibc-utils package in Fedora 40, with 2c1b0f0 (Move
> > memory tracing libraries to glibc-utils, 2024-05-15)¹.  The
> > commit message notes:
> >
> >     On x86_64, glibc-utils will now only contain the 64-bit
> >     version of these libraries but still need the 32-bit
> >     version (in order to support tracing i686 applications).
> >     Therefore, on i686 the libraries remain in the main
> >     glibc package.
> >
> > If you're interested in installing the various dependencies
> > needed to run the test suite on Fedora, take a look at the
> > Fedora git package spec file².
>
> Hmm. I wonder if our test scripts could be a little more forgiving here.
>
> The glibc malloc debugging stuff has always been turned on by default
> since a731fa916e (Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the
> test suite for detecting heap corruption, 2012-09-14). Back then the
> setup just involved setting some environment variables. If we were on a
> system where it didn't exist, it was no big deal. We'd just run without
> it.
>
> 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 sounds like it's possible to have
> glibc but not the malloc debug library. In which case we'll produce
> errors (at the very least it seems like ld.so will complain to stderr,
> which perhaps is the source of the test failures here).
>
> Can we do a better job of detecting that the library is available?
>
> 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 dunno. Maybe this is not a common enough thing to worry
> about. It just seems bad for us to make life harder for people running
> the tests for an optional thing.
>
>   Side note: The glibc malloc stuff seems a bit more redundant these
>   days, since we run with ASan in CI (which I'd expect to catch a
>   superset of what the malloc debugging would). There is some value in
>   catching things sooner, though (I don't usually do an ASan run on my
>   workstation, and of course some people may not use CI at all).
>
Hi Peff,

This is definitely a good suggestion. I think at atleast we should
have this stated in INSTALL instructions.

What do you think ?

Usman

> -Peff

Re: [RFC]: Test Were failing on Fedora Linux.

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

The above did not work for me until I did "s/malloc/&_debug/" on the
command line.  At this point in the start-up sequence in the test
framework, we should be able to run "git" from the PATH just fine,
so it would be a good way to check if we can trigger the malloc
check with the way how we expect to be able to trigger it.

Thanks.

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

[Jeff King]

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

> Jeff King <peff@peff.net> 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 <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=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
-- 
2.47.0.495.g1253739cc1

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

[Toon Claes]

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

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

[Jeff King]

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 2/1] test-lib: move malloc-debug setup after $PATH setup

[Jeff King]

Originally, the conditional definition of the setup/teardown functions
for malloc checking could be run at any time, because they depended only
on command-line options and the system getconf function.

But since 02d900361c (test-lib: check malloc debug LD_PRELOAD before
using, 2024-11-11), we probe the system by running "git version". Since
this code runs before we've set $PATH to point to the version of Git we
intend to test, we actually run the system version of git.

This mostly works, since what we really care about is whether the
LD_PRELOAD works, and it should work the same with any program. But
there are some corner cases:

  1. You might not have a system git at all, in which case the preload
     will appear to fail, even though it could work with the actual
     built version of git.

  2. Your system git could be linked in a different way. For example, if
     it was built statically, then it will ignore LD_PRELOAD entirely,
     and we might assume that the preload works, even though it might
     not when used with a dynamic build.

We could give a more complete path to the version of Git we intend to
test, but features like GIT_TEST_INSTALLED make that not entirely
trivial. So instead, let's just bump the setup until after we've set up
the $PATH. There's no need for us to do it early, as long as it is done
before the first test runs.

Reported-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Tested by removing "git" from my $PATH and checking whether it was used.
I didn't do a full test with the static version, but I confirmed that:

  gcc -static foo.c
  LD_PRELOAD=bogus.so ./a.out

does not complain.

 t/test-lib.sh | 100 +++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4fe757fe9a..6c60e8adae 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -577,56 +577,6 @@ 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 conflict SANITIZE
-# options.
-if test -n "$valgrind" ||
-   test -n "$SANITIZE_ADDRESS" ||
-   test -n "$SANITIZE_LEAK" ||
-   test -n "$TEST_NO_MALLOC_CHECK"
-then
-	setup_malloc_check () {
-		: nothing
-	}
-	teardown_malloc_check () {
-		: nothing
-	}
-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 &&
-	   stderr=$(LD_PRELOAD=$_USE_GLIBC_PRELOAD git version 2>&1 >/dev/null) &&
-	   test -z "$stderr"
-	then
-		_USE_GLIBC_TUNABLES=YesPlease
-	fi
-	setup_malloc_check () {
-		local g
-		local t
-		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
-		export MALLOC_CHECK_ MALLOC_PERTURB_
-		if test -n "$_USE_GLIBC_TUNABLES"
-		then
-			g=
-			LD_PRELOAD=$_USE_GLIBC_PRELOAD
-			for t in \
-				glibc.malloc.check=1 \
-				glibc.malloc.perturb=165
-			do
-				g="${g#:}:$t"
-			done
-			GLIBC_TUNABLES=$g
-			export LD_PRELOAD GLIBC_TUNABLES
-		fi
-	}
-	teardown_malloc_check () {
-		unset MALLOC_CHECK_ MALLOC_PERTURB_
-		unset LD_PRELOAD GLIBC_TUNABLES
-	}
-fi
-
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
 unset CDPATH
@@ -1486,6 +1436,56 @@ GIT_ATTR_NOSYSTEM=1
 GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/.."
 export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM GIT_CEILING_DIRECTORIES
 
+# Add libc MALLOC and MALLOC_PERTURB test only if we are not executing
+# the test with valgrind and have not compiled with conflict SANITIZE
+# options.
+if test -n "$valgrind" ||
+   test -n "$SANITIZE_ADDRESS" ||
+   test -n "$SANITIZE_LEAK" ||
+   test -n "$TEST_NO_MALLOC_CHECK"
+then
+	setup_malloc_check () {
+		: nothing
+	}
+	teardown_malloc_check () {
+		: nothing
+	}
+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 &&
+	   stderr=$(LD_PRELOAD=$_USE_GLIBC_PRELOAD git version 2>&1 >/dev/null) &&
+	   test -z "$stderr"
+	then
+		_USE_GLIBC_TUNABLES=YesPlease
+	fi
+	setup_malloc_check () {
+		local g
+		local t
+		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
+		export MALLOC_CHECK_ MALLOC_PERTURB_
+		if test -n "$_USE_GLIBC_TUNABLES"
+		then
+			g=
+			LD_PRELOAD=$_USE_GLIBC_PRELOAD
+			for t in \
+				glibc.malloc.check=1 \
+				glibc.malloc.perturb=165
+			do
+				g="${g#:}:$t"
+			done
+			GLIBC_TUNABLES=$g
+			export LD_PRELOAD GLIBC_TUNABLES
+		fi
+	}
+	teardown_malloc_check () {
+		unset MALLOC_CHECK_ MALLOC_PERTURB_
+		unset LD_PRELOAD GLIBC_TUNABLES
+	}
+fi
+
 if test -z "$GIT_TEST_CMP"
 then
 	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
-- 
2.47.0.527.gfb211c7f3b

Re: [PATCH 2/1] test-lib: move malloc-debug setup after $PATH setup

[Toon Claes]

Jeff King <peff@peff.net> writes:

> Originally, the conditional definition of the setup/teardown functions
> for malloc checking could be run at any time, because they depended only
> on command-line options and the system getconf function.
>
> [...snip...]
>
>  if test -z "$GIT_TEST_CMP"
>  then
>  	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
> -- 
> 2.47.0.527.gfb211c7f3b

For what it's worth, I've tested this patch and I wanted to quickly
confirm this works as expected.

-- 
Toon

Re: [PATCH 2/1] test-lib: move malloc-debug setup after $PATH setup

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> Originally, the conditional definition of the setup/teardown functions
>> for malloc checking could be run at any time, because they depended only
>> on command-line options and the system getconf function.
>>
>> [...snip...]
>>
>>  if test -z "$GIT_TEST_CMP"
>>  then
>>  	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
>> -- 
>> 2.47.0.527.gfb211c7f3b
>
> For what it's worth, I've tested this patch and I wanted to quickly
> confirm this works as expected.

Thanks, both.