Re: [PATCH] Avoid infinite loop in malformed packfiles

From: Jeff King <peff@peff.net>
Date: Aug 31, 2020, 09:29 UTC
Message-ID: <20200831092946.GA2812764@coredump.intra.peff.net>
In-Reply-To: <xmqqwo1gglf5.fsf@gitster.c.googlers.com>

On Sun, Aug 30, 2020 at 09:15:10AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
> >> Will that work? I'd expect that modern pack files end up being
> >> offset deltas, rather than reference deltas.
> >
> > True, but going down all the way would work:
> 
> Perhaps, but I'd rather use pack-objects to prepare the repository
> with no-delta-base-offset to force ref deltas.

Yeah, that seems like a much better test setup.

It does raise an interesting question, though. I had imagined we would limit the depth of all delta chains here, not just ref-deltas. But it is true that ofs deltas can't cycle. Without cycles, neither type can go on indefinitely (they are limited by the number of entries in the packfile). I could see arguments going either way:

  - ofs deltas cannot cycle, so we do not need a counter that limits
    them (and which _could_ find a false positive). So we should not
    limit them.

  - a counter is preventing us from following cycles indefinitely, but
    also hardening us against misbehavior due to bugs or insanely large
    delta chains (intentional or not). So we should include ofs deltas
    in our limit.

A related point is that delta chains might be composed of both types. If we don't differentiate between the two types, then the limit is clearly total chain length. If we do, then is the limit the total number of ref-deltas found in the current lookup, or is it the number of consecutive ref-deltas? I guess it would have to be the former if our goal is to catch cycles (since a cycle could include an ofs-delta, as long as a ref-delta is the part that forms the loop).

-Peff