threads / patch / 57170

patcht4126: this test does not pass SANITIZE_LEAK; quit claiming it does

Subject: [PATCH] t4126: this test does not pass SANITIZE_LEAK; quit claiming it does

## tl;dr

4 messages between Dec 31, 2021 and Jan 4, 2022. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Elijah Newren via GitGitGadget· Dec 31, 2021, 05:15 UTC · lore

From: Elijah Newren <newren@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    t4126: this test does not pass SANITIZE_LEAK; quit claiming it does
    
    My builds in several different topics keep running into this, and I'm
    sure it's a false positive -- I didn't change anything affecting this
    test. I've just been ignoring it and submitting anyway, and I suspect
    others are too.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1177%2Fnewren%2Ft4126-apply-empty-is-not-leak-free-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1177/newren/t4126-apply-empty-is-not-leak-free-v1
Pull-Request: https://github.com/git/git/pull/1177

 t/t4126-apply-empty.sh | 1 -
 1 file changed, 1 deletion(-)

Show changes to t/t4126-apply-empty.sh +0 −1

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index 82284d2f45d..ebbac79f20e 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -3,7 +3,6 @@
 test_description='apply empty'
 
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907

-- 
gitgitgadget

Johannes Schindelin· Jan 1, 2022, 23:17 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH] t4126: this test does not pass SANITIZE_LEAK; quit claiming it does

Hi Elijah,

On Fri, 31 Dec 2021, Elijah Newren via GitGitGadget wrote:

Show 10 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     t4126: this test does not pass SANITIZE_LEAK; quit claiming it does
>
>     My builds in several different topics keep running into this, and I'm
>     sure it's a false positive -- I didn't change anything affecting this
>     test. I've just been ignoring it and submitting anyway, and I suspect
>     others are too.

So that's what is causing this. Thank you for chasing it down, it has been on my TO-DO list e.g. due to https://github.com/git-for-windows/git/runs/4622790431?check_suite_focus=true#step:5:146

Given that it points to a leak in `cmd_format_patch()` (oh no, we're _LEAKING_ some _MEMORY_ in a built-in, how did that ever happen, we must free it before... *checks notes* ... wait, we're quitting immediately after this function anyway? </sarcasm>) I wonder whether the disruption caused by `linux-leaks` should really be considered worth the benefit.

Anyway. Thank you for the patch!

Ciao, Dscho

Junio C Hamano· Jan 2, 2022, 01:21 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH] t4126: this test does not pass SANITIZE_LEAK; quit claiming it does

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     t4126: this test does not pass SANITIZE_LEAK; quit claiming it does

Is this continuation of [*1*]?

I think it will go away when dee839a2 (format-patch: mark rev_info with UNLEAK, 2021-12-16) is merged.

[Reference]

*1* https://lore.kernel.org/git/xmqqee6dz5s9.fsf@gitster.g/

Elijah Newren· Jan 4, 2022, 01:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] t4126: this test does not pass SANITIZE_LEAK; quit claiming it does

On Sat, Jan 1, 2022 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >     t4126: this test does not pass SANITIZE_LEAK; quit claiming it does
>
> Is this continuation of [*1*]?

Ah, yes it is.

> I think it will go away when dee839a2 (format-patch: mark
> rev_info with UNLEAK, 2021-12-16) is merged.

Can we fast track that patch? The patch looks good to me. In general, though, I would think the simpler fix of just removing the TEST_PASSES_SANITIZE_LEAK=true would be safer and could be fast tracked.

I'm sorry I missed these potential problems cropping up when I reviewed the earlier series (which added these flags to a bunch of scripts). I just didn't foresee these consequences.