Re: [PATCH 5/5] branch: fix branch_checked_out() leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 14 2022, Derrick Stolee wrote:

> On 6/13/2022 8:33 PM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Jun 13 2022, Derrick Stolee wrote:
>> 
>>> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:
>
>>> While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the
>>> leakage and prevent it in the future, t2407 uses helpers such as 'git
>>> clone' that cause the test to fail under that mode.
>> 
>> If you apply this:
>> 	
>> 	diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
>> 	index 0760595337b..d41171acb83 100755
>> 	--- a/t/t2407-worktree-heads.sh
>> 	+++ b/t/t2407-worktree-heads.sh
>> 	@@ -10,16 +10,8 @@ test_expect_success 'setup' '
>> 	 		test_commit $i &&
>> 	 		git branch wt-$i &&
>> 	 		git worktree add wt-$i wt-$i || return 1
>> 	-	done &&
>> 	-
>> 	-	# Create a server that updates each branch by one commit
>> 	-	git clone . server &&
>> 	-	git remote add server ./server &&
>> 	-	for i in 1 2 3 4
>> 	-	do
>> 	-		git -C server checkout wt-$i &&
>> 	-		test_commit -C server A-$i || return 1
>> 	 	done
>> 	+
>> 	 '
>> 	 
>> 	 test_expect_success 'refuse to overwrite: checked out in worktree' '
>> 
>> And compile with SANITIZE=leak then this will pass as:
>> 
>> 	./t2407-worktree-heads.sh  --run=1,6
>
> Of course this works for the tests that don't need the 'server' repo,
> but it fails in the tests that _do_ need it.
>
> I'm able to make this work by creating the 'server' with init and
> creating the wt-$i branches from scratch (they don't need to be
> fast-forward updates).
>
> The linux-leaks tests still fail due to 'git fetch' and 'git bisect'
> calls, but these can be avoided by carefully splitting the tests and
> using the !SANITIZE_LEAK prereq.

I mean copy/paste the existing file to a new one with this on top, then
remove all tests that aren't your new one. That worked for me, doesn't
it work for you?

>> Normally I'd just say "let's leave it for later", but in this case the
>> entire point of the commit and the relatively lengthy test is to deal
>> with a memory leak, so just copy/pasting the few lines of setup you
>> actually need to a new test & testing with SANITIZE=leak seems worth the
>> effort in this case.
>
> Well, the point isn't to use automation to check for leaks, but instead
> to fix leaks and add tests for the case where we previously had leaks.
> The tests demonstrate that we aren't accidentally introducing a use-after-
> free or double-free.

I mean that the patch subject is "fix[...]leaks", and this gives it a
CI'd regression test, which seems worth it.

Of course it's also nice to check for use-after-fre etc., but we have
other coverage for that. E.g. glibc in CI spots double-free issues with
how we set it up, and we tend to segfault soon enough (or someone runs
SANITIZE=address) on use-after-free.