{"thread":{"id":"62127","subject":"[PATCH 0/3] cache-tree: fix segfaults with invalid cache-trees","startedAt":"2024-09-17T07:13:09Z","lastAt":"2024-10-07T04:38:24Z","messageCount":13,"participants":["Patrick Steinhardt","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"502915","messageId":"cover.1726556195.git.ps@pks.im","threadId":"62127","inReplyTo":null,"subject":"[PATCH 0/3] cache-tree: fix segfaults with invalid cache-trees","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T07:13:05Z","receivedAt":"2024-09-17T07:13:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes segfaults that can happen when the index\nhas a corrupted cache-tree extension. This scenario is covered by t4058,\nwhich documented this segfault. I've noitced that this triggers the leak\nsanitizer, so I've decided to fix these segfaults now and handle them\ngracefully.\n\nPatrick\n\nPatrick Steinhardt (3):\n cache-tree: refactor verification to return error codes\n cache-tree: detect mismatching number of index entries\n unpack-trees: detect mismatching number of cache-tree/index entries\n\n cache-tree.c | 102 ++++++++++++++++++++++++++-----------\n cache-tree.h | 2 +-\n read-cache.c | 5 +-\n t/t4058-diff-duplicates.sh | 19 ++++---\n unpack-trees.c | 12 +++--\n 5 files changed, 97 insertions(+), 43 deletions(-)\n\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502916","messageId":"1f13bc0e3259ea9b76f1417303a8ef063f3a7cbe.1726556195.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1726556195.git.ps@pks.im","subject":"[PATCH 1/3] cache-tree: refactor verification to return error codes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T07:13:08Z","receivedAt":"2024-09-17T07:13:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `cache_tree_verify()` will `BUG()` whenever it finds that\nthe cache-tree extension of the index is corrupt. The function is thus\ninherently untestable because the resulting call to `abort()` will be\ndetected by our testing framework and labelled an error. And rightfully\nso: it shouldn't ever be possible to hit bugs, as they should indicate a\nprogramming error rather than corruption of on-disk state.\n\nRefactor the function to instead return error codes. This also ensures\nthat the function can be used e.g. by git-fsck(1) without the whole\nprocess dying.\n\nSigned-off-by: Patrick Steinhardt \n---\n cache-tree.c | 97 +++++++++++++++++++++++++++++++++++---------------\n cache-tree.h | 2 +-\n read-cache.c | 5 +--\n unpack-trees.c | 10 ++++--\n 4 files changed, 79 insertions(+), 35 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 50610c3f3cb..4228b6fad48 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -2,6 +2,7 @@\n \n #include \"git-compat-util.h\"\n #include \"environment.h\"\n+#include \"gettext.h\"\n #include \"hex.h\"\n #include \"lockfile.h\"\n #include \"tree.h\"\n@@ -864,15 +865,15 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n \treturn 0;\n }\n \n-static void verify_one_sparse(struct index_state *istate,\n-\t\t\t struct strbuf *path,\n-\t\t\t int pos)\n+static int verify_one_sparse(struct index_state *istate,\n+\t\t\t struct strbuf *path,\n+\t\t\t int pos)\n {\n \tstruct cache_entry *ce = istate->cache[pos];\n-\n \tif (!S_ISSPARSEDIR(ce->ce_mode))\n-\t\tBUG(\"directory '%s' is present in index, but not sparse\",\n-\t\t path->buf);\n+\t\treturn error(_(\"directory '%s' is present in index, but not sparse\"),\n+\t\t\t path->buf);\n+\treturn 0;\n }\n \n /*\n@@ -881,6 +882,7 @@ static void verify_one_sparse(struct index_state *istate,\n * 1 - Restart verification - a call to ensure_full_index() freed the cache\n * tree that is being verified and verification needs to be restarted from\n * the new toplevel cache tree.\n+ * -1 - Verification failed.\n */\n static int verify_one(struct repository *r,\n \t\t struct index_state *istate,\n@@ -890,18 +892,23 @@ static int verify_one(struct repository *r,\n \tint i, pos, len = path->len;\n \tstruct strbuf tree_buf = STRBUF_INIT;\n \tstruct object_id new_oid;\n+\tint ret;\n \n \tfor (i = 0; i < it->subtree_nr; i++) {\n \t\tstrbuf_addf(path, \"%s/\", it->down[i]->name);\n-\t\tif (verify_one(r, istate, it->down[i]->cache_tree, path))\n-\t\t\treturn 1;\n+\t\tret = verify_one(r, istate, it->down[i]->cache_tree, path);\n+\t\tif (ret)\n+\t\t\tgoto out;\n+\n \t\tstrbuf_setlen(path, len);\n \t}\n \n \tif (it->entry_count < 0 ||\n \t /* no verification on tests (t7003) that replace trees */\n-\t lookup_replace_object(r, &it->oid) != &it->oid)\n-\t\treturn 0;\n+\t lookup_replace_object(r, &it->oid) != &it->oid) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tif (path->len) {\n \t\t/*\n@@ -911,12 +918,14 @@ static int verify_one(struct repository *r,\n \t\t */\n \t\tint is_sparse = istate->sparse_index;\n \t\tpos = index_name_pos(istate, path->buf, path->len);\n-\t\tif (is_sparse && !istate->sparse_index)\n-\t\t\treturn 1;\n+\t\tif (is_sparse && !istate->sparse_index) {\n+\t\t\tret = 1;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\tif (pos >= 0) {\n-\t\t\tverify_one_sparse(istate, path, pos);\n-\t\t\treturn 0;\n+\t\t\tret = verify_one_sparse(istate, path, pos);\n+\t\t\tgoto out;\n \t\t}\n \n \t\tpos = -pos - 1;\n@@ -934,16 +943,23 @@ static int verify_one(struct repository *r,\n \t\tunsigned mode;\n \t\tint entlen;\n \n-\t\tif (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE))\n-\t\t\tBUG(\"%s with flags 0x%x should not be in cache-tree\",\n-\t\t\t ce->name, ce->ce_flags);\n+\t\tif (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) {\n+\t\t\tret = error(_(\"%s with flags 0x%x should not be in cache-tree\"),\n+\t\t\t\t ce->name, ce->ce_flags);\n+\t\t\tgoto out;\n+\t\t}\n+\n \t\tname = ce->name + path->len;\n \t\tslash = strchr(name, '/');\n \t\tif (slash) {\n \t\t\tentlen = slash - name;\n+\n \t\t\tsub = find_subtree(it, ce->name + path->len, entlen, 0);\n-\t\t\tif (!sub || sub->cache_tree->entry_count < 0)\n-\t\t\t\tBUG(\"bad subtree '%.*s'\", entlen, name);\n+\t\t\tif (!sub || sub->cache_tree->entry_count < 0) {\n+\t\t\t\tret = error(_(\"bad subtree '%.*s'\"), entlen, name);\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\n \t\t\toid = &sub->cache_tree->oid;\n \t\t\tmode = S_IFDIR;\n \t\t\ti += sub->cache_tree->entry_count;\n@@ -956,27 +972,50 @@ static int verify_one(struct repository *r,\n \t\tstrbuf_addf(&tree_buf, \"%o %.*s%c\", mode, entlen, name, '\\0');\n \t\tstrbuf_add(&tree_buf, oid->hash, r->hash_algo->rawsz);\n \t}\n+\n \thash_object_file(r->hash_algo, tree_buf.buf, tree_buf.len, OBJ_TREE,\n \t\t\t &new_oid);\n-\tif (!oideq(&new_oid, &it->oid))\n-\t\tBUG(\"cache-tree for path %.*s does not match. \"\n-\t\t \"Expected %s got %s\", len, path->buf,\n-\t\t oid_to_hex(&new_oid), oid_to_hex(&it->oid));\n+\n+\tif (!oideq(&new_oid, &it->oid)) {\n+\t\tret = error(_(\"cache-tree for path %.*s does not match. \"\n+\t\t\t \"Expected %s got %s\"), len, path->buf,\n+\t\t\t oid_to_hex(&new_oid), oid_to_hex(&it->oid));\n+\t\tgoto out;\n+\t}\n+\n+\tret = 0;\n+out:\n \tstrbuf_setlen(path, len);\n \tstrbuf_release(&tree_buf);\n-\treturn 0;\n+\treturn ret;\n }\n \n-void cache_tree_verify(struct repository *r, struct index_state *istate)\n+int cache_tree_verify(struct repository *r, struct index_state *istate)\n {\n \tstruct strbuf path = STRBUF_INIT;\n+\tint ret;\n \n-\tif (!istate->cache_tree)\n-\t\treturn;\n-\tif (verify_one(r, istate, istate->cache_tree, &path)) {\n+\tif (!istate->cache_tree) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n+\n+\tret = verify_one(r, istate, istate->cache_tree, &path);\n+\tif (ret < 0)\n+\t\tgoto out;\n+\tif (ret > 0) {\n \t\tstrbuf_reset(&path);\n-\t\tif (verify_one(r, istate, istate->cache_tree, &path))\n+\n+\t\tret = verify_one(r, istate, istate->cache_tree, &path);\n+\t\tif (ret < 0)\n+\t\t\tgoto out;\n+\t\tif (ret > 0)\n \t\t\tBUG(\"ensure_full_index() called twice while verifying cache tree\");\n \t}\n+\n+\tret = 0;\n+\n+out:\n \tstrbuf_release(&path);\n+\treturn ret;\n }\ndiff --git a/cache-tree.h b/cache-tree.h\nindex faae88be63c..b82c4963e7c 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -33,7 +33,7 @@ struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n int cache_tree_fully_valid(struct cache_tree *);\n int cache_tree_update(struct index_state *, int);\n-void cache_tree_verify(struct repository *, struct index_state *);\n+int cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n #define WRITE_TREE_MISSING_OK 1\ndiff --git a/read-cache.c b/read-cache.c\nindex 4e67dc182e7..d72a3266b6f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3331,8 +3331,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tint new_shared_index, ret, test_split_index_env;\n \tstruct split_index *si = istate->split_index;\n \n-\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\tcache_tree_verify(the_repository, istate);\n+\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0) &&\n+\t cache_tree_verify(the_repository, istate) < 0)\n+\t\treturn -1;\n \n \tif ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {\n \t\tif (flags & COMMIT_LOCK)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9a55cb62040..21cc197d471 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2070,9 +2070,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tif (o->dst_index) {\n \t\tmove_index_extensions(&o->internal.result, o->src_index);\n \t\tif (!ret) {\n-\t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\t\t\tcache_tree_verify(the_repository,\n-\t\t\t\t\t\t &o->internal.result);\n+\t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0) &&\n+\t\t\t cache_tree_verify(the_repository,\n+\t\t\t\t\t &o->internal.result) < 0) {\n+\t\t\t\tret = -1;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\n \t\t\tif (!o->skip_cache_tree_update &&\n \t\t\t !cache_tree_fully_valid(o->internal.result.cache_tree))\n \t\t\t\tcache_tree_update(&o->internal.result,\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502917","messageId":"595693a6420b2571aabd51ed989bedfa0cfa62e2.1726556195.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1726556195.git.ps@pks.im","subject":"[PATCH 2/3] cache-tree: detect mismatching number of index entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T07:13:11Z","receivedAt":"2024-09-17T07:13:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t4058 we have some tests that exercise git-read-tree(1) when used\nwith a tree that contains duplicate entries. While the expectation is\nthat we fail, we ideally should fail gracefully without a segfault.\n\nBut that is not the case: we never check that the number of entries in\nthe cache-tree is less than or equal to the number of entries in the\nindex. This can lead to an out-of-bounds read as we unconditionally\naccess `istate->cache[idx]`, where `idx` is controlled by the number of\ncache-tree entries and the current position therein. The result is a\nsegfault.\n\nFix this segfault by adding a sanity check for the number of index\nentries before dereferencing them.\n\nSigned-off-by: Patrick Steinhardt \n---\n cache-tree.c | 5 +++++\n t/t4058-diff-duplicates.sh | 12 ++++++------\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 4228b6fad48..1e625673086 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -933,6 +933,11 @@ static int verify_one(struct repository *r,\n \t\tpos = 0;\n \t}\n \n+\tif (it->entry_count + pos > istate->cache_nr) {\n+\t\tret = error(_(\"corrupted cache-tree has entries not present in index\"));\n+\t\tgoto out;\n+\t}\n+\n \ti = 0;\n \twhile (i < it->entry_count) {\n \t\tstruct cache_entry *ce = istate->cache[pos + i];\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 2501c89c1c9..3f602adb055 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -132,15 +132,15 @@ test_expect_success 'create a few commits' '\n \trm commit_id up final\n '\n \n-test_expect_failure 'git read-tree does not segfault' '\n-\ttest_when_finished rm .git/index.lock &&\n-\ttest_might_fail git read-tree --reset base\n+test_expect_success 'git read-tree does not segfault' '\n+\ttest_must_fail git read-tree --reset base 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n-test_expect_failure 'reset --hard does not segfault' '\n-\ttest_when_finished rm .git/index.lock &&\n+test_expect_success 'reset --hard does not segfault' '\n \tgit checkout base &&\n-\ttest_might_fail git reset --hard\n+\ttest_must_fail git reset --hard 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n test_expect_failure 'git diff HEAD does not segfault' '\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502918","messageId":"e2ba015ced1f390966c7bae46969d7725e23fec5.1726556195.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1726556195.git.ps@pks.im","subject":"[PATCH 3/3] unpack-trees: detect mismatching number of cache-tree/index entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T07:13:13Z","receivedAt":"2024-09-17T07:13:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as the preceding commit, we unconditionally dereference the index's\ncache entries depending on the number of cache-tree entries, which can\nlead to a segfault when the cache-tree is corrupted. Fix this bug.\n\nThis also makes t4058 pass with the leak sanitizer enabled.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t4058-diff-duplicates.sh | 7 +++++--\n unpack-trees.c | 2 ++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 3f602adb055..18e5ac88c31 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -10,6 +10,8 @@\n # that the diff output isn't wildly unreasonable.\n \n test_description='test tree diff when trees have duplicate entries'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # make_tree_entry \n@@ -143,11 +145,12 @@ test_expect_success 'reset --hard does not segfault' '\n \ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n-test_expect_failure 'git diff HEAD does not segfault' '\n+test_expect_success 'git diff HEAD does not segfault' '\n \tgit checkout base &&\n \tGIT_TEST_CHECK_CACHE_TREE=false &&\n \tgit reset --hard &&\n-\ttest_might_fail git diff HEAD\n+\ttest_must_fail git diff HEAD 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n test_expect_failure 'can switch to another branch when status is empty' '\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 21cc197d471..e10a9d12091 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -808,6 +808,8 @@ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names,\n \n \tif (!o->merge)\n \t\tBUG(\"We need cache-tree to do this optimization\");\n+\tif (nr_entries + pos > o->src_index->cache_nr)\n+\t\treturn error(_(\"corrupted cache-tree has entries not present in index\"));\n \n \t/*\n \t * Do what unpack_callback() and unpack_single_entry() normally\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502959","messageId":"CAPig+cQ472dzChHKgrL_DNRe4KMNHNVxR1Qh3n6M=OgtBkG1+w@mail.gmail.com","threadId":"62127","inReplyTo":"1f13bc0e3259ea9b76f1417303a8ef063f3a7cbe.1726556195.git.ps@pks.im","subject":"Re: [PATCH 1/3] cache-tree: refactor verification to return error codes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-09-17T17:05:50Z","receivedAt":"2024-09-17T17:06:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 17, 2024 at 3:13 AM Patrick Steinhardt wrote:\n> The function `cache_tree_verify()` will `BUG()` whenever it finds that\n> the cache-tree extension of the index is corrupt. The function is thus\n> inherently untestable because the resulting call to `abort()` will be\n> detected by our testing framework and labelled an error. And rightfully\n> so: it shouldn't ever be possible to hit bugs, as they should indicate a\n> programming error rather than corruption of on-disk state.\n>\n> Refactor the function to instead return error codes. This also ensures\n> that the function can be used e.g. by git-fsck(1) without the whole\n> process dying.\n\nMakes sense.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/cache-tree.c b/cache-tree.c\n> @@ -890,18 +892,23 @@ static int verify_one(struct repository *r,\n> struct strbuf tree_buf = STRBUF_INIT;\n> for (i = 0; i < it->subtree_nr; i++) {\n> strbuf_addf(path, \"%s/\", it->down[i]->name);\n> - if (verify_one(r, istate, it->down[i]->cache_tree, path))\n> - return 1;\n> + ret = verify_one(r, istate, it->down[i]->cache_tree, path);\n> + if (ret)\n> + goto out;\n\nAssuming I am understanding correctly that the original code was\nleaking the strbuf by returning early, I was surprised that the commit\nmessage didn't mention that the patch is also fixing the leak.\n(Probably not worth a reroll, though.)\n"},{"id":"502982","messageId":"Zupg_VUWOfa_i7kU@pks.im","threadId":"62127","inReplyTo":"CAPig+cQ472dzChHKgrL_DNRe4KMNHNVxR1Qh3n6M=OgtBkG1+w@mail.gmail.com","subject":"Re: [PATCH 1/3] cache-tree: refactor verification to return error codes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T05:11:31Z","receivedAt":"2024-09-18T05:11:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 01:05:50PM -0400, Eric Sunshine wrote:\n> On Tue, Sep 17, 2024 at 3:13 AM Patrick Steinhardt wrote:\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > diff --git a/cache-tree.c b/cache-tree.c\n> > @@ -890,18 +892,23 @@ static int verify_one(struct repository *r,\n> > struct strbuf tree_buf = STRBUF_INIT;\n> > for (i = 0; i < it->subtree_nr; i++) {\n> > strbuf_addf(path, \"%s/\", it->down[i]->name);\n> > - if (verify_one(r, istate, it->down[i]->cache_tree, path))\n> > - return 1;\n> > + ret = verify_one(r, istate, it->down[i]->cache_tree, path);\n> > + if (ret)\n> > + goto out;\n> \n> Assuming I am understanding correctly that the original code was\n> leaking the strbuf by returning early, I was surprised that the commit\n> message didn't mention that the patch is also fixing the leak.\n> (Probably not worth a reroll, though.)\n\nIt just wasn't my main motivation here, so I forgot to mention it. Added\nit now though, so it's included in case I'll have to reroll. Thanks!\n\nPatrick\n"},{"id":"503047","messageId":"xmqqttec8ly0.fsf@gitster.g","threadId":"62127","inReplyTo":"595693a6420b2571aabd51ed989bedfa0cfa62e2.1726556195.git.ps@pks.im","subject":"Re: [PATCH 2/3] cache-tree: detect mismatching number of index entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-19T01:35:35Z","receivedAt":"2024-09-19T01:35:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +\tif (it->entry_count + pos > istate->cache_nr) {\n> +\t\tret = error(_(\"corrupted cache-tree has entries not present in index\"));\n> +\t\tgoto out;\n> +\t}\n\nIs it a safe assumption that the if() condition always indicates an\nerror? When sparse-index is in effect, istate->cache_nr may be a\nnumber that is smaller than the true number of paths in the index\n(because all paths under a subdirectory we are not interested in are\nfolded into a single tree-ish entry), and I am not sure how it\nshould interact with it->entry_count (i.e. the number of paths under\nthe current directory we are looking at, which obviously cannot be a\nsparsified entry) and pos (i.e. the index into active_cache[] that\nrepresend the first path under the current directory)?\n\nI guess as long as \"it\" is not folded, it does not matter how other\npaths from different directories in active_cache[] are sparsified or\nexpanded, as long as \"pos\" keeps track of the current position\ncorrectly.\n\n> diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\n> index 2501c89c1c9..3f602adb055 100755\n> --- a/t/t4058-diff-duplicates.sh\n> +++ b/t/t4058-diff-duplicates.sh\n> @@ -132,15 +132,15 @@ test_expect_success 'create a few commits' '\n> \trm commit_id up final\n> '\n> \n> -test_expect_failure 'git read-tree does not segfault' '\n> -\ttest_when_finished rm .git/index.lock &&\n> -\ttest_might_fail git read-tree --reset base\n> +test_expect_success 'git read-tree does not segfault' '\n> +\ttest_must_fail git read-tree --reset base 2>err &&\n> +\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n> '\n\nVery good. test_might_fail is a sign of trouble, and this gives us\na lot more predictability.\n"},{"id":"503330","messageId":"ZvJgnqSwANCXmj0G@pks.im","threadId":"62127","inReplyTo":"xmqqttec8ly0.fsf@gitster.g","subject":"Re: [PATCH 2/3] cache-tree: detect mismatching number of index entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T06:48:04Z","receivedAt":"2024-09-24T06:48:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 18, 2024 at 06:35:35PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > +\tif (it->entry_count + pos > istate->cache_nr) {\n> > +\t\tret = error(_(\"corrupted cache-tree has entries not present in index\"));\n> > +\t\tgoto out;\n> > +\t}\n> \n> Is it a safe assumption that the if() condition always indicates an\n> error? When sparse-index is in effect, istate->cache_nr may be a\n> number that is smaller than the true number of paths in the index\n> (because all paths under a subdirectory we are not interested in are\n> folded into a single tree-ish entry), and I am not sure how it\n> should interact with it->entry_count (i.e. the number of paths under\n> the current directory we are looking at, which obviously cannot be a\n> sparsified entry) and pos (i.e. the index into active_cache[] that\n> represend the first path under the current directory)?\n> \n> I guess as long as \"it\" is not folded, it does not matter how other\n> paths from different directories in active_cache[] are sparsified or\n> expanded, as long as \"pos\" keeps track of the current position\n> correctly.\n\nIt seems like we end up calling `ensure_full_index()` for a sparse\nindex, which does cause us to signal to the caller that they should\nrestart verification. So for all I understand, this function shouldn't\nact on a sparsely-populated index.\n\nBut I cannot see how it could lead to anything sensible when the added\ncondition is violated because the first thing we do in the loop is this:\n\n\tstruct cache_entry *ce = istate->cache[pos + i];\n\nAnd before we do anything else, we dereference that pointer. So if the\ncondition doesn't hold we _will_ get an out-of-bounds read of the cache\narray and act on the garbage data. And that causes the observed segfault\non my machine and in the test.\n\nSo I think that ensuring this property is always the right thing to do.\nBut I wouldn't be surprised if overall this code could require more love\nto make it behave sanely in all scenarios. It certainly feels somewhat\nfragile to me.\n\nPatrick\n"},{"id":"503371","messageId":"xmqqed59ugsy.fsf@gitster.g","threadId":"62127","inReplyTo":"ZvJgnqSwANCXmj0G@pks.im","subject":"Re: [PATCH 2/3] cache-tree: detect mismatching number of index entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-24T17:01:33Z","receivedAt":"2024-09-24T17:01:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I guess as long as \"it\" is not folded, it does not matter how other\n>> paths from different directories in active_cache[] are sparsified or\n>> expanded, as long as \"pos\" keeps track of the current position\n>> correctly.\n>\n> It seems like we end up calling `ensure_full_index()` for a sparse\n> index, which does cause us to signal to the caller that they should\n> restart verification. So for all I understand, this function shouldn't\n> act on a sparsely-populated index.\n\nOK. That sounds sensible and safe.\n\n"},{"id":"504241","messageId":"cover.1728275640.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1726556195.git.ps@pks.im","subject":"[PATCH v2 0/3] cache-tree: fix segfaults with invalid cache-trees","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T04:38:12Z","receivedAt":"2024-10-07T04:38:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that fixes segfaults when\nthe index has a corrupted cache-tree extension.\n\nI completely forgot about this series, and it seems to have slipped the\nradar, until I rediscovered some of the segfaults while doing the last\ncouple of leak fixes. So I decided to just resend it with Eric's comment\naddressed, which boils down to a single clarification of one of the\ncommit messages.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n cache-tree: refactor verification to return error codes\n cache-tree: detect mismatching number of index entries\n unpack-trees: detect mismatching number of cache-tree/index entries\n\n cache-tree.c | 102 ++++++++++++++++++++++++++-----------\n cache-tree.h | 2 +-\n read-cache.c | 5 +-\n t/t4058-diff-duplicates.sh | 19 ++++---\n unpack-trees.c | 12 +++--\n 5 files changed, 97 insertions(+), 43 deletions(-)\n\nRange-diff against v1:\n1: 413faa2b81 ! 1: df5a2d0dbc cache-tree: refactor verification to return error codes\n @@ Commit message\n \n Refactor the function to instead return error codes. This also ensures\n that the function can be used e.g. by git-fsck(1) without the whole\n - process dying.\n + process dying. Furthermore, this refactoring plugs some memory leaks\n + when returning early by creating a common exit path.\n \n Signed-off-by: Patrick Steinhardt \n \n2: 4bdcc43518 = 2: d63087c53c cache-tree: detect mismatching number of index entries\n3: fbffeeb6f1 = 3: 5e578c1f41 unpack-trees: detect mismatching number of cache-tree/index entries\n\nbase-commit: 3969d78396e707c5a900dd5e15c365c54bef0283\n-- \n2.47.0.rc0.dirty\n\n"},{"id":"504242","messageId":"df5a2d0dbcdf291d3a0f6e0cc24facf29ad09685.1728275640.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1728275640.git.ps@pks.im","subject":"[PATCH v2 1/3] cache-tree: refactor verification to return error codes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T04:38:15Z","receivedAt":"2024-10-07T04:38:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `cache_tree_verify()` will `BUG()` whenever it finds that\nthe cache-tree extension of the index is corrupt. The function is thus\ninherently untestable because the resulting call to `abort()` will be\ndetected by our testing framework and labelled an error. And rightfully\nso: it shouldn't ever be possible to hit bugs, as they should indicate a\nprogramming error rather than corruption of on-disk state.\n\nRefactor the function to instead return error codes. This also ensures\nthat the function can be used e.g. by git-fsck(1) without the whole\nprocess dying. Furthermore, this refactoring plugs some memory leaks\nwhen returning early by creating a common exit path.\n\nSigned-off-by: Patrick Steinhardt \n---\n cache-tree.c | 97 +++++++++++++++++++++++++++++++++++---------------\n cache-tree.h | 2 +-\n read-cache.c | 5 +--\n unpack-trees.c | 10 ++++--\n 4 files changed, 79 insertions(+), 35 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 50610c3f3c..4228b6fad4 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -2,6 +2,7 @@\n \n #include \"git-compat-util.h\"\n #include \"environment.h\"\n+#include \"gettext.h\"\n #include \"hex.h\"\n #include \"lockfile.h\"\n #include \"tree.h\"\n@@ -864,15 +865,15 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n \treturn 0;\n }\n \n-static void verify_one_sparse(struct index_state *istate,\n-\t\t\t struct strbuf *path,\n-\t\t\t int pos)\n+static int verify_one_sparse(struct index_state *istate,\n+\t\t\t struct strbuf *path,\n+\t\t\t int pos)\n {\n \tstruct cache_entry *ce = istate->cache[pos];\n-\n \tif (!S_ISSPARSEDIR(ce->ce_mode))\n-\t\tBUG(\"directory '%s' is present in index, but not sparse\",\n-\t\t path->buf);\n+\t\treturn error(_(\"directory '%s' is present in index, but not sparse\"),\n+\t\t\t path->buf);\n+\treturn 0;\n }\n \n /*\n@@ -881,6 +882,7 @@ static void verify_one_sparse(struct index_state *istate,\n * 1 - Restart verification - a call to ensure_full_index() freed the cache\n * tree that is being verified and verification needs to be restarted from\n * the new toplevel cache tree.\n+ * -1 - Verification failed.\n */\n static int verify_one(struct repository *r,\n \t\t struct index_state *istate,\n@@ -890,18 +892,23 @@ static int verify_one(struct repository *r,\n \tint i, pos, len = path->len;\n \tstruct strbuf tree_buf = STRBUF_INIT;\n \tstruct object_id new_oid;\n+\tint ret;\n \n \tfor (i = 0; i < it->subtree_nr; i++) {\n \t\tstrbuf_addf(path, \"%s/\", it->down[i]->name);\n-\t\tif (verify_one(r, istate, it->down[i]->cache_tree, path))\n-\t\t\treturn 1;\n+\t\tret = verify_one(r, istate, it->down[i]->cache_tree, path);\n+\t\tif (ret)\n+\t\t\tgoto out;\n+\n \t\tstrbuf_setlen(path, len);\n \t}\n \n \tif (it->entry_count < 0 ||\n \t /* no verification on tests (t7003) that replace trees */\n-\t lookup_replace_object(r, &it->oid) != &it->oid)\n-\t\treturn 0;\n+\t lookup_replace_object(r, &it->oid) != &it->oid) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tif (path->len) {\n \t\t/*\n@@ -911,12 +918,14 @@ static int verify_one(struct repository *r,\n \t\t */\n \t\tint is_sparse = istate->sparse_index;\n \t\tpos = index_name_pos(istate, path->buf, path->len);\n-\t\tif (is_sparse && !istate->sparse_index)\n-\t\t\treturn 1;\n+\t\tif (is_sparse && !istate->sparse_index) {\n+\t\t\tret = 1;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\tif (pos >= 0) {\n-\t\t\tverify_one_sparse(istate, path, pos);\n-\t\t\treturn 0;\n+\t\t\tret = verify_one_sparse(istate, path, pos);\n+\t\t\tgoto out;\n \t\t}\n \n \t\tpos = -pos - 1;\n@@ -934,16 +943,23 @@ static int verify_one(struct repository *r,\n \t\tunsigned mode;\n \t\tint entlen;\n \n-\t\tif (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE))\n-\t\t\tBUG(\"%s with flags 0x%x should not be in cache-tree\",\n-\t\t\t ce->name, ce->ce_flags);\n+\t\tif (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) {\n+\t\t\tret = error(_(\"%s with flags 0x%x should not be in cache-tree\"),\n+\t\t\t\t ce->name, ce->ce_flags);\n+\t\t\tgoto out;\n+\t\t}\n+\n \t\tname = ce->name + path->len;\n \t\tslash = strchr(name, '/');\n \t\tif (slash) {\n \t\t\tentlen = slash - name;\n+\n \t\t\tsub = find_subtree(it, ce->name + path->len, entlen, 0);\n-\t\t\tif (!sub || sub->cache_tree->entry_count < 0)\n-\t\t\t\tBUG(\"bad subtree '%.*s'\", entlen, name);\n+\t\t\tif (!sub || sub->cache_tree->entry_count < 0) {\n+\t\t\t\tret = error(_(\"bad subtree '%.*s'\"), entlen, name);\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\n \t\t\toid = &sub->cache_tree->oid;\n \t\t\tmode = S_IFDIR;\n \t\t\ti += sub->cache_tree->entry_count;\n@@ -956,27 +972,50 @@ static int verify_one(struct repository *r,\n \t\tstrbuf_addf(&tree_buf, \"%o %.*s%c\", mode, entlen, name, '\\0');\n \t\tstrbuf_add(&tree_buf, oid->hash, r->hash_algo->rawsz);\n \t}\n+\n \thash_object_file(r->hash_algo, tree_buf.buf, tree_buf.len, OBJ_TREE,\n \t\t\t &new_oid);\n-\tif (!oideq(&new_oid, &it->oid))\n-\t\tBUG(\"cache-tree for path %.*s does not match. \"\n-\t\t \"Expected %s got %s\", len, path->buf,\n-\t\t oid_to_hex(&new_oid), oid_to_hex(&it->oid));\n+\n+\tif (!oideq(&new_oid, &it->oid)) {\n+\t\tret = error(_(\"cache-tree for path %.*s does not match. \"\n+\t\t\t \"Expected %s got %s\"), len, path->buf,\n+\t\t\t oid_to_hex(&new_oid), oid_to_hex(&it->oid));\n+\t\tgoto out;\n+\t}\n+\n+\tret = 0;\n+out:\n \tstrbuf_setlen(path, len);\n \tstrbuf_release(&tree_buf);\n-\treturn 0;\n+\treturn ret;\n }\n \n-void cache_tree_verify(struct repository *r, struct index_state *istate)\n+int cache_tree_verify(struct repository *r, struct index_state *istate)\n {\n \tstruct strbuf path = STRBUF_INIT;\n+\tint ret;\n \n-\tif (!istate->cache_tree)\n-\t\treturn;\n-\tif (verify_one(r, istate, istate->cache_tree, &path)) {\n+\tif (!istate->cache_tree) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n+\n+\tret = verify_one(r, istate, istate->cache_tree, &path);\n+\tif (ret < 0)\n+\t\tgoto out;\n+\tif (ret > 0) {\n \t\tstrbuf_reset(&path);\n-\t\tif (verify_one(r, istate, istate->cache_tree, &path))\n+\n+\t\tret = verify_one(r, istate, istate->cache_tree, &path);\n+\t\tif (ret < 0)\n+\t\t\tgoto out;\n+\t\tif (ret > 0)\n \t\t\tBUG(\"ensure_full_index() called twice while verifying cache tree\");\n \t}\n+\n+\tret = 0;\n+\n+out:\n \tstrbuf_release(&path);\n+\treturn ret;\n }\ndiff --git a/cache-tree.h b/cache-tree.h\nindex faae88be63..b82c4963e7 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -33,7 +33,7 @@ struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n int cache_tree_fully_valid(struct cache_tree *);\n int cache_tree_update(struct index_state *, int);\n-void cache_tree_verify(struct repository *, struct index_state *);\n+int cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n #define WRITE_TREE_MISSING_OK 1\ndiff --git a/read-cache.c b/read-cache.c\nindex 4e67dc182e..d72a3266b6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3331,8 +3331,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tint new_shared_index, ret, test_split_index_env;\n \tstruct split_index *si = istate->split_index;\n \n-\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\tcache_tree_verify(the_repository, istate);\n+\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0) &&\n+\t cache_tree_verify(the_repository, istate) < 0)\n+\t\treturn -1;\n \n \tif ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {\n \t\tif (flags & COMMIT_LOCK)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9a55cb6204..21cc197d47 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2070,9 +2070,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tif (o->dst_index) {\n \t\tmove_index_extensions(&o->internal.result, o->src_index);\n \t\tif (!ret) {\n-\t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\t\t\tcache_tree_verify(the_repository,\n-\t\t\t\t\t\t &o->internal.result);\n+\t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0) &&\n+\t\t\t cache_tree_verify(the_repository,\n+\t\t\t\t\t &o->internal.result) < 0) {\n+\t\t\t\tret = -1;\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\n \t\t\tif (!o->skip_cache_tree_update &&\n \t\t\t !cache_tree_fully_valid(o->internal.result.cache_tree))\n \t\t\t\tcache_tree_update(&o->internal.result,\n-- \n2.47.0.rc0.dirty\n\n"},{"id":"504244","messageId":"d63087c53c5e57c63fe27a7a7dffa8fdb312f30a.1728275640.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1728275640.git.ps@pks.im","subject":"[PATCH v2 2/3] cache-tree: detect mismatching number of index entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T04:38:18Z","receivedAt":"2024-10-07T04:38:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t4058 we have some tests that exercise git-read-tree(1) when used\nwith a tree that contains duplicate entries. While the expectation is\nthat we fail, we ideally should fail gracefully without a segfault.\n\nBut that is not the case: we never check that the number of entries in\nthe cache-tree is less than or equal to the number of entries in the\nindex. This can lead to an out-of-bounds read as we unconditionally\naccess `istate->cache[idx]`, where `idx` is controlled by the number of\ncache-tree entries and the current position therein. The result is a\nsegfault.\n\nFix this segfault by adding a sanity check for the number of index\nentries before dereferencing them.\n\nSigned-off-by: Patrick Steinhardt \n---\n cache-tree.c | 5 +++++\n t/t4058-diff-duplicates.sh | 12 ++++++------\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 4228b6fad4..1e62567308 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -933,6 +933,11 @@ static int verify_one(struct repository *r,\n \t\tpos = 0;\n \t}\n \n+\tif (it->entry_count + pos > istate->cache_nr) {\n+\t\tret = error(_(\"corrupted cache-tree has entries not present in index\"));\n+\t\tgoto out;\n+\t}\n+\n \ti = 0;\n \twhile (i < it->entry_count) {\n \t\tstruct cache_entry *ce = istate->cache[pos + i];\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 2501c89c1c..3f602adb05 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -132,15 +132,15 @@ test_expect_success 'create a few commits' '\n \trm commit_id up final\n '\n \n-test_expect_failure 'git read-tree does not segfault' '\n-\ttest_when_finished rm .git/index.lock &&\n-\ttest_might_fail git read-tree --reset base\n+test_expect_success 'git read-tree does not segfault' '\n+\ttest_must_fail git read-tree --reset base 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n-test_expect_failure 'reset --hard does not segfault' '\n-\ttest_when_finished rm .git/index.lock &&\n+test_expect_success 'reset --hard does not segfault' '\n \tgit checkout base &&\n-\ttest_might_fail git reset --hard\n+\ttest_must_fail git reset --hard 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n test_expect_failure 'git diff HEAD does not segfault' '\n-- \n2.47.0.rc0.dirty\n\n"},{"id":"504245","messageId":"5e578c1f4161ba5c23fd5b27e792807b632e9c06.1728275640.git.ps@pks.im","threadId":"62127","inReplyTo":"cover.1728275640.git.ps@pks.im","subject":"[PATCH v2 3/3] unpack-trees: detect mismatching number of cache-tree/index entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-07T04:38:21Z","receivedAt":"2024-10-07T04:38:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as the preceding commit, we unconditionally dereference the index's\ncache entries depending on the number of cache-tree entries, which can\nlead to a segfault when the cache-tree is corrupted. Fix this bug.\n\nThis also makes t4058 pass with the leak sanitizer enabled.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t4058-diff-duplicates.sh | 7 +++++--\n unpack-trees.c | 2 ++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 3f602adb05..18e5ac88c3 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -10,6 +10,8 @@\n # that the diff output isn't wildly unreasonable.\n \n test_description='test tree diff when trees have duplicate entries'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # make_tree_entry \n@@ -143,11 +145,12 @@ test_expect_success 'reset --hard does not segfault' '\n \ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n-test_expect_failure 'git diff HEAD does not segfault' '\n+test_expect_success 'git diff HEAD does not segfault' '\n \tgit checkout base &&\n \tGIT_TEST_CHECK_CACHE_TREE=false &&\n \tgit reset --hard &&\n-\ttest_might_fail git diff HEAD\n+\ttest_must_fail git diff HEAD 2>err &&\n+\ttest_grep \"error: corrupted cache-tree has entries not present in index\" err\n '\n \n test_expect_failure 'can switch to another branch when status is empty' '\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 21cc197d47..e10a9d1209 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -808,6 +808,8 @@ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names,\n \n \tif (!o->merge)\n \t\tBUG(\"We need cache-tree to do this optimization\");\n+\tif (nr_entries + pos > o->src_index->cache_nr)\n+\t\treturn error(_(\"corrupted cache-tree has entries not present in index\"));\n \n \t/*\n \t * Do what unpack_callback() and unpack_single_entry() normally\n-- \n2.47.0.rc0.dirty\n\n"}]}