[PATCH 0/2] t0000: 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 <redacted>
---
 tree-diff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tree-diff.c b/tree-diff.c
index 1572615bd9..437c98a70e 100644
--- a/tree-diff.c
+++ b/tree-diff.c

@@ -21,7 +21,9 @@
 		ALLOC_ARRAY((x), nr); \
 } while(0)
 #define FAST_ARRAY_FREE(x, nr) do { \
-	if ((nr) > 2) \
+	if ((nr) <= 2) \
+		xalloca_free((x)); \
+	else \
 		free((x)); \
 } while(0)
 

-- 
2.33.0.481.g26d3bed244

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 <redacted>
---
 t/t0000-basic.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index cb87768513..545ff5af13 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
 P=$(test_oid root)
 
 test_expect_success 'git commit-tree records the correct tree in a commit' '
-	commit0=$(echo NO | git commit-tree $P) &&
-	tree=$(git show --pretty=raw $commit0 |
-		 sed -n -e "s/^tree //p" -e "/^author /q") &&
+	echo NO | git commit-tree $P >out &&
+	commit0=$(cat out) &&
+	git show --pretty=raw $commit0 >out &&
+	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
 	test "z$tree" = "z$P"
 '
 
 test_expect_success 'git commit-tree records the correct parent in a commit' '
-	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
-	parent=$(git show --pretty=raw $commit1 |
-		sed -n -e "s/^parent //p" -e "/^author /q") &&
+	echo NO | git commit-tree $P -p $commit0 >out &&
+	commit1=$(cat out) &&
+	git show --pretty=raw $commit1 >out &&
+	parent=$(cat out | sed -n -e "s/^parent //p" -e "/^author /q") &&
 	test "z$commit0" = "z$parent"
 '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
-	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	     parent=$(git show --pretty=raw $commit2 |
+	echo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&
+	commit2=$(cat out) &&
+	git show --pretty=raw $commit2 >out &&
+	parent=$(cat out |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		sort -u) &&
 	test "z$commit0" = "z$parent" &&
-	numparent=$(git show --pretty=raw $commit2 |
+	git show --pretty=raw $commit2 >out &&
+	numparent=$(cat out |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		wc -l) &&
 	test $numparent = 1

-- 
2.33.0.481.g26d3bed244

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

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.

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

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

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

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

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

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

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

-- 
2.33.0.481.g26d3bed244

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 <redacted>
---
v2: includes an improved commit message, thanks to Taylor

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

diff --git a/tree-diff.c b/tree-diff.c
index 1572615bd9..437c98a70e 100644
--- a/tree-diff.c
+++ b/tree-diff.c

@@ -21,7 +21,9 @@
 		ALLOC_ARRAY((x), nr); \
 } while(0)
 #define FAST_ARRAY_FREE(x, nr) do { \
-	if ((nr) > 2) \
+	if ((nr) <= 2) \
+		xalloca_free((x)); \
+	else \
 		free((x)); \
 } while(0)
 

-- 
2.33.0.481.g26d3bed244

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

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index cb87768513..5c342de713 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -1271,28 +1271,29 @@ P=$(test_oid root)
 
 test_expect_success 'git commit-tree records the correct tree in a commit' '
 	commit0=$(echo NO | git commit-tree $P) &&
-	tree=$(git show --pretty=raw $commit0 |
-		 sed -n -e "s/^tree //p" -e "/^author /q") &&
+	git show --pretty=raw $commit0 >out &&
+	tree=$(sed -n -e "s/^tree //p" -e "/^author /q" out) &&
 	test "z$tree" = "z$P"
 '
 
 test_expect_success 'git commit-tree records the correct parent in a commit' '
 	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
-	parent=$(git show --pretty=raw $commit1 |
-		sed -n -e "s/^parent //p" -e "/^author /q") &&
+	git show --pretty=raw $commit1 >out &&
+	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" out) &&
 	test "z$commit0" = "z$parent"
 '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	     parent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		sort -u) &&
+	git show --pretty=raw $commit2 >out &&
+	cat >match.sed <<-\EOF &&
+	s/^parent //p
+	/^author /q
+	EOF
+	parent=$(sed -n -f match.sed out | sort -u) &&
 	test "z$commit0" = "z$parent" &&
-	numparent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		wc -l) &&
-	test $numparent = 1
+	git show --pretty=raw $commit2 >out &&
+	test_stdout_line_count = 1 sed -n -f match.sed out
 '
 
 test_expect_success 'update-index D/F conflict' '

-- 
2.33.0.481.g26d3bed244

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/

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

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.

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

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

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.

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 [off-list ref]

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.