patchfsck: do not loop infinitely when processing packs

> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index 5be273611a..75e75e627c 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -160,6 +160,10 @@ test_expect_success 'hash-object' '
>  	git hash-object large1
>  '
>  
> +test_expect_success 'fsck does not loop forever' '
> +	git fsck
> +'
> +
>  test_expect_success 'cat-file a large file' '
>  	git cat-file blob :large1 >/dev/null
>  '

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> > index 5be273611a..75e75e627c 100755
> > --- a/t/t1050-large.sh
> > +++ b/t/t1050-large.sh
> > @@ -160,6 +160,10 @@ test_expect_success 'hash-object' '
> >  	git hash-object large1
> >  '
> >  
> > +test_expect_success 'fsck does not loop forever' '
> > +	git fsck
> > +'
> > +
> >  test_expect_success 'cat-file a large file' '
> >  	git cat-file blob :large1 >/dev/null
> >  '
> 
> Wow, this is a fun test ;-).
> 
> Thanks.  Will queue.

> I noticed that the code here seems to have come in with the 2.53 cycle,
> so we may want to cherry-pick it to `maint` at some point if it seems
> like the problem occurs often.  From what I can tell, it only occurs
> when one explicitly invokes `git fsck`[0] and not on transfer, so it
> shouldn't cause a DoS against server implementations.
> 
> Of course, we should wait for Patrick, who authored this code, to chime
> in and lend his expertise here.  I must admit I'm not very familiar with
> this area, although I had recently seen the MRU code when working on
> pack index v3 (and then I thought, "is this actually the problem?").

> On Sun, Feb 22, 2026 at 11:07:41PM +0000, brian m. carlson wrote:
> 
> > I noticed that the code here seems to have come in with the 2.53 cycle,
> > so we may want to cherry-pick it to `maint` at some point if it seems
> > like the problem occurs often.  From what I can tell, it only occurs
> > when one explicitly invokes `git fsck`[0] and not on transfer, so it
> > shouldn't cause a DoS against server implementations.
> > 
> > Of course, we should wait for Patrick, who authored this code, to chime
> > in and lend his expertise here.  I must admit I'm not very familiar with
> > this area, although I had recently seen the MRU code when working on
> > pack index v3 (and then I thought, "is this actually the problem?").
> 
> The problem seems to bisect to c31bad4f7d (packfile: track packs via the
> MRU list exclusively, 2025-10-30), which is not terribly surprising, as
> it was one of the known risks of collapsing the two lists into one.
> 
> Your solution is using the tool provided by that commit for its edge
> case:
> 
>     Note that there is one important edge case: `for_each_packed_object()`
>     uses the MRU list to iterate through packs, and then it lists each
>     object in those packs. This would have the effect that we now sort the
>     current pack towards the front, thus modifying the list of packfiles we
>     are iterating over, with the consequence that we'll see an infinite
>     loop. This edge case is worked around by introducing a new field that
>     allows us to skip updating the MRU.
> 
> So in that sense it is the right thing. But it really makes me wonder if
> we are going back to keeping two lists (one MRU and one in some stable
> order). Or at the very least providing _some_ iteration method that is
> guaranteed to be stable (whether a linked list or a function), so that
> iterating code is not subject to this subtle dependency by default.
> 
> Having to identify each potential spot and set a "btw, don't switch the
> pack list order!" flag seems error-prone. And also loses efficiency when
> you are iterating a pack and accessing objects in it (since we can't
> push that pack to the front of the MRU then, even though we'd expect
> there to be high locality with our iteration).

> On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:
> 
> > As pointed out in [1] the root cause is actually something different,
> > and we merely expose this now with the MRU-based iteration. But I
> > wouldn't mind if we eventually switched back to maintaining two lists,
> > or finding a different way for how to maintain the iteration order.
> 
> Maybe I don't understand what you're saying, but isn't the root cause
> the same?
> 
> Code is iterating the list, and then during that iteration calls
> find_pack_entry(). The fact that fsck only calls find_pack_entry() in
> some subset of cases is immaterial, I'd think. The risk is always there
> when iterating now.

> On Mon, Feb 23, 2026 at 04:25:23AM -0500, Jeff King wrote:
> > On Mon, Feb 23, 2026 at 09:46:00AM +0100, Patrick Steinhardt wrote:
> > 
> > > As pointed out in [1] the root cause is actually something different,
> > > and we merely expose this now with the MRU-based iteration. But I
> > > wouldn't mind if we eventually switched back to maintaining two lists,
> > > or finding a different way for how to maintain the iteration order.
> > 
> > Maybe I don't understand what you're saying, but isn't the root cause
> > the same?
> > 
> > Code is iterating the list, and then during that iteration calls
> > find_pack_entry(). The fact that fsck only calls find_pack_entry() in
> > some subset of cases is immaterial, I'd think. The risk is always there
> > when iterating now.
> 
> It is, true. All I'm saying is that the problem runs a bit deeper, and
> that fixing the actual root cause would also fix the issue reported by
> brian.

