Re: [PATCH 2/3] cache-tree: detect mismatching number of index entries

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 24, 2024, 06:48 UTC
Message-ID: <ZvJgnqSwANCXmj0G@pks.im>
In-Reply-To: <xmqqttec8ly0.fsf@gitster.g>

On Wed, Sep 18, 2024 at 06:35:35PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +	if (it->entry_count + pos > istate->cache_nr) {
> > +		ret = error(_("corrupted cache-tree has entries not present in index"));
> > +		goto out;
> > +	}
> 
> Is it a safe assumption that the if() condition always indicates an
> error?  When sparse-index is in effect, istate->cache_nr may be a
> number that is smaller than the true number of paths in the index
> (because all paths under a subdirectory we are not interested in are
> folded into a single tree-ish entry), and I am not sure how it
> should interact with it->entry_count (i.e. the number of paths under
> the current directory we are looking at, which obviously cannot be a
> sparsified entry) and pos (i.e. the index into active_cache[] that
> represend the first path under the current directory)?
> 
> I guess as long as "it" is not folded, it does not matter how other
> paths from different directories in active_cache[] are sparsified or
> expanded, as long as "pos" keeps track of the current position
> correctly.

It seems like we end up calling `ensure_full_index()` for a sparse index, which does cause us to signal to the caller that they should restart verification. So for all I understand, this function shouldn't act on a sparsely-populated index.

But I cannot see how it could lead to anything sensible when the added condition is violated because the first thing we do in the loop is this:

	struct cache_entry *ce = istate->cache[pos + i];

And before we do anything else, we dereference that pointer. So if the condition doesn't hold we _will_ get an out-of-bounds read of the cache array and act on the garbage data. And that causes the observed segfault on my machine and in the test.

So I think that ensuring this property is always the right thing to do. But I wouldn't be surprised if overall this code could require more love to make it behave sanely in all scenarios. It certainly feels somewhat fragile to me.

Patrick