patch, 2 partst0000: truly leak free

While looking at the leaks reported by our new CI job, noticed there was a hidden one when running t0000 in macOS.

The first patch fixex that leak, and the second one fixes the reason why it was hidden.

  [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA
  [PATCH 2/2] t0000: avoid masking git exit value through pipes

Carlo

b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07) adds a way to route some bigger allocations out of the stack and free them through the addition of two conveniently named macros, but leaves the calls to free the xalloca part, which could be also in the heap, if the system doesn't HAVE_ALLOCA (ex: macOS).

Add the missing free call, and while at it, change the expression to match in both macros for easy of readability.

This avoids a leak reported by LSAN as while running t0000 but that wouldn't fail the test (which will be fixed next) :

  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 tree-diff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Wed, Sep 15, 2021 at 07:37:05PM -0700, Carlo Marcelo Arenas Belón wrote:

s/easy/ease/ or s/easy of/easier/.

Nit; extra space between the closing parenthesis and colon.

OK. So the point is that FAST_ARRAY_ALLOC uses xalloca() for small arrays. But that might turn into a full-blown malloc() if we don't have alloca.h. So we need to call xalloca_free() which is a noop if we used alloca(), but calls free() if we actually used malloc() instead.

Now that I wrote it out myself, I think you basically said as much in the patch message. But it may have been clearer to say:

    Add the missing free call, [xmalloca_free(), which is a noop if we
    allocated memory in the stack frame, but a real free() if we
    allocaegd in the heap instead].

Thanks, Taylor

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Nicely spotted and analyzed.  Thanks.

9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds tets for commit-tree that mask the return exit from git as described in a378fee5b07.

Fix the tests, to avoid pipes by using instead a temporary file.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0000-basic.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

On Wed, Sep 15, 2021 at 07:37:06PM -0700, Carlo Marcelo Arenas Belón wrote:

s/tets/tests

In this and the below tests which had a similar transformation, the first invocation does not mask its error, since it's on the right-hand side of a pipe.

But piping "git show" to sed will mask the exit code of the former. So that makes sense. But I would like to see us avoid an unnecessary cat-into-pipe and instead redirect out into sed, like "sed -n -e ... <out".

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Good eyes. There is no reason why we want to cat a single file into pipe (unless we are testing the pipe mechanism of the underlying OS, that is)---the downstream command can be fed the file from its standard input, or for commands like "sed" that takes its input from files listed on the command line, you should be able to write it without the input "<" redirection.

Thanks.

On Wed, Sep 15, 2021 at 11:23:28PM -0700, Junio C Hamano wrote:

Even better, thanks for noticing the flaw in my suggestion.

Thanks, Taylor

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

"tests" (no need to resend---already locally tweaked).

Thanks.

On Wed, Sep 15 2021, Carlo Marcelo Arenas Belón wrote:

Well spotted. This looks good to me sans the cat v.s. pipe to sed that was already pointed out. In addition to that (Taylor may have meant this, but not said so explicitly) it looks like you can also e.g.:

    v=$(echo foo | ...) &&
    git show ... $v

Instead of:

    echo foo | ... >out &&
    v=$(cat out) &&
    git show ... $v

But that's a small nit either way.

On the change as a whole:

For what it's worth two ways we could have avoided this sort of edge case is if my SANITIZE=leak series would e.g. save the log of leaks somewhere and scour it later, i.e. something like what Jeff King suggested in[1]. I just re-rolled it at [2], but not with that approach (but response to your comments on another thread).

I don't think that's worth doing for an intial implementation of that feature for the reasons argued in its 2/2, just say'n.

The other (and more general) way would be to resurrect my GIT_TEST_PIPEFAIL mode[3]. I just tried it now in combination with the SANITIZE=leak test mode, and it would have caught this issue[4]!

I'll see if I can re-poke the bash maintainer (Chet Ramey) about some way forward for that mode. I had an off-list discussion with him about my proposed "set -o pipefail" change back in January and he rightly pointed out that it's intended behavior, meant to catch the sort of thing that was discussed here on-list in the thread around pagers and pipefail [5].

So since writing that WIP patch I've come around to his view that "set -o pipefail" can't be changed like that in general, but perhaps he'd accept a patch for an optional configuration on top of that. I'll contact him.

1. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v6-0.2-00000000000-20210916T085311Z-avarab@gmail.com/
3. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/
4. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
5. https://lore.kernel.org/git/YAG%2FvzctP4JwSp5x@zira.vinc17.org/

While looking at the leaks reported by our new CI job, noticed there was a hidden one when running t0000 in macOS.

The first one fixes the leak and the second one the reason why it was silent.

v2 includes all suggestions and feedback, and tries probably too hard to modify the last test in a way that wouldn't require long lines.

Carlo Marcelo Arenas Belón (2):
  tree-diff: fix leak when not HAVE_ALLOCA_H
  t0000: avoid masking git exit value through pipes

 t/t0000-basic.sh | 23 ++++++++++++-----------
 tree-diff.c      |  4 +++-
 2 files changed, 15 insertions(+), 12 deletions(-)

b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07) adds a way to route some bigger allocations out of the stack and free them through the addition of two conveniently named macros, but leaves the calls to free the xalloca part, which could be also in the heap, if the system doesn't HAVE_ALLOCA_H (ex: macOS and other BSD).

Add the missing free call, xalloca_free(), which is a noop if we allocated memory in the stack frame, but a real free() if we allocated in the heap instead, and while at it, change the expression to match in both macros for ease of readability.

This avoids a leak reported by LSAN while running t0000 but that wouldn't fail the test (which is fixed in the next patch):

  SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2: includes an improved commit message, thanks to Taylor

 tree-diff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Thu, Sep 16, 2021 at 01:55:22AM -0700, Carlo Marcelo Arenas Belón wrote:

Thanks, this is definitely my bug introduced by b8ba412bf7 and this is the right fix.

I continue to find the whole xalloca() thing pretty gross, and doubly so now that there are _two_ layers of "maybe alloca(), and maybe malloc()" logic (one in xalloca(), and one in this FAST_ARRAY stuff).

We should definitely take this fix to address the immediate problem, but I wonder if this size logic should be pushed into xalloca to make this kind of problem harder. Of course this is the only caller, so it might not matter much either way.

(I'd also be really happy to see it go away entirely, as alloca() is a foot-gun in the first place. But I think it did make things slightly faster. It might be worth re-measuring).

-Peff

9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds tests for commit-tree that mask the return exit from git as described in a378fee5b07 (Documentation: add shell guidelines, 2018-10-05).

Fix the tests, to avoid pipes by using a temporary file instead.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
* avoid changing some of the code as suggested by Taylor
* no need for pipes or stdin redirection as suggested by Junio

 t/t0000-basic.sh | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)y

On Thu, Sep 16, 2021 at 01:55:21AM -0700, Carlo Marcelo Arenas Belón wrote:

Thanks, this version looks good to me. I suspect the "tries probably too hard" is referring to putting the sed expressions into a file and then loading them from that file with `-f`.

I wouldn't mind the long line, or splitting it across multiple lines with `\`. But what you wrote is fine, too.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

BTW: no need to put "reroll for" in the subject line for this series.
The [PATCH v2] indicates that it is a reroll.