| Message ID | pull.1053.v3.git.1633630041829.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 2f90c87850592ee4673efed4e53f8bc1de58177e |
| Headers | show |
| Series | [v3] sparse index: fix use-after-free bug in cache_tree_verify() | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > * Fixed the spelling of Stolee's name (sorry Stolee) > * Added "-q" to the test to prevent a failure on Microsoft's fork[1] > > [1] > https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ I've seen the exchange, but ... > - for OPERATION in "merge -m merge" cherry-pick rebase > + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" > do ... it looks too strange that only one of them requires a "--quiet" option. Is it a possibility to get whoever's fork corrected so that it behaves sensibly without requiring the "-q" option only for the particular rebase backend? In the meantime, I'll queue the patch as-is (I actually queued the previous round with namefix already). Thanks.
On 07/10/2021 22:23, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> * Fixed the spelling of Stolee's name (sorry Stolee) >> * Added "-q" to the test to prevent a failure on Microsoft's fork[1] >> >> [1] >> https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ > > I've seen the exchange, but ... > >> - for OPERATION in "merge -m merge" cherry-pick rebase >> + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" >> do > > ... it looks too strange that only one of them requires a "--quiet" > option. Is it a possibility to get whoever's fork corrected so that > it behaves sensibly without requiring the "-q" option only for the > particular rebase backend? The issue is caused by a patch that Microsoft is carrying that stops apply from creating paths with the skip-worktree bit set. As they're upstreaming their sparse index and checkout work I expect it will show up on the list sooner or later. I agree the "-q" is odd and it also means the test is weaker but I'm not sure what else we can do. > In the meantime, I'll queue the patch as-is (I actually queued the > previous round with namefix already). Thanks Phillip > Thanks. >
On 08/10/21 01.07, Phillip Wood via GitGitGadget wrote: > -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; What is verify_one() doing? I think it worth mentioning it in the comment above.
On 10/8/2021 5:09 AM, Phillip Wood wrote: > On 07/10/2021 22:23, Junio C Hamano wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> * Fixed the spelling of Stolee's name (sorry Stolee) >>> * Added "-q" to the test to prevent a failure on Microsoft's fork[1] >>> [1] >>> https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ >> >> I've seen the exchange, but ... >> >>> - for OPERATION in "merge -m merge" cherry-pick rebase >>> + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" >>> do >> >> ... it looks too strange that only one of them requires a "--quiet" >> option. Is it a possibility to get whoever's fork corrected so that >> it behaves sensibly without requiring the "-q" option only for the >> particular rebase backend? > > The issue is caused by a patch that Microsoft is carrying that stops apply from creating paths with the skip-worktree bit set. As they're upstreaming their sparse index and checkout work I expect it will show up on the list sooner or later. I agree the "-q" is odd and it also means the test is weaker but I'm not sure what else we can do. That particular patch is old and is due to some interactions with how VFS for Git (ab)uses the skip-worktree bit. I'm not sure it will ever come upstream. It is probably very much like a recent example [1] that we tried to upstream only to realize that it should be replaced with something better. [1] https://lore.kernel.org/git/65905bf4e001118e8b9ced95c1bcecbacb6334ac.1633013461.git.gitgitgadget@gmail.com/ I'm fine to leave the `-q` out of this patch and I can add it myself when we take this into microsoft/git. That can also motivate me to rethink that patch. Thanks, -Stolee
Phillip Wood <phillip.wood123@gmail.com> writes: > On 07/10/2021 22:23, Junio C Hamano wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> * Fixed the spelling of Stolee's name (sorry Stolee) >>> * Added "-q" to the test to prevent a failure on Microsoft's fork[1] >>> [1] >>> https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ >> I've seen the exchange, but ... >> >>> - for OPERATION in "merge -m merge" cherry-pick rebase >>> + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" >>> do >> ... it looks too strange that only one of them requires a "--quiet" >> option. Is it a possibility to get whoever's fork corrected so that >> it behaves sensibly without requiring the "-q" option only for the >> particular rebase backend? > > The issue is caused by a patch that Microsoft is carrying that stops > apply from creating paths with the skip-worktree bit set. As they're > upstreaming their sparse index and checkout work I expect it will show > up on the list sooner or later. I agree the "-q" is odd and it also > means the test is weaker but I'm not sure what else we can do. Perhaps passing "-q" to the other variant of "rebase" would make it clear that (1) we do not want to worry about traces involved in the verbose message generation and (2) there is nothing fishy going on in only one of the "rebase" backends.
Hi Bagas On 08/10/2021 10:38, Bagas Sanjaya wrote: > On 08/10/21 01.07, Phillip Wood via GitGitGadget wrote: >> -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; > > What is verify_one() doing? I think it worth mentioning it in the > comment above. I think it's pretty obvious if you read the code rather than my patch. It is a common pattern in git that a function with "one" in the name is a helper for another similarly named function without the "one". In this case verify_one() is a recursive helper for cache_tree_verify() Best Wishes Phillip
Hi Junio On 08/10/2021 20:57, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> On 07/10/2021 22:23, Junio C Hamano wrote: >>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> * Fixed the spelling of Stolee's name (sorry Stolee) >>>> * Added "-q" to the test to prevent a failure on Microsoft's fork[1] >>>> [1] >>>> https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ >>> I've seen the exchange, but ... >>> >>>> - for OPERATION in "merge -m merge" cherry-pick rebase >>>> + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" >>>> do >>> ... it looks too strange that only one of them requires a "--quiet" >>> option. Is it a possibility to get whoever's fork corrected so that >>> it behaves sensibly without requiring the "-q" option only for the >>> particular rebase backend? >> >> The issue is caused by a patch that Microsoft is carrying that stops >> apply from creating paths with the skip-worktree bit set. As they're >> upstreaming their sparse index and checkout work I expect it will show >> up on the list sooner or later. I agree the "-q" is odd and it also >> means the test is weaker but I'm not sure what else we can do. > > Perhaps passing "-q" to the other variant of "rebase" would make it > clear that (1) we do not want to worry about traces involved in the > verbose message generation and (2) there is nothing fishy going on > in only one of the "rebase" backends. I'm not sure about that. There are really three levels of output from rebase - quiet, normal and verbose. I think passing "-q" suppresses virtually all the output - there is no indication of which commits have been picked. As test appears to be comparing the output of the command for the sparse and non-spare case as a proxy for "it behaves the same for sparse and non-sparse checkouts/indexes" passing "-q" to rebase weakens the test considerably. Stolee indicated [1] that he is happy for us to drop the "-q" for the "--apply" case so I'd be inclined to go back to your corrected version of V2. Best Wishes Phillip [1] https://lore.kernel.org/git/e281c2e2-2044-1a11-e2bc-5ab3ee92c300@gmail.com/
Phillip Wood <phillip.wood123@gmail.com> writes: > I'm not sure about that. There are really three levels of output from > rebase - quiet, normal and verbose. I think passing "-q" suppresses > virtually all the output - there is no indication of which commits > have been picked. As test appears to be comparing the output of the > command for the sparse and non-spare case as a proxy for "it behaves > the same for sparse and non-sparse checkouts/indexes" passing "-q" to > rebase weakens the test considerably. True. Also because the behaviour of "rebase" using different backends are sufficiently different, I no longer consider it a funny inconsistency that one backend has to to use "-q" while the other doesn't. > Stolee indicated [1] that he is > happy for us to drop the "-q" for the "--apply" case so I'd be > inclined to go back to your corrected version of V2. OK. Can we have a v4 that is identical to "corrected" v2, then, please? That's easier than having to dig v2 up and remember and apply the "correction" ;-). Thanks.
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..80c77bb432e 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 -q" "rebase --merge" do test_all_match git checkout -B temp update-deep && test_all_match git $OPERATION update-folder1 &&