On Mon, Jun 13 2022, Derrick Stolee wrote: > 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@xxxxxxxxxx> > 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.