> So we might want to have another look at hardening packfile iteration
> either by reinstating the second list for iteration or by extending
> `repo_for_each_pack()` to also set the `skip_updating_mru` bit. Over
> time though I'd rather get rid of `repo_for_each_pack()`, and once that
> is the case and packed object iteration is neatly encapsulated in the
> backend the risk of only having the MRU will be significantly reduced.

> Having to identify each potential spot and set a "btw, don't switch the
> pack list order!" flag seems error-prone. And also loses efficiency when
> you are iterating a pack and accessing objects in it (since we can't
> push that pack to the front of the MRU then, even though we'd expect
> there to be high locality with our iteration).

> When we iterate over our packfiles in the fsck code, we do so twice.
> The first time, we count the number of objects in all of the packs
> together and later on, we iterate a second time, processing each pack
> and verifying its integrity.
> 
> This would normally work fine, but if we have two packs and we're
> processing the second, the verification process will open the pack to
> read from it, which will place it at the beginning of the most recently
> used list.  Since this same list is used for iteration, the pack we most
> recently processed before this will then be behind the current pack in
> the linked list, so when we next process the list, we will go back to
> the first pack again and then loop forever.  This also makes our
> progress indicator loop up to many thousands of percent, which is not
> only nonsensical, but a clear indication that something has gone wrong.
> 
> Solve this by skipping our MRU updates when we're iterating over
> packfiles, which avoids the reordering that causes problems.

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> I realize that t1050 may seem like a bizarre place to put this test.
> However, I was debugging my sha256-interop branch and why the final test
> calling `git fsck` was failing, so I placed a `git fsck` earlier in the
> test to double-check and discovered the problem.  Since we already have
> a natural testcase here, I thought I'd just place the test where we
> already know it will trigger the problem.

> diff --git a/packfile.h b/packfile.h
> index acc5c55ad5..086d98c1a0 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -191,8 +192,13 @@ static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct
>  
>  	odb_prepare_alternates(repo->objects);
>  
> +	data.repo = repo;
> +
>  	for (struct odb_source *source = repo->objects->sources; source; source = source->next) {
> -		struct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);
> +		struct packfile_list_entry *entry;
> +
> +		source->packfiles->skip_mru_updates = true;
> +		entry = packfile_store_get_packs(source->packfiles);
>  		if (!entry)
>  			continue;
>  		data.source = source;
> @@ -212,7 +218,10 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *
>  		return;
>  
>  	for (source = data->source->next; source; source = source->next) {
> -		struct packfile_list_entry *entry = packfile_store_get_packs(source->packfiles);
> +		struct packfile_list_entry *entry;
> +
> +		source->packfiles->skip_mru_updates = true;
> +		entry = packfile_store_get_packs(source->packfiles);
>  		if (!entry)
>  			continue;
>  		data->source = source;
> @@ -220,6 +229,9 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *
>  		return;
>  	}
>  
> +	for (struct odb_source *source = data->repo->objects->sources; source; source = source->next)
> +		source->packfiles->skip_mru_updates = false;
> +
>  	data->source = NULL;
>  	data->entry = NULL;
>  }

> This is because ultimately the root cause seems to be a bit more
> complex: we don't only care about there being multiple packfiles. We
> also care about "core.bigFileThreshold".
> 
> Typically, we don't execute `find_pack_entry()` at all when verifying
> packfiles as we iterate through objects in packfile order. We thus don't
> have to look up objects via their object ID, but instead we do so by
> using their packfile offset. And this mechanism will not end up in
> `find_pack_entry()`, and thus we wouldn't update the MRU.
> 
> But there's an exception: when the size of the object that is to be
> checked exceeds "core.bigFileThreshold" we won't read it directly, but
> we'll instead use `stream_object_signature()`, which eventually ends up
> calling `odb_read_stream_open()`. And that of course _will_ call
> `find_pack_entry()`, as we're now in the mode where we search by object
> ID, not by offset. And consequently, we'll update the MRU in this call
> path.

> With that knowledge it's kind of easy to reproduce the issue: we simply
> need two packfiles, and each of them must contain at least one blob that
> is larger than "core.bigFileThreshold".
> 
> Now I agree that the below proposed fix would be a good change to make
> the code more solid while we still have `repo_for_each_pack()` (I plan
> to eventually get rid of it). But arguably, the above logic is kind of
> broken regardless of this: we are asked to verify objects in the current
> pack, but we may end up verifying the object via a different pack. So if
> the same object were to exist in multiple packs, we might end up only
> verifying one of its instances.

> On Sun, Feb 22, 2026 at 06:37:10PM +0000, brian m. carlson wrote:
> > When we iterate over our packfiles in the fsck code, we do so twice.
> > The first time, we count the number of objects in all of the packs
> > together and later on, we iterate a second time, processing each pack
> > and verifying its integrity.
> > 
> > This would normally work fine, but if we have two packs and we're
> > processing the second, the verification process will open the pack to
> > read from it, which will place it at the beginning of the most recently
> > used list.  Since this same list is used for iteration, the pack we most
> > recently processed before this will then be behind the current pack in
> > the linked list, so when we next process the list, we will go back to
> > the first pack again and then loop forever.  This also makes our
> > progress indicator loop up to many thousands of percent, which is not
> > only nonsensical, but a clear indication that something has gone wrong.
> > 
> > Solve this by skipping our MRU updates when we're iterating over
> > packfiles, which avoids the reordering that causes problems.
> 
> Right, this makes sense. We know that we cannot modify the list of packs
> in case we're iterating through them, so `repo_for_each_pack()` should
> indeed skip the MRU updates.
> 
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > I realize that t1050 may seem like a bizarre place to put this test.
> > However, I was debugging my sha256-interop branch and why the final test
> > calling `git fsck` was failing, so I placed a `git fsck` earlier in the
> > test to double-check and discovered the problem.  Since we already have
> > a natural testcase here, I thought I'd just place the test where we
> > already know it will trigger the problem.
> 
> Makes me wonder though why none of the tests t1450-fsck exhibit this
> pattern. I cannot imagine that there is no test there that doesn't have
> multiple packs. *goes checking* We actually might not, but when trying
> to come up with a minimum reproducer I failed at first.
> 
> This is because ultimately the root cause seems to be a bit more
> complex: we don't only care about there being multiple packfiles. We
> also care about "core.bigFileThreshold".
> 
> Typically, we don't execute `find_pack_entry()` at all when verifying
> packfiles as we iterate through objects in packfile order. We thus don't
> have to look up objects via their object ID, but instead we do so by
> using their packfile offset. And this mechanism will not end up in
> `find_pack_entry()`, and thus we wouldn't update the MRU.
> 
> But there's an exception: when the size of the object that is to be
> checked exceeds "core.bigFileThreshold" we won't read it directly, but
> we'll instead use `stream_object_signature()`, which eventually ends up
> calling `odb_read_stream_open()`. And that of course _will_ call
> `find_pack_entry()`, as we're now in the mode where we search by object
> ID, not by offset. And consequently, we'll update the MRU in this call
> path.
> 
> With that knowledge it's kind of easy to reproduce the issue: we simply
> need two packfiles, and each of them must contain at least one blob that
> is larger than "core.bigFileThreshold".
> 
> Now I agree that the below proposed fix would be a good change to make
> the code more solid while we still have `repo_for_each_pack()` (I plan
> to eventually get rid of it). But arguably, the above logic is kind of
> broken regardless of this: we are asked to verify objects in the current
> pack, but we may end up verifying the object via a different pack. So if
> the same object were to exist in multiple packs, we might end up only
> verifying one of its instances.
> 
> I've got a couple patches in the making that'll fix this.

> Typically, we don't execute `find_pack_entry()` at all when verifying
> packfiles as we iterate through objects in packfile order. We thus don't
> have to look up objects via their object ID, but instead we do so by
> using their packfile offset. And this mechanism will not end up in
> `find_pack_entry()`, and thus we wouldn't update the MRU.

> On 2026-02-23 at 08:43:41, Patrick Steinhardt wrote:
>> Typically, we don't execute `find_pack_entry()` at all when verifying
>> packfiles as we iterate through objects in packfile order. We thus don't
>> have to look up objects via their object ID, but instead we do so by
>> using their packfile offset. And this mechanism will not end up in
>> `find_pack_entry()`, and thus we wouldn't update the MRU.
>
> If you're thinking about `nth_packed_object_id`, that is index (object
> ID) order, not packfile order.  I actually made this mistake when
> writing the interop code and having that function operate in pack order
> breaks a surprising number of things in very subtle ways, notably
> generating multi-pack indexes.
>
> I will be sending a patch in the future documenting that requirement
> clearly.
>
>> I've got a couple patches in the making that'll fix this.
>
> I'm happy to drop this patch in favour of yours.  Thanks for a quick
> response.