On Wed, Sep 18, 2024 at 06:35:35PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
