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

From: Andrzej Hunt <andrzej@ahunt.org>
To: Jeff King <peff@peff.net>,
	Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/7] Fix all leaks in t0001
Date: Mon, 15 Mar 2021 17:23:44 +0100	[thread overview]
Message-ID: <6a072f9a-ac12-0cfe-9b04-c9342017f071@ahunt.org> (raw)
In-Reply-To: <d22dc5e6-415e-e265-e894-67b28fe9fe54@ahunt.org>

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