Re: [PATCH 0/7] Fix all leaks in t0001

On 14/03/2021 17:54, Andrzej Hunt wrote:

I was wrong when it comes to LSAN being broken. What was actually happening is: we default to running ASAN and LSAN with abort_on_error=1, and I had overridden that setting when running with ASAN. When I switched to LSAN, abort_on_error was enabled again - and I was just misinterpreting the intentional abortion as opposed to seeing an unexplained crashes. [As far as I can tell, abort_on_error is needed to detect leaks during a test_must_fail and similar scenarios.]

Actually I might have been unintentionally right the first time round: running clang+LSAN against t0000 (with pipe-related fixes applied) results in reports as follows, which are effectively useless:

Direct leak of 41 byte(s) in 1 object(s) allocated from:
    #0 0x44fc98 in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/lsan/lsan_interceptors.cpp:56:3
    #1 0x7fcdbb3dc749 in strdup (/lib64/libc.so.6+0x8b749)

Switching back to ASAN shows me the real stack instead:

Direct leak of 41 byte(s) in 1 object(s) allocated from:
    #0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0x9ab1b8 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
    ... rest of stack here ...

To make it trickier, that new "leak" happens inside a test_must_fail - the LSAN output is swallowed, making it hard to diagnose. I'll try to prepare a separate patch to not discard stderr in that scenario.

The issue here is that:

 - test_must_fail does actually keep git's stderr on stderr (there is
   some redirection within test_must_fail itself - but eventually
   what was git's stderr becomes test_must_fail's stderr).
 - many uses of test_must_fail redirect stderr into a temporary file.
 - when git fails as expected, test_must_fail succeeds, and the
   next command in the test validates that temporary file (that
   validation step prints that error output on failure).
 - however if git aborted, or didn't fail as expected: test_must_fail
   returns an error code. git's error messages remain in the temporary
   file and aren't printed.

My potentially naive idea is that within test_must_fail, git's output should be sent to both test_must_fail's stderr, and also to fd-4 (which seems to be where verbose output should be sent).

However hat might also result in errors being printed twice (I believe that will happen for test_must_fail uses that don't redirect stderr into a temporary file).