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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 14, 2022, 00:33 UTC
Message-ID: <220614.868rq0ytaa.gmgdl@evledraar.gmail.com>
In-Reply-To: <6cd7db33-6ab5-9843-4483-4cce9835b177@github.com>

On Mon, Jun 13 2022, Derrick Stolee wrote:

Show 44 quoted lines

> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote:
>> This is a replacement for some patches from v2 of my 'git rebase
>> --update-refs' topic [1]. After some feedback from Philip, I've decided to
>> pull that topic while I rework how I track the refs to rewrite [2]. This
>> series moves forward with the branch_checked_out() helper that was a bit
>> more complicated than expected at first glance. This series is a culmination
>> of the discussion started by Junio at [3].
>> 
>
> Junio pointed out that patch 1 introduced a memory leak when a ref
> is checked out in multiple places. Here is a patch to fix that
> scenario. It applies cleanly on top of patch 4, so I include it as
> a new "patch 5". I will include it in any v2 of the full series, if
> needed.
>
> Thanks,
> -Stolee
>
> ---- >8 ----
>
> From c3842b36ebb4053ac49b0306154b840431f9bf6f Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <derrickstolee@github.com>
> Date: Mon, 13 Jun 2022 10:33:20 -0400
> Subject: [PATCH 5/5] branch: fix branch_checked_out() leaks
>
> The branch_checked_out() method populates a strmap linking a refname to
> a worktree that has that branch checked out. While unlikely, it is
> possible that a bug or filesystem manipulation could create a scenario
> where the same ref is checked out in multiple places. Further, there are
> some states in an interactive rebase where HEAD and REBASE_HEAD point to
> the same ref, leading to multiple insertions into the strmap. In either
> case, the strmap_put() method returns the old value which is leaked.
>
> Update branch_checked_out() to consume that pointer and free it.
>
> Add a test in t2407 that checks this erroneous case. The test "checks
> itself" by first confirming that the filesystem manipulations it makes
> trigger the branch_checked_out() logic, and then sets up similar
> manipulations to make it look like there are multiple worktrees pointing
> to the same ref.
>
> 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

I.e. you only needed the earlier part of the setup, and not "clone.

Given that I think it makes sense to just create a t2408-worktree-heads-leak.sh or something for this new test, then you can use TEST_PASSES_SANITIZE_LEAK.

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.