* [PATCH 0/2] t0000: truly leak free @ 2021-09-16 2:37 Carlo Marcelo Arenas Belón 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 2:37 UTC (permalink / raw) To: git; +Cc: peff, gitster 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA 2021-09-16 2:37 [PATCH 0/2] t0000: truly leak free Carlo Marcelo Arenas Belón @ 2021-09-16 2:37 ` Carlo Marcelo Arenas Belón 2021-09-16 5:17 ` Taylor Blau 2021-09-16 5:27 ` Junio C Hamano 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón 2 siblings, 2 replies; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 2:37 UTC (permalink / raw) To: git; +Cc: peff, gitster, Carlo Marcelo Arenas Belón 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(-) 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón @ 2021-09-16 5:17 ` Taylor Blau 2021-09-16 5:27 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Taylor Blau @ 2021-09-16 5:17 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, peff, gitster 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. > 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)); \ 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón 2021-09-16 5:17 ` Taylor Blau @ 2021-09-16 5:27 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2021-09-16 5:27 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, peff Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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). Nicely spotted and analyzed. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 2:37 [PATCH 0/2] t0000: truly leak free Carlo Marcelo Arenas Belón 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón @ 2021-09-16 2:37 ` Carlo Marcelo Arenas Belón 2021-09-16 5:21 ` Taylor Blau ` (2 more replies) 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón 2 siblings, 3 replies; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 2:37 UTC (permalink / raw) To: git; +Cc: peff, gitster, Carlo Marcelo Arenas Belón 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(-) 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón @ 2021-09-16 5:21 ` Taylor Blau 2021-09-16 6:23 ` Junio C Hamano 2021-09-16 5:29 ` Junio C Hamano 2021-09-16 10:45 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 15+ messages in thread From: Taylor Blau @ 2021-09-16 5:21 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, peff, gitster 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 > 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") && 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 5:21 ` Taylor Blau @ 2021-09-16 6:23 ` Junio C Hamano 2021-09-16 16:49 ` Taylor Blau 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2021-09-16 6:23 UTC (permalink / raw) To: Taylor Blau; +Cc: Carlo Marcelo Arenas Belón, git, peff Taylor Blau <me@ttaylorr.com> writes: >> + tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") && > > 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". 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 6:23 ` Junio C Hamano @ 2021-09-16 16:49 ` Taylor Blau 0 siblings, 0 replies; 15+ messages in thread From: Taylor Blau @ 2021-09-16 16:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, Carlo Marcelo Arenas Belón, git, peff On Wed, Sep 15, 2021 at 11:23:28PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> + tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") && > > > > 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". > > [...] 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. Even better, thanks for noticing the flaw in my suggestion. Thanks, Taylor ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 5:21 ` Taylor Blau @ 2021-09-16 5:29 ` Junio C Hamano 2021-09-16 10:45 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2021-09-16 5:29 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, peff Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds > tets for commit-tree that mask the return exit from git as described "tests" (no need to resend---already locally tweaked). Thanks. > 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(-) > > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 5:21 ` Taylor Blau 2021-09-16 5:29 ` Junio C Hamano @ 2021-09-16 10:45 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-09-16 10:45 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, peff, gitster On Wed, Sep 15 2021, 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 > 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(-) > > 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 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/ ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users 2021-09-16 2:37 [PATCH 0/2] t0000: truly leak free Carlo Marcelo Arenas Belón 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 ` Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón ` (2 more replies) 2 siblings, 3 replies; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 UTC (permalink / raw) To: git; +Cc: me, gitster, Carlo Marcelo Arenas Belón 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 ` Carlo Marcelo Arenas Belón 2021-09-16 15:00 ` Jeff King 2021-09-16 8:55 ` [PATCH v2 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 16:53 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Taylor Blau 2 siblings, 1 reply; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 UTC (permalink / raw) To: git; +Cc: me, gitster, Carlo Marcelo Arenas Belón 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(-) 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H 2021-09-16 8:55 ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón @ 2021-09-16 15:00 ` Jeff King 0 siblings, 0 replies; 15+ messages in thread From: Jeff King @ 2021-09-16 15:00 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, me, gitster 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] t0000: avoid masking git exit value through pipes 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 ` Carlo Marcelo Arenas Belón 2021-09-16 16:53 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Taylor Blau 2 siblings, 0 replies; 15+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-16 8:55 UTC (permalink / raw) To: git; +Cc: me, gitster, Carlo Marcelo Arenas Belón 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 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón @ 2021-09-16 16:53 ` Taylor Blau 2 siblings, 0 replies; 15+ messages in thread From: Taylor Blau @ 2021-09-16 16:53 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, gitster 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 <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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-09-16 18:35 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-16 2:37 [PATCH 0/2] t0000: truly leak free Carlo Marcelo Arenas Belón 2021-09-16 2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón 2021-09-16 5:17 ` Taylor Blau 2021-09-16 5:27 ` Junio C Hamano 2021-09-16 2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 5:21 ` Taylor Blau 2021-09-16 6:23 ` Junio C Hamano 2021-09-16 16:49 ` Taylor Blau 2021-09-16 5:29 ` Junio C Hamano 2021-09-16 10:45 ` Ævar Arnfjörð Bjarmason 2021-09-16 8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón 2021-09-16 8:55 ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón 2021-09-16 15:00 ` Jeff King 2021-09-16 8:55 ` [PATCH v2 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón 2021-09-16 16:53 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Taylor Blau
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).