Re: [PATCH 3/5] pack-objects: clamp negative window size to 0

From: Jeff King <peff@peff.net>
Date: May 3, 2021, 14:55 UTC
Message-ID: <YJAOzTIXkO2lhxSs@coredump.intra.peff.net>
In-Reply-To: <3d77d70b-2cc5-4ca9-8753-fa9af5111842@gmail.com>

On Mon, May 03, 2021 at 08:10:24AM -0400, Derrick Stolee wrote:

Show 13 quoted lines

> On 5/1/2021 10:03 AM, Jeff King wrote:
> > A negative window size makes no sense, and the code in find_deltas() is
> > not prepared to handle it. If you pass "-1", for example, we end up
> > generate a 0-length array of "struct unpacked", but our loop assumes it
> > has at least one entry in it (and we end up reading garbage memory).
> > 
> > We could complain to the user about this, but it's more forgiving to
> > just clamp it to 0, which means "do not find any deltas at all". The
> > 0-case is already tested earlier in the script, so we'll make sure this
> > does the same thing.
> 
> This seems like a reasonable approach. It takes existing "undefined"
> behavior and turns it into well-understood, "defined" behavior.

I was on the fence on doing that, or just:

  if (window < 0)
	die("sorry dude, negative windows are nonsense");

So if anybody has a strong preference, I could be easily persuaded. Part of what led me to being forgiving was that we similarly clamp too-large depth values (with a warning; I didn't think it was really necessary here, though).

-Peff