{"thread":{"id":"65044","subject":"[PATCH] fsck: do not loop infinitely when processing packs","startedAt":"2026-02-22T18:37:24Z","lastAt":"2026-02-24T22:32:08Z","messageCount":14,"participants":["brian m. carlson","Junio C Hamano","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"536658","messageId":"20260222183710.2963424-1-sandals@crustytoothpaste.net","threadId":"65044","inReplyTo":null,"subject":"[PATCH] fsck: do not loop infinitely when processing packs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-22T18:37:10Z","receivedAt":"2026-02-22T18:37:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When we iterate over our packfiles in the fsck code, we do so twice.\nThe first time, we count the number of objects in all of the packs\ntogether and later on, we iterate a second time, processing each pack\nand verifying its integrity.\n\nThis would normally work fine, but if we have two packs and we're\nprocessing the second, the verification process will open the pack to\nread from it, which will place it at the beginning of the most recently\nused list.  Since this same list is used for iteration, the pack we most\nrecently processed before this will then be behind the current pack in\nthe linked list, so when we next process the list, we will go back to\nthe first pack again and then loop forever.  This also makes our\nprogress indicator loop up to many thousands of percent, which is not\nonly nonsensical, but a clear indication that something has gone wrong.\n\nSolve this by skipping our MRU updates when we're iterating over\npackfiles, which avoids the reordering that causes problems.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nI realize that t1050 may seem like a bizarre place to put this test.\nHowever, I was debugging my sha256-interop branch and why the final test\ncalling `git fsck` was failing, so I placed a `git fsck` earlier in the\ntest to double-check and discovered the problem.  Since we already have\na natural testcase here, I thought I'd just place the test where we\nalready know it will trigger the problem.\n\n packfile.h       | 16 ++++++++++++++--\n t/t1050-large.sh |  4 ++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/packfile.h b/packfile.h\nindex acc5c55ad5..086d98c1a0 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -183,6 +183,7 @@ struct packfile_list_entry *packfile_store_get_packs(struct packfile_store *stor\n struct repo_for_each_pack_data {\n \tstruct odb_source *source;\n \tstruct packfile_list_entry *entry;\n+\tstruct repository *repo;\n };\n \n static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct repository *repo)\n@@ -191,8 +192,13 @@ static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct\n \n \todb_prepare_alternates(repo->objects);\n \n+\tdata.repo = repo;\n+\n \tfor (struct odb_source *source = repo->objects->sources; source; source = source->next) {\n-\t\tstruct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);\n+\t\tstruct packfile_list_entry *entry;\n+\n+\t\tsource->packfiles->skip_mru_updates = true;\n+\t\tentry = packfile_store_get_packs(source->packfiles);\n \t\tif (!entry)\n \t\t\tcontinue;\n \t\tdata.source = source;\n@@ -212,7 +218,10 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *\n \t\treturn;\n \n \tfor (source = data->source->next; source; source = source->next) {\n-\t\tstruct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);\n+\t\tstruct packfile_list_entry *entry;\n+\n+\t\tsource->packfiles->skip_mru_updates = true;\n+\t\tentry = packfile_store_get_packs(source->packfiles);\n \t\tif (!entry)\n \t\t\tcontinue;\n \t\tdata->source = source;\n@@ -220,6 +229,9 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *\n \t\treturn;\n \t}\n \n+\tfor (struct odb_source *source = data->repo->objects->sources; source; source = source->next)\n+\t\tsource->packfiles->skip_mru_updates = false;\n+\n \tdata->source = NULL;\n \tdata->entry = NULL;\n }\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 5be273611a..75e75e627c 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -160,6 +160,10 @@ test_expect_success 'hash-object' '\n \tgit hash-object large1\n '\n \n+test_expect_success 'fsck does not loop forever' '\n+\tgit fsck\n+'\n+\n test_expect_success 'cat-file a large file' '\n \tgit cat-file blob :large1 >/dev/null\n '\n"},{"id":"536673","messageId":"xmqqv7fopflu.fsf@gitster.g","threadId":"65044","inReplyTo":"20260222183710.2963424-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-22T22:39:57Z","receivedAt":"2026-02-22T22:39:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> diff --git a/t/t1050-large.sh b/t/t1050-large.sh\n> index 5be273611a..75e75e627c 100755\n> --- a/t/t1050-large.sh\n> +++ b/t/t1050-large.sh\n> @@ -160,6 +160,10 @@ test_expect_success 'hash-object' '\n>  \tgit hash-object large1\n>  '\n>  \n> +test_expect_success 'fsck does not loop forever' '\n> +\tgit fsck\n> +'\n> +\n>  test_expect_success 'cat-file a large file' '\n>  \tgit cat-file blob :large1 >/dev/null\n>  '\n\nWow, this is a fun test ;-).\n\nThanks.  Will queue.\n\n"},{"id":"536674","messageId":"aZuMPcMYwFi4Sch5@fruit.crustytoothpaste.net","threadId":"65044","inReplyTo":"xmqqv7fopflu.fsf@gitster.g","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-22T23:07:41Z","receivedAt":"2026-02-22T23:07:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-02-22 at 22:39:57, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > diff --git a/t/t1050-large.sh b/t/t1050-large.sh\n> > index 5be273611a..75e75e627c 100755\n> > --- a/t/t1050-large.sh\n> > +++ b/t/t1050-large.sh\n> > @@ -160,6 +160,10 @@ test_expect_success 'hash-object' '\n> >  \tgit hash-object large1\n> >  '\n> >  \n> > +test_expect_success 'fsck does not loop forever' '\n> > +\tgit fsck\n> > +'\n> > +\n> >  test_expect_success 'cat-file a large file' '\n> >  \tgit cat-file blob :large1 >/dev/null\n> >  '\n> \n> Wow, this is a fun test ;-).\n> \n> Thanks.  Will queue.\n\nI noticed that the code here seems to have come in with the 2.53 cycle,\nso we may want to cherry-pick it to `maint` at some point if it seems\nlike the problem occurs often.  From what I can tell, it only occurs\nwhen one explicitly invokes `git fsck`[0] and not on transfer, so it\nshouldn't cause a DoS against server implementations.\n\nOf course, we should wait for Patrick, who authored this code, to chime\nin and lend his expertise here.  I must admit I'm not very familiar with\nthis area, although I had recently seen the MRU code when working on\npack index v3 (and then I thought, \"is this actually the problem?\").\n\n[0] The code I saw is the `if (check_full)` branch in `cmd_fsck`, which\nis obviously only invoked by the `fsck` command itself.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"536694","messageId":"20260223071215.GA136463@coredump.intra.peff.net","threadId":"65044","inReplyTo":"aZuMPcMYwFi4Sch5@fruit.crustytoothpaste.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T07:12:15Z","receivedAt":"2026-02-23T07:12:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 22, 2026 at 11:07:41PM +0000, brian m. carlson wrote:\n\n> I noticed that the code here seems to have come in with the 2.53 cycle,\n> so we may want to cherry-pick it to `maint` at some point if it seems\n> like the problem occurs often.  From what I can tell, it only occurs\n> when one explicitly invokes `git fsck`[0] and not on transfer, so it\n> shouldn't cause a DoS against server implementations.\n> \n> Of course, we should wait for Patrick, who authored this code, to chime\n> in and lend his expertise here.  I must admit I'm not very familiar with\n> this area, although I had recently seen the MRU code when working on\n> pack index v3 (and then I thought, \"is this actually the problem?\").\n\nThe problem seems to bisect to c31bad4f7d (packfile: track packs via the\nMRU list exclusively, 2025-10-30), which is not terribly surprising, as\nit was one of the known risks of collapsing the two lists into one.\n\nYour solution is using the tool provided by that commit for its edge\ncase:\n\n    Note that there is one important edge case: `for_each_packed_object()`\n    uses the MRU list to iterate through packs, and then it lists each\n    object in those packs. This would have the effect that we now sort the\n    current pack towards the front, thus modifying the list of packfiles we\n    are iterating over, with the consequence that we'll see an infinite\n    loop. This edge case is worked around by introducing a new field that\n    allows us to skip updating the MRU.\n\nSo in that sense it is the right thing. But it really makes me wonder if\nwe are going back to keeping two lists (one MRU and one in some stable\norder). Or at the very least providing _some_ iteration method that is\nguaranteed to be stable (whether a linked list or a function), so that\niterating code is not subject to this subtle dependency by default.\n\nHaving to identify each potential spot and set a \"btw, don't switch the\npack list order!\" flag seems error-prone. And also loses efficiency when\nyou are iterating a pack and accessing objects in it (since we can't\npush that pack to the front of the MRU then, even though we'd expect\nthere to be high locality with our iteration).\n\n-Peff\n"},{"id":"536709","messageId":"aZwTPfmyrFp-QAPq@pks.im","threadId":"65044","inReplyTo":"20260222183710.2963424-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T08:43:41Z","receivedAt":"2026-02-23T08:43:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 22, 2026 at 06:37:10PM +0000, brian m. carlson wrote:\n> When we iterate over our packfiles in the fsck code, we do so twice.\n> The first time, we count the number of objects in all of the packs\n> together and later on, we iterate a second time, processing each pack\n> and verifying its integrity.\n> \n> This would normally work fine, but if we have two packs and we're\n> processing the second, the verification process will open the pack to\n> read from it, which will place it at the beginning of the most recently\n> used list.  Since this same list is used for iteration, the pack we most\n> recently processed before this will then be behind the current pack in\n> the linked list, so when we next process the list, we will go back to\n> the first pack again and then loop forever.  This also makes our\n> progress indicator loop up to many thousands of percent, which is not\n> only nonsensical, but a clear indication that something has gone wrong.\n> \n> Solve this by skipping our MRU updates when we're iterating over\n> packfiles, which avoids the reordering that causes problems.\n\nRight, this makes sense. We know that we cannot modify the list of packs\nin case we're iterating through them, so `repo_for_each_pack()` should\nindeed skip the MRU updates.\n\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> I realize that t1050 may seem like a bizarre place to put this test.\n> However, I was debugging my sha256-interop branch and why the final test\n> calling `git fsck` was failing, so I placed a `git fsck` earlier in the\n> test to double-check and discovered the problem.  Since we already have\n> a natural testcase here, I thought I'd just place the test where we\n> already know it will trigger the problem.\n\nMakes me wonder though why none of the tests t1450-fsck exhibit this\npattern. I cannot imagine that there is no test there that doesn't have\nmultiple packs. *goes checking* We actually might not, but when trying\nto come up with a minimum reproducer I failed at first.\n\nThis is because ultimately the root cause seems to be a bit more\ncomplex: we don't only care about there being multiple packfiles. We\nalso care about \"core.bigFileThreshold\".\n\nTypically, we don't execute `find_pack_entry()` at all when verifying\npackfiles as we iterate through objects in packfile order. We thus don't\nhave to look up objects via their object ID, but instead we do so by\nusing their packfile offset. And this mechanism will not end up in\n`find_pack_entry()`, and thus we wouldn't update the MRU.\n\nBut there's an exception: when the size of the object that is to be\nchecked exceeds \"core.bigFileThreshold\" we won't read it directly, but\nwe'll instead use `stream_object_signature()`, which eventually ends up\ncalling `odb_read_stream_open()`. And that of course _will_ call\n`find_pack_entry()`, as we're now in the mode where we search by object\nID, not by offset. And consequently, we'll update the MRU in this call\npath.\n\nWith that knowledge it's kind of easy to reproduce the issue: we simply\nneed two packfiles, and each of them must contain at least one blob that\nis larger than \"core.bigFileThreshold\".\n\nNow I agree that the below proposed fix would be a good change to make\nthe code more solid while we still have `repo_for_each_pack()` (I plan\nto eventually get rid of it). But arguably, the above logic is kind of\nbroken regardless of this: we are asked to verify objects in the current\npack, but we may end up verifying the object via a different pack. So if\nthe same object were to exist in multiple packs, we might end up only\nverifying one of its instances.\n\nI've got a couple patches in the making that'll fix this.\n\n> diff --git a/packfile.h b/packfile.h\n> index acc5c55ad5..086d98c1a0 100644\n> --- a/packfile.h\n> +++ b/packfile.h\n> @@ -191,8 +192,13 @@ static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct\n>  \n>  \todb_prepare_alternates(repo->objects);\n>  \n> +\tdata.repo = repo;\n> +\n>  \tfor (struct odb_source *source = repo->objects->sources; source; source = source->next) {\n> -\t\tstruct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);\n> +\t\tstruct packfile_list_entry *entry;\n> +\n> +\t\tsource->packfiles->skip_mru_updates = true;\n> +\t\tentry = packfile_store_get_packs(source->packfiles);\n>  \t\tif (!entry)\n>  \t\t\tcontinue;\n>  \t\tdata.source = source;\n> @@ -212,7 +218,10 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *\n>  \t\treturn;\n>  \n>  \tfor (source = data->source->next; source; source = source->next) {\n> -\t\tstruct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);\n> +\t\tstruct packfile_list_entry *entry;\n> +\n> +\t\tsource->packfiles->skip_mru_updates = true;\n> +\t\tentry = packfile_store_get_packs(source->packfiles);\n>  \t\tif (!entry)\n>  \t\t\tcontinue;\n>  \t\tdata->source = source;\n> @@ -220,6 +229,9 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *\n>  \t\treturn;\n>  \t}\n>  \n> +\tfor (struct odb_source *source = data->repo->objects->sources; source; source = source->next)\n> +\t\tsource->packfiles->skip_mru_updates = false;\n> +\n>  \tdata->source = NULL;\n>  \tdata->entry = NULL;\n>  }\n\nI still think that this hardening here would be worth it, but it has the\nproblem that we won't reset the value in case the caller breaks out of\nthe loop by themselves. I don't really have a good idea for how to fix\nthis, except by turning it into a function with a callback.\n\nPatrick\n"},{"id":"536712","messageId":"aZwTyLMWbcXWnYhQ@pks.im","threadId":"65044","inReplyTo":"20260223071215.GA136463@coredump.intra.peff.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T08:46:00Z","receivedAt":"2026-02-23T08:46:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 02:12:15AM -0500, Jeff King wrote:\n> On Sun, Feb 22, 2026 at 11:07:41PM +0000, brian m. carlson wrote:\n> \n> > I noticed that the code here seems to have come in with the 2.53 cycle,\n> > so we may want to cherry-pick it to `maint` at some point if it seems\n> > like the problem occurs often.  From what I can tell, it only occurs\n> > when one explicitly invokes `git fsck`[0] and not on transfer, so it\n> > shouldn't cause a DoS against server implementations.\n> > \n> > Of course, we should wait for Patrick, who authored this code, to chime\n> > in and lend his expertise here.  I must admit I'm not very familiar with\n> > this area, although I had recently seen the MRU code when working on\n> > pack index v3 (and then I thought, \"is this actually the problem?\").\n> \n> The problem seems to bisect to c31bad4f7d (packfile: track packs via the\n> MRU list exclusively, 2025-10-30), which is not terribly surprising, as\n> it was one of the known risks of collapsing the two lists into one.\n> \n> Your solution is using the tool provided by that commit for its edge\n> case:\n> \n>     Note that there is one important edge case: `for_each_packed_object()`\n>     uses the MRU list to iterate through packs, and then it lists each\n>     object in those packs. This would have the effect that we now sort the\n>     current pack towards the front, thus modifying the list of packfiles we\n>     are iterating over, with the consequence that we'll see an infinite\n>     loop. This edge case is worked around by introducing a new field that\n>     allows us to skip updating the MRU.\n> \n> So in that sense it is the right thing. But it really makes me wonder if\n> we are going back to keeping two lists (one MRU and one in some stable\n> order). Or at the very least providing _some_ iteration method that is\n> guaranteed to be stable (whether a linked list or a function), so that\n> iterating code is not subject to this subtle dependency by default.\n> \n> Having to identify each potential spot and set a \"btw, don't switch the\n> pack list order!\" flag seems error-prone. And also loses efficiency when\n> you are iterating a pack and accessing objects in it (since we can't\n> push that pack to the front of the MRU then, even though we'd expect\n> there to be high locality with our iteration).\n\nAs pointed out in [1] the root cause is actually something different,\nand we merely expose this now with the MRU-based iteration. But I\nwouldn't mind if we eventually switched back to maintaining two lists,\nor finding a different way for how to maintain the iteration order.\n\nPatrick\n\n[1]: <aZwTPfmyrFp-QAPq@pks.im>\n"},{"id":"536722","messageId":"20260223092523.GA209277@coredump.intra.peff.net","threadId":"65044","inReplyTo":"aZwTyLMWbcXWnYhQ@pks.im","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T09:25:23Z","receivedAt":"2026-02-23T09:25:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:\n\n> As pointed out in [1] the root cause is actually something different,\n> and we merely expose this now with the MRU-based iteration. But I\n> wouldn't mind if we eventually switched back to maintaining two lists,\n> or finding a different way for how to maintain the iteration order.\n\nMaybe I don't understand what you're saying, but isn't the root cause\nthe same?\n\nCode is iterating the list, and then during that iteration calls\nfind_pack_entry(). The fact that fsck only calls find_pack_entry() in\nsome subset of cases is immaterial, I'd think. The risk is always there\nwhen iterating now.\n\n-Peff\n"},{"id":"536723","messageId":"20260223092749.GA209358@coredump.intra.peff.net","threadId":"65044","inReplyTo":"aZwTPfmyrFp-QAPq@pks.im","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T09:27:49Z","receivedAt":"2026-02-23T09:27:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 09:43:41AM +0100, Patrick Steinhardt wrote:\n\n> This is because ultimately the root cause seems to be a bit more\n> complex: we don't only care about there being multiple packfiles. We\n> also care about \"core.bigFileThreshold\".\n> \n> Typically, we don't execute `find_pack_entry()` at all when verifying\n> packfiles as we iterate through objects in packfile order. We thus don't\n> have to look up objects via their object ID, but instead we do so by\n> using their packfile offset. And this mechanism will not end up in\n> `find_pack_entry()`, and thus we wouldn't update the MRU.\n> \n> But there's an exception: when the size of the object that is to be\n> checked exceeds \"core.bigFileThreshold\" we won't read it directly, but\n> we'll instead use `stream_object_signature()`, which eventually ends up\n> calling `odb_read_stream_open()`. And that of course _will_ call\n> `find_pack_entry()`, as we're now in the mode where we search by object\n> ID, not by offset. And consequently, we'll update the MRU in this call\n> path.\n\nGood find.\n\n> With that knowledge it's kind of easy to reproduce the issue: we simply\n> need two packfiles, and each of them must contain at least one blob that\n> is larger than \"core.bigFileThreshold\".\n> \n> Now I agree that the below proposed fix would be a good change to make\n> the code more solid while we still have `repo_for_each_pack()` (I plan\n> to eventually get rid of it). But arguably, the above logic is kind of\n> broken regardless of this: we are asked to verify objects in the current\n> pack, but we may end up verifying the object via a different pack. So if\n> the same object were to exist in multiple packs, we might end up only\n> verifying one of its instances.\n\nYeah, that was my immediate response after reading your analysis above\n(that fsck should not be doing find_pack_entry() in the first place\nhere).\n\n-Peff\n"},{"id":"536725","messageId":"aZwfmXG113t6OsUH@pks.im","threadId":"65044","inReplyTo":"20260223092523.GA209277@coredump.intra.peff.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:36:25Z","receivedAt":"2026-02-23T09:36:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 04:25:23AM -0500, Jeff King wrote:\n> On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:\n> \n> > As pointed out in [1] the root cause is actually something different,\n> > and we merely expose this now with the MRU-based iteration. But I\n> > wouldn't mind if we eventually switched back to maintaining two lists,\n> > or finding a different way for how to maintain the iteration order.\n> \n> Maybe I don't understand what you're saying, but isn't the root cause\n> the same?\n> \n> Code is iterating the list, and then during that iteration calls\n> find_pack_entry(). The fact that fsck only calls find_pack_entry() in\n> some subset of cases is immaterial, I'd think. The risk is always there\n> when iterating now.\n\nIt is, true. All I'm saying is that the problem runs a bit deeper, and\nthat fixing the actual root cause would also fix the issue reported by\nbrian.\n\nSo we might want to have another look at hardening packfile iteration\neither by reinstating the second list for iteration or by extending\n`repo_for_each_pack()` to also set the `skip_updating_mru` bit. Over\ntime though I'd rather get rid of `repo_for_each_pack()`, and once that\nis the case and packed object iteration is neatly encapsulated in the\nbackend the risk of only having the MRU will be significantly reduced.\n\nThanks!\n\nPatrick\n"},{"id":"536726","messageId":"20260223094645.GA210808@coredump.intra.peff.net","threadId":"65044","inReplyTo":"aZwfmXG113t6OsUH@pks.im","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-23T09:46:45Z","receivedAt":"2026-02-23T09:46:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 23, 2026 at 10:36:25AM +0100, Patrick Steinhardt wrote:\n\n> On Mon, Feb 23, 2026 at 04:25:23AM -0500, Jeff King wrote:\n> > On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:\n> > \n> > > As pointed out in [1] the root cause is actually something different,\n> > > and we merely expose this now with the MRU-based iteration. But I\n> > > wouldn't mind if we eventually switched back to maintaining two lists,\n> > > or finding a different way for how to maintain the iteration order.\n> > \n> > Maybe I don't understand what you're saying, but isn't the root cause\n> > the same?\n> > \n> > Code is iterating the list, and then during that iteration calls\n> > find_pack_entry(). The fact that fsck only calls find_pack_entry() in\n> > some subset of cases is immaterial, I'd think. The risk is always there\n> > when iterating now.\n> \n> It is, true. All I'm saying is that the problem runs a bit deeper, and\n> that fixing the actual root cause would also fix the issue reported by\n> brian.\n\nAh, OK, after reading your other email again, I see what you're saying.\nThe root cause (for you) is that it is unexpected for fsck to call\nfind_pack_entry() at all in this case. Which I agree is wrong, but I\njust wouldn't haven't called it the \"root\". ;)\n\nBut I think we are both on the same page that there are two problems\nworth looking at (fsck should not be looking up the object again, and we\nshould make iteration less susceptible to re-ordering bugs).\n\n> So we might want to have another look at hardening packfile iteration\n> either by reinstating the second list for iteration or by extending\n> `repo_for_each_pack()` to also set the `skip_updating_mru` bit. Over\n> time though I'd rather get rid of `repo_for_each_pack()`, and once that\n> is the case and packed object iteration is neatly encapsulated in the\n> backend the risk of only having the MRU will be significantly reduced.\n\nOK. Of the two short term solutions, I prefer the double-list. IMHO\nskip_updating_mru is a bit of a hack in the first place, because it\nmisses opportunities to update the MRU.\n\n-Peff\n"},{"id":"536733","messageId":"aZwjiBfHg4tdauJu@pks.im","threadId":"65044","inReplyTo":"aZwTPfmyrFp-QAPq@pks.im","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-23T09:53:12Z","receivedAt":"2026-02-23T09:53:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 23, 2026 at 09:43:41AM +0100, Patrick Steinhardt wrote:\n> On Sun, Feb 22, 2026 at 06:37:10PM +0000, brian m. carlson wrote:\n> > When we iterate over our packfiles in the fsck code, we do so twice.\n> > The first time, we count the number of objects in all of the packs\n> > together and later on, we iterate a second time, processing each pack\n> > and verifying its integrity.\n> > \n> > This would normally work fine, but if we have two packs and we're\n> > processing the second, the verification process will open the pack to\n> > read from it, which will place it at the beginning of the most recently\n> > used list.  Since this same list is used for iteration, the pack we most\n> > recently processed before this will then be behind the current pack in\n> > the linked list, so when we next process the list, we will go back to\n> > the first pack again and then loop forever.  This also makes our\n> > progress indicator loop up to many thousands of percent, which is not\n> > only nonsensical, but a clear indication that something has gone wrong.\n> > \n> > Solve this by skipping our MRU updates when we're iterating over\n> > packfiles, which avoids the reordering that causes problems.\n> \n> Right, this makes sense. We know that we cannot modify the list of packs\n> in case we're iterating through them, so `repo_for_each_pack()` should\n> indeed skip the MRU updates.\n> \n> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> > ---\n> > I realize that t1050 may seem like a bizarre place to put this test.\n> > However, I was debugging my sha256-interop branch and why the final test\n> > calling `git fsck` was failing, so I placed a `git fsck` earlier in the\n> > test to double-check and discovered the problem.  Since we already have\n> > a natural testcase here, I thought I'd just place the test where we\n> > already know it will trigger the problem.\n> \n> Makes me wonder though why none of the tests t1450-fsck exhibit this\n> pattern. I cannot imagine that there is no test there that doesn't have\n> multiple packs. *goes checking* We actually might not, but when trying\n> to come up with a minimum reproducer I failed at first.\n> \n> This is because ultimately the root cause seems to be a bit more\n> complex: we don't only care about there being multiple packfiles. We\n> also care about \"core.bigFileThreshold\".\n> \n> Typically, we don't execute `find_pack_entry()` at all when verifying\n> packfiles as we iterate through objects in packfile order. We thus don't\n> have to look up objects via their object ID, but instead we do so by\n> using their packfile offset. And this mechanism will not end up in\n> `find_pack_entry()`, and thus we wouldn't update the MRU.\n> \n> But there's an exception: when the size of the object that is to be\n> checked exceeds \"core.bigFileThreshold\" we won't read it directly, but\n> we'll instead use `stream_object_signature()`, which eventually ends up\n> calling `odb_read_stream_open()`. And that of course _will_ call\n> `find_pack_entry()`, as we're now in the mode where we search by object\n> ID, not by offset. And consequently, we'll update the MRU in this call\n> path.\n> \n> With that knowledge it's kind of easy to reproduce the issue: we simply\n> need two packfiles, and each of them must contain at least one blob that\n> is larger than \"core.bigFileThreshold\".\n> \n> Now I agree that the below proposed fix would be a good change to make\n> the code more solid while we still have `repo_for_each_pack()` (I plan\n> to eventually get rid of it). But arguably, the above logic is kind of\n> broken regardless of this: we are asked to verify objects in the current\n> pack, but we may end up verifying the object via a different pack. So if\n> the same object were to exist in multiple packs, we might end up only\n> verifying one of its instances.\n> \n> I've got a couple patches in the making that'll fix this.\n\nI've sent out the patches via [1]. Thanks!\n\nPatrick\n\n[1]: <20260223-pks-fsck-fix-v1-0-c29036832b6e@pks.im>\n"},{"id":"536817","messageId":"xmqqy0kjo3yr.fsf@gitster.g","threadId":"65044","inReplyTo":"20260223071215.GA136463@coredump.intra.peff.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-23T15:49:00Z","receivedAt":"2026-02-23T15:49:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Having to identify each potential spot and set a \"btw, don't switch the\n> pack list order!\" flag seems error-prone. And also loses efficiency when\n> you are iterating a pack and accessing objects in it (since we can't\n> push that pack to the front of the MRU then, even though we'd expect\n> there to be high locality with our iteration).\n\nAh, you said so clearly what I was feeling but I couldn't form into\nwords.  Using a stable second list to stably iterate over it for a\ncodepath like the fsck does sound a lot less error prone.\n"},{"id":"537032","messageId":"aZ4k5C_i_rK_yq68@fruit.crustytoothpaste.net","threadId":"65044","inReplyTo":"aZwTPfmyrFp-QAPq@pks.im","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-24T22:23:32Z","receivedAt":"2026-02-24T22:23:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-02-23 at 08:43:41, Patrick Steinhardt wrote:\n> Typically, we don't execute `find_pack_entry()` at all when verifying\n> packfiles as we iterate through objects in packfile order. We thus don't\n> have to look up objects via their object ID, but instead we do so by\n> using their packfile offset. And this mechanism will not end up in\n> `find_pack_entry()`, and thus we wouldn't update the MRU.\n\nIf you're thinking about `nth_packed_object_id`, that is index (object\nID) order, not packfile order.  I actually made this mistake when\nwriting the interop code and having that function operate in pack order\nbreaks a surprising number of things in very subtle ways, notably\ngenerating multi-pack indexes.\n\nI will be sending a patch in the future documenting that requirement\nclearly.\n\n> I've got a couple patches in the making that'll fix this.\n\nI'm happy to drop this patch in favour of yours.  Thanks for a quick\nresponse.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"537034","messageId":"xmqq5x7lepsq.fsf@gitster.g","threadId":"65044","inReplyTo":"aZ4k5C_i_rK_yq68@fruit.crustytoothpaste.net","subject":"Re: [PATCH] fsck: do not loop infinitely when processing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-24T22:32:05Z","receivedAt":"2026-02-24T22:32:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2026-02-23 at 08:43:41, Patrick Steinhardt wrote:\n>> Typically, we don't execute `find_pack_entry()` at all when verifying\n>> packfiles as we iterate through objects in packfile order. We thus don't\n>> have to look up objects via their object ID, but instead we do so by\n>> using their packfile offset. And this mechanism will not end up in\n>> `find_pack_entry()`, and thus we wouldn't update the MRU.\n>\n> If you're thinking about `nth_packed_object_id`, that is index (object\n> ID) order, not packfile order.  I actually made this mistake when\n> writing the interop code and having that function operate in pack order\n> breaks a surprising number of things in very subtle ways, notably\n> generating multi-pack indexes.\n>\n> I will be sending a patch in the future documenting that requirement\n> clearly.\n>\n>> I've got a couple patches in the making that'll fix this.\n>\n> I'm happy to drop this patch in favour of yours.  Thanks for a quick\n> response.\n\nOK, so I'll retire your fef2a726 (fsck: do not loop infinitely when\nprocessing packs, 2026-02-22) and replace it with the four-patch\nseries:\n\n26fc7b59cd t/helper: improve \"genrandom\" test helper\n10a6762719 object-file: adapt `stream_object_signature()` to take a stream\n41b42e3527 packfile: expose function to read object stream for an offset\n13eb65d366 pack-check: fix verification of large objects\n\nThanks.\n"}]}