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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2014, 18:50 UTC
Message-ID: <xmqqppdayal7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141029171158.GA32188@peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> 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).

Show 7 quoted lines

> 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.

Show 16 quoted lines

>  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;