Re: [RFC/PATCH] cache-tree: avoid infinite loop on zero-entry tree

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> I'm a little iffy on this just because it is fixing one particular bug,
> and I am sure there are probably a bunch of other ways to have a bogus
> index. Fundamentally, I think we pretty much trust that the index was
> not maliciously generated (unlike packfiles, for instance, which can
> come from elsewhere).  Still, this is one step closer to safe, and the
> bug was seen in the wild, so maybe it is worth doing.

Is it cheap to sanity-check the input when we map in the cache-tree
upon read_cache()?  Then we can just invalidate the cache-tree,
either in its entirety (easy) or just the bogus subpart (maybe not
worth doing).

> We could alternatively (or in addition) reject 0-entry cache trees when
> reading them from disk. The trick, though, is that it is not just
> records with 0 entries, but ones where the sum of the entries and
> subtree entries is 0. Given that it is not something we expect to
> happen, it is easier to catch it here. And we know there can be no
> regressions for missed corner cases, because the case we are catching
> here would _always_ have gone into an infinite loop before this patch.

OK.  I wonder if we can instead die here but propagate the error
back up the callchain and have the ultimate caller rebuild the cache
tree without paying attention to the existing data that we now know
is bogus.

>  cache-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 215202c..32772b9 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -303,6 +303,8 @@ static int update_one(struct cache_tree *it,
>  				    flags);
>  		if (subcnt < 0)
>  			return subcnt;
> +		if (!subcnt)
> +			die("index cache-tree records empty sub-tree");
>  		i += subcnt;
>  		sub->count = subcnt; /* to be used in the next loop */
>  		*skip_count += subskip;