Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines

From: Jeff King <peff@peff.net>
Date: Feb 17, 2013, 19:14 UTC
Message-ID: <20130217191422.GA25096@sigill.intra.peff.net>
In-Reply-To: <20130217104939.GE6759@elie.Belkin>

On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:

Show 20 quoted lines

> Jeff King wrote:
> 
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> [...]
> > @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)
> [...]
> > -		strbuf_reset(&buffer);
> > -		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
> > -			strbuf_reset(&buffer);
> > +		if (read_packets_until_flush(&last->buf, &last->len) < 0)
> 
> Style nit: this made me wonder "What would it mean if
> read_packets_until_flush() > 0?"  Since the convention for this
> function is "0 for success", I would personally find
> 
> 		if (read_packets_until_flush(...))
> 			handle error;
> 
> easier to read.

My intent was that it followed the error convention of "negative is error, 0 is success, and positive is not used, but reserved for future use". And I tend to think the "< 0" makes it obvious that we are interested in error. But I don't feel that strongly, so if people would rather see it the other way, I can live with it.

Show 7 quoted lines

> > +			die("smart-http metadata lines are invalid at %s",
> > +			    refs_url);
> 
> Especially given that other clients would be likely to run into
> trouble in the same situation, as long as this cooks in "next" for a
> suitable amount of time to catch bad servers, it looks like a good
> idea.

Yeah, I have a slight concern that this series would break something in another implementation, so I would like to see this cook in "next" for a while (and would be slated for master probably not in this release, but in the next one). But I think this change is pretty straightforward. If an implementation is producing bogus packet lines and expecting us not to complain, it really needs to be fixed.

-Peff