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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 14, 2022, 15:37 UTC
Message-ID: <0d00d910-a7aa-dfd8-a24c-51968800c2a8@github.com>
In-Reply-To: <220614.868rq0ytaa.gmgdl@evledraar.gmail.com>

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:

Show 32 quoted lines

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

Show 5 quoted lines

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

Thanks, -Stolee