| Message ID | pull.1053.v2.git.1633600244854.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] sparse index: fix use-after-free bug in cache_tree_verify() | expand |
On 10/7/2021 5:50 AM, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > In a sparse index it is possible for the tree that is being verified > to be freed while it is being verified. This happens when the index is > sparse but the cache tree is not and index_name_pos() looks up a path > from the cache tree that is a descendant of a sparse index entry. That > triggers a call to ensure_full_index() which frees the cache tree that > is being verified. Carrying on trying to verify the tree after this > results in a use-after-free bug. Instead restart the verification if a > sparse index is converted to a full index. This bug is triggered by a > call to reset_head() in "git rebase --apply". Thanks to René Scharfe > and Derick Stolee for their help analyzing the problem. nit: s/Derick/Derrick/ Otherwise, this version looks good to me. Thanks for putting the last bit of polish on it. I'm taking this patch into our microsoft/git fork as we speak [1]. [1] https://github.com/microsoft/git/pull/439 Thanks, -Stolee
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. Thanks, -Stolee
Hi Stolee On 07/10/2021 14:35, Derrick Stolee wrote: > On 10/7/2021 5:50 AM, Phillip Wood via GitGitGadget wrote: >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> In a sparse index it is possible for the tree that is being verified >> to be freed while it is being verified. This happens when the index is >> sparse but the cache tree is not and index_name_pos() looks up a path >> from the cache tree that is a descendant of a sparse index entry. That >> triggers a call to ensure_full_index() which frees the cache tree that >> is being verified. Carrying on trying to verify the tree after this >> results in a use-after-free bug. Instead restart the verification if a >> sparse index is converted to a full index. This bug is triggered by a >> call to reset_head() in "git rebase --apply". Thanks to René Scharfe >> and Derick Stolee for their help analyzing the problem. > > nit: s/Derick/Derrick/ Sorry, maybe Junio can tweak that when he applies the patch, if not I'll fix it. > Otherwise, this version looks good to me. Thanks for putting the last > bit of polish on it. > > I'm taking this patch into our microsoft/git fork as we speak [1]. > > [1] https://github.com/microsoft/git/pull/439 That's nice to know, Thanks Phillip
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? Best Wishes Phillip > Thanks, > -Stolee >
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: + 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
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 <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 >
diff --git a/cache-tree.c b/cache-tree.c index 90919f9e345..8044e21bcf3 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -826,10 +826,17 @@ static void verify_one_sparse(struct repository *r, path->buf); } -static void verify_one(struct repository *r, - struct index_state *istate, - struct cache_tree *it, - struct strbuf *path) +/* + * Returns: + * 0 - Verification completed. + * 1 - Restart verification - a call to ensure_full_index() freed the cache + * tree that is being verified and verification needs to be restarted from + * the new toplevel cache tree. + */ +static int verify_one(struct repository *r, + struct index_state *istate, + struct cache_tree *it, + struct strbuf *path) { int i, pos, len = path->len; struct strbuf tree_buf = STRBUF_INIT; @@ -837,21 +844,30 @@ static void verify_one(struct repository *r, for (i = 0; i < it->subtree_nr; i++) { strbuf_addf(path, "%s/", it->down[i]->name); - verify_one(r, istate, it->down[i]->cache_tree, path); + if (verify_one(r, istate, it->down[i]->cache_tree, path)) + return 1; strbuf_setlen(path, len); } if (it->entry_count < 0 || /* no verification on tests (t7003) that replace trees */ lookup_replace_object(r, &it->oid) != &it->oid) - return; + return 0; if (path->len) { + /* + * If the index is sparse and the cache tree is not + * index_name_pos() may trigger ensure_full_index() which will + * free the tree that is being verified. + */ + int is_sparse = istate->sparse_index; pos = index_name_pos(istate, path->buf, path->len); + if (is_sparse && !istate->sparse_index) + return 1; if (pos >= 0) { verify_one_sparse(r, istate, it, path, pos); - return; + return 0; } pos = -pos - 1; @@ -899,6 +915,7 @@ static void verify_one(struct repository *r, oid_to_hex(&new_oid), oid_to_hex(&it->oid)); strbuf_setlen(path, len); strbuf_release(&tree_buf); + return 0; } void cache_tree_verify(struct repository *r, struct index_state *istate) @@ -907,6 +924,10 @@ void cache_tree_verify(struct repository *r, struct index_state *istate) if (!istate->cache_tree) return; - verify_one(r, istate, istate->cache_tree, &path); + if (verify_one(r, istate, istate->cache_tree, &path)) { + strbuf_reset(&path); + if (verify_one(r, istate, istate->cache_tree, &path)) + BUG("ensure_full_index() called twice while verifying cache tree"); + } strbuf_release(&path); } 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" do test_all_match git checkout -B temp update-deep && test_all_match git $OPERATION update-folder1 &&