From: Phillip Wood Date: Thu, 07 Oct 2021 17:59:41 GMT Subject: Re: [PATCH v2] sparse index: fix use-after-free bug in cache_tree_verify() Message-ID: In-Reply-To: <69f83ec7-315e-0488-774c-bb8490001b13@gmail.com> Hi Stolee On 07/10/2021 16:44, Derrick Stolee wrote: > On 10/7/2021 11:05 AM, Phillip Wood wrote: >> Hi Stolee >> >> On 07/10/2021 14:53, Derrick Stolee wrote: >>> On 10/7/2021 5:50 AM, Phillip Wood via GitGitGadget wrote: >>>> From: Phillip Wood >>> ... >>>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh >>>> index 886e78715fe..85d5279b33c 100755 >>>> --- a/t/t1092-sparse-checkout-compatibility.sh >>>> +++ b/t/t1092-sparse-checkout-compatibility.sh >>>> @@ -484,7 +484,7 @@ test_expect_success 'checkout and reset (mixed) [sparse]' ' >>>>   test_expect_success 'merge, cherry-pick, and rebase' ' >>>>       init_repos && >>>>   -    for OPERATION in "merge -m merge" cherry-pick rebase >>>> +    for OPERATION in "merge -m merge" cherry-pick "rebase --apply" "rebase --merge" >>> >>> I spoke too soon. On my machine, the 'git rebase --apply' tests fail >>> because of some verbose output that does not match across the full >>> and sparse cases. Using "rebase -q --apply" works for me. >> >> Oh, that's strange, the CI tests pass on gitgitgadget and that script passes locally for me. Do you know what the output is that does not match? > > It's entirely possible that it's something in git-for-windows/git or > microsoft/git that is causing the difference: Yes, if I apply the hunk below from 044d9fdaeb ("sparse-checkout: avoid writing entries with the skip-worktree bit", 2017-03-01) which is in microsoft/vfs-2.33.0 to my fix then I see the same failure. It looks like this change makes the apply back end fall back to a three way merge where a simple patch application succeeded before. Adding "-q" to the test feels like a bit of a hack but it's probably the best we can do - at least it still catches any crashes. Best Wishes Phillip diff --git a/apply.c b/apply.c index 43a0aebf4e..4c1ca6d360 100644 --- a/apply.c +++ b/apply.c @@ -3346,6 +3345,24 @@ static int checkout_target(struct index_state *istate, { struct checkout costate = CHECKOUT_INIT; + /* + * Do not checkout the entry if the skipworktree bit is set + * + * Both callers of this method (check_preimage and load_current) + * check for the existance of the file before calling this + * method so we know that the file doesn't exist at this point + * and we don't need to perform that check again here. + * We just need to check the skip-worktree and return. + * + * This is to prevent git from creating a file in the + * working directory that has the skip-worktree bit on, + * then updating the index from the patch and not keeping + * the working directory version up to date with what it + * changed the index version to be. + */ + if (ce_skip_worktree(ce)) + return 0; + costate.refresh_cache = 1; costate.istate = istate; if (checkout_entry(ce, &costate, NULL, NULL) || > + diff -u full-checkout-out sparse-checkout-out > --- full-checkout-out 2021-10-07 13:37:00.475394970 +0000 > +++ sparse-checkout-out 2021-10-07 13:37:00.531396095 +0000 > @@ -1,3 +1,10 @@ > First, rewinding head to replay your work on top of it... > Applying: update folder1 > +Using index info to reconstruct a base tree... > +Falling back to patching base and 3-way merge... > +Merging: > +e1886b3 update folder2 > +virtual update folder1 > +found 1 common ancestor: > +virtual b4ad7e16921c16e36f1d5d45ea4fa186efa8422a > Applying: update deep > + return 1 > error: last command exited with $?=1 > > [1] https://github.com/microsoft/git/runs/3827705316?check_suite_focus=true#step:5:10469 > > Thanks, > -Stolee >