Re: [PATCH] http-walker: fix buffer underflow processing remote alternates

From: Jeff King <peff@peff.net>
Date: Mar 13, 2017, 13:50 UTC
Message-ID: <20170313135053.3pi56dfdlcva66ml@sigill.intra.peff.net>
In-Reply-To: <xmqqfuihu382.fsf@gitster.mtv.corp.google.com>

On Sun, Mar 12, 2017 at 10:59:09PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> > Let's fix this by using strbuf_strip_suffix() to drop the
> > characters we want. As a bonus this lets us handle names
> > that do not end in "objects" (all git repos do, but there is
> > nothing to say that an alternate object store needs to be a
> > git repo).
> 
> Hmph.
> 
> Isn't the reason why the function wants to strip "objects" at the
> end because it then expects to be able to tack strings like
> "objects/info/packs", etc. after the result of stripping, i.e.
> $URL/../foo.git/, and get usable URLs to download other things?

Good point. I think that behavior is a misfeature in the first place. It should be leaving the path as-is and tacking "info/packs", etc.

But without fixing that, yeah, there is not much value in the "maybe strip" behavior (unless you happen to provide the incorrect path that does not include "objects" in the first place, but then it would not work as a _local_ alternate).

Show 5 quoted lines

> I think it is very sensible to use strip_suffix() to notice that the
> alternate does not end with "/objects", but I am not sure if it is a
> good idea to proceed without stripping when it does not end with
> "/objects".  Shouldn't we be ignoring (with warning, possibly) such
> a remote alternate?

Yeah, probably warn and ignore is the best bet. I'll re-work the patch.

-Peff