git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Date
Jun 14, 2022, 19:27 UTC
Message-ID
<0aa9478bc3815726a47ea99cac0fd333e95c1584.1655234853.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1254.v2.git.1655234853.gitgitgadget@gmail.com>
From: Derrick Stolee <derrickstolee@github.com>

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 <derrickstolee@github.com>
---
 branch.c                  | 25 +++++++++++++++----------
 t/t2407-worktree-heads.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 10 deletions(-)
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

-- 
gitgitgadget
Previous: Derrick Stolee via GitGitGadgetNext: Phillip Wood
Message 27 of 28 in “Create branch_checked_out() helper”
  1. 0/4 Create branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 8, 2022
  2. 1/4 branch: add branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 8, 2022
  3. Junio C HamanoJun 9, 2022
  4. Ævar Arnfjörð BjarmasonJun 13, 2022
  5. Derrick StoleeJun 14, 2022
  6. Ævar Arnfjörð BjarmasonJun 14, 2022
  7. Phillip WoodJun 14, 2022
  8. Phillip WoodJun 14, 2022
  9. Derrick StoleeJun 14, 2022
  10. 2/4 branch: check for bisects and rebasesDerrick Stolee via GitGitGadget, Jun 8, 2022
  11. Junio C HamanoJun 8, 2022
  12. 3/4 fetch: use new branch_checked_out() and add testsDerrick Stolee via GitGitGadget, Jun 8, 2022
  13. Ævar Arnfjörð BjarmasonJun 14, 2022
  14. Phillip WoodJun 14, 2022
  15. Derrick StoleeJun 14, 2022
  16. 4/4 branch: use branch_checked_out() when deleting refsDerrick Stolee via GitGitGadget, Jun 8, 2022
  17. 5/5 branch: fix branch_checked_out() leaksDerrick Stolee, Jun 13, 2022
  18. Junio C HamanoJun 13, 2022
  19. Ævar Arnfjörð BjarmasonJun 14, 2022
  20. Derrick StoleeJun 14, 2022
  21. Ævar Arnfjörð BjarmasonJun 14, 2022
  22. 0/5 Create branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 14, 2022
  23. 1/5 branch: add branch_checked_out() helperDerrick Stolee via GitGitGadget, Jun 14, 2022
  24. 2/5 branch: check for bisects and rebasesDerrick Stolee via GitGitGadget, Jun 14, 2022
  25. 3/5 fetch: use new branch_checked_out() and add testsDerrick Stolee via GitGitGadget, Jun 14, 2022
  26. 4/5 branch: use branch_checked_out() when deleting refsDerrick Stolee via GitGitGadget, Jun 14, 2022
  27. 5/5 branch: fix branch_checked_out() leaksDerrick Stolee via GitGitGadget, Jun 14, 2022
  28. Phillip WoodJun 19, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.