[v2,5/5] branch: fix branch_checked_out() leaks

Commit Message

Derrick Stolee June 14, 2022, 7:27 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 branch.c                  | 25 +++++++++++++++----------
 t/t2407-worktree-heads.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 10 deletions(-)

Patch

diff --git a/branch.c b/branch.c
index d4ddec6f4e5..6007ee7baeb 100644
--- a/branch.c
+++ b/branch.c
@@ -385,25 +385,29 @@  static void prepare_checked_out_branches(void)
 	worktrees = get_worktrees();
 
 	while (worktrees[i]) {
+		char *old;
 		struct wt_status_state state = { 0 };
 		struct worktree *wt = worktrees[i++];
 
 		if (wt->is_bare)
 			continue;
 
-		if (wt->head_ref)
-			strmap_put(&current_checked_out_branches,
-				   wt->head_ref,
-				   xstrdup(wt->path));
+		if (wt->head_ref) {
+			old = strmap_put(&current_checked_out_branches,
+					 wt->head_ref,
+					 xstrdup(wt->path));
+			free(old);
+		}
 
 		if (wt_status_check_rebase(wt, &state) &&
 		    (state.rebase_in_progress || state.rebase_interactive_in_progress) &&
 		    state.branch) {
 			struct strbuf ref = STRBUF_INIT;
 			strbuf_addf(&ref, "refs/heads/%s", state.branch);
-			strmap_put(&current_checked_out_branches,
-				   ref.buf,
-				   xstrdup(wt->path));
+			old = strmap_put(&current_checked_out_branches,
+					 ref.buf,
+					 xstrdup(wt->path));
+			free(old);
 			strbuf_release(&ref);
 		}
 		wt_status_state_free_buffers(&state);
@@ -412,9 +416,10 @@  static void prepare_checked_out_branches(void)
 		    state.branch) {
 			struct strbuf ref = STRBUF_INIT;
 			strbuf_addf(&ref, "refs/heads/%s", state.branch);
-			strmap_put(&current_checked_out_branches,
-				   ref.buf,
-				   xstrdup(wt->path));
+			old = strmap_put(&current_checked_out_branches,
+					 ref.buf,
+					 xstrdup(wt->path));
+			free(old);
 			strbuf_release(&ref);
 		}
 		wt_status_state_free_buffers(&state);
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index a5aec1486c5..b6be42f74a2 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -98,4 +98,32 @@  test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase
 	grep "refusing to fetch into branch" err
 '
 
+test_expect_success 'refuse to overwrite when in error states' '
+	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
+	test_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&
+
+	# Both branches are currently under rebase.
+	mkdir -p .git/worktrees/wt-3/rebase-merge &&
+	touch .git/worktrees/wt-3/rebase-merge/interactive &&
+	echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&
+	mkdir -p .git/worktrees/wt-4/rebase-merge &&
+	touch .git/worktrees/wt-4/rebase-merge/interactive &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/head-name &&
+	echo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/onto &&
+
+	# Both branches are currently under bisect.
+	touch .git/worktrees/wt-4/BISECT_LOG &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&
+	touch .git/worktrees/wt-1/BISECT_LOG &&
+	echo refs/heads/fake-1 >.git/worktrees/wt-1/BISECT_START &&
+
+	for i in 1 2
+	do
+		test_must_fail git branch -f fake-$i HEAD 2>err &&
+		grep "cannot force update the branch '\''fake-$i'\'' checked out at" err ||
+			return 1
+	done
+'
+
 test_done