| Message ID | pull.1053.v4.git.1634375229338.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e058b1846c3b2051aab364d7b80e8c1696958a48 |
| Headers | show |
| Series | [v4] sparse index: fix use-after-free bug in cache_tree_verify() | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > [RFC] sparse index: fix use-after-free bug in cache_tree_verify() > > Changes since V3 > > * removed "-q" from the test [1]. This is the same as V2 with a typo > fixed in the commit message > > [1] https://lore.kernel.org/git/ > e281c2e2-2044-1a11-e2bc-5ab3ee92c300@gmail.com/ Thanks. Unfortunately I've already merged the previosu version on the 11th, so I took the liberty of turning this round into an incremental. How does this look? ----- >8 --------- >8 --------- >8 --------- >8 ----- From: Phillip Wood <phillip.wood@dunelm.org.uk> Date: Sat, 16 Oct 2021 09:07:09 +0000 Subject: [PATCH] t1092: run "rebase --apply" without "-q" in the test We run a few Git subcommands and make sure they produce identical results with and without sparse-index. To this set of subcommands, an earlier commit added "rebase --apply", but did so with the "-q" option, in order to work around a breakge caused by a version used at Microsoft with some unreleased changes. Because we would want to make sure the commands produce indentical results, including reports given to the output that lists which commits were picked, use of "-q" loses too much interesting information. Let's drop "-q" from the command invocation and revisit the issue when the problematic changes are upstreamed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t1092-sparse-checkout-compatibility.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 80c77bb432..85d5279b33 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 --apply -q" "rebase --merge" + 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 &&
On 10/17/2021 1:38 AM, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> [RFC] sparse index: fix use-after-free bug in cache_tree_verify() >> >> Changes since V3 >> >> * removed "-q" from the test [1]. This is the same as V2 with a typo >> fixed in the commit message >> >> [1] https://lore.kernel.org/git/ >> e281c2e2-2044-1a11-e2bc-5ab3ee92c300@gmail.com/ > > Thanks. Unfortunately I've already merged the previosu version on > the 11th, so I took the liberty of turning this round into an > incremental. How does this look? > > ----- >8 --------- >8 --------- >8 --------- >8 ----- > From: Phillip Wood <phillip.wood@dunelm.org.uk> > Date: Sat, 16 Oct 2021 09:07:09 +0000 > Subject: [PATCH] t1092: run "rebase --apply" without "-q" in the test > > We run a few Git subcommands and make sure they produce identical > results with and without sparse-index. To this set of subcommands, > an earlier commit added "rebase --apply", but did so with the "-q" > option, in order to work around a breakge caused by a version used s/breakge/breakage/ > at Microsoft with some unreleased changes. > > Because we would want to make sure the commands produce indentical s/indentical/identical/ > results, including reports given to the output that lists which > commits were picked, use of "-q" loses too much interesting > information. Let's drop "-q" from the command invocation and > revisit the issue when the problematic changes are upstreamed. I think this summarizes the situation quite well. Thanks. -Stolee
Hi Junio On 17/10/2021 06:38, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> [RFC] sparse index: fix use-after-free bug in cache_tree_verify() >> >> Changes since V3 >> >> * removed "-q" from the test [1]. This is the same as V2 with a typo >> fixed in the commit message >> >> [1] https://lore.kernel.org/git/ >> e281c2e2-2044-1a11-e2bc-5ab3ee92c300@gmail.com/ > > Thanks. Unfortunately I've already merged the previosu version on > the 11th, Oh sorry I'd missed that so I took the liberty of turning this round into an > incremental. How does this look? It looks fine to me with Stolee's typo fixes Thanks Phillip > ----- >8 --------- >8 --------- >8 --------- >8 ----- > From: Phillip Wood <phillip.wood@dunelm.org.uk> > Date: Sat, 16 Oct 2021 09:07:09 +0000 > Subject: [PATCH] t1092: run "rebase --apply" without "-q" in the test > > We run a few Git subcommands and make sure they produce identical > results with and without sparse-index. To this set of subcommands, > an earlier commit added "rebase --apply", but did so with the "-q" > option, in order to work around a breakge caused by a version used > at Microsoft with some unreleased changes. > > Because we would want to make sure the commands produce indentical > results, including reports given to the output that lists which > commits were picked, use of "-q" loses too much interesting > information. Let's drop "-q" from the command invocation and > revisit the issue when the problematic changes are upstreamed. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Helped-by: Derrick Stolee <dstolee@microsoft.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/t1092-sparse-checkout-compatibility.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 80c77bb432..85d5279b33 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 --apply -q" "rebase --merge" > + 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 && >
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 &&