Re: [PATCH v2] sparse index: fix use-after-free bug in cache_tree_verify()

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 7, 2021, 17:59 UTC
Message-ID: <ebbe8616-0863-812b-e112-103680f7298b@gmail.com>
In-Reply-To: <69f83ec7-315e-0488-774c-bb8490001b13@gmail.com>

Hi Stolee

On 07/10/2021 16:44, Derrick Stolee wrote:

Show 25 quoted lines

> 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 <phillip.wood@dunelm.org.uk>
>>> ...
>>>> 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
>