| Message ID | 6cd7db33-6ab5-9843-4483-4cce9835b177@github.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create branch_checked_out() helper | expand |
Derrick Stolee <derrickstolee@github.com> writes: > 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(¤t_checked_out_branches, > - wt->head_ref, > - xstrdup(wt->path)); > + if (wt->head_ref) { > + old = strmap_put(¤t_checked_out_branches, > + wt->head_ref, > + xstrdup(wt->path)); > + free(old); > + } While it is equivalent to free(strmap_put(¤t_checked_out_branches, wt->head_ref, xstrdup(wt->path))); writing the "put the new one" and "discard the old one (if exists)" as separate steps like the above does make it easier to follow, at least to me. Thanks. Will queue.
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@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.
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: >> 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. > 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
On Tue, Jun 14 2022, Derrick Stolee wrote: > 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: > >>> 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. I mean copy/paste the existing file to a new one with this on top, then remove all tests that aren't your new one. That worked for me, doesn't it work for you? >> 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. I mean that the patch subject is "fix[...]leaks", and this gives it a CI'd regression test, which seems worth it. Of course it's also nice to check for use-after-fre etc., but we have other coverage for that. E.g. glibc in CI spots double-free issues with how we set it up, and we tend to segfault soon enough (or someone runs SANITIZE=address) on use-after-free.
diff --git a/branch.c b/branch.c index c0fe6ea0b65..390c092a18f 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(¤t_checked_out_branches, - wt->head_ref, - xstrdup(wt->path)); + if (wt->head_ref) { + old = strmap_put(¤t_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(¤t_checked_out_branches, - ref.buf, - xstrdup(wt->path)); + old = strmap_put(¤t_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(¤t_checked_out_branches, - ref.buf, - xstrdup(wt->path)); + old = strmap_put(¤t_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 6dcc0d39a2d..0760595337b 100755 --- a/t/t2407-worktree-heads.sh +++ b/t/t2407-worktree-heads.sh @@ -78,4 +78,43 @@ test_expect_success 'refuse to overwrite: worktree in rebase' ' grep "refusing to fetch into branch '\''refs/heads/wt-4'\''" 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_* && + + git branch -f fake1 && + mkdir -p .git/worktrees/wt-3/rebase-merge && + touch .git/worktrees/wt-3/rebase-merge/interactive && + echo refs/heads/fake1 >.git/worktrees/wt-3/rebase-merge/head-name && + echo refs/heads/fake2 >.git/worktrees/wt-3/rebase-merge/onto && + + git branch -f fake2 && + touch .git/worktrees/wt-4/BISECT_LOG && + echo refs/heads/fake2 >.git/worktrees/wt-4/BISECT_START && + + # First, ensure we prevent writing when only one reason to fail. + 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 && + + # Second, set up duplicate values. + mkdir -p .git/worktrees/wt-4/rebase-merge && + touch .git/worktrees/wt-4/rebase-merge/interactive && + echo refs/heads/fake2 >.git/worktrees/wt-4/rebase-merge/head-name && + echo refs/heads/fake1 >.git/worktrees/wt-4/rebase-merge/onto && + + touch .git/worktrees/wt-1/BISECT_LOG && + echo refs/heads/fake1 >.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