Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH

From: Jeff King <peff@peff.net>
Date: Sep 10, 2018, 13:17 UTC
Message-ID: <20180910131724.GA5233@sigill.intra.peff.net>
In-Reply-To: <20180910052558.GB55941@aiede.svl.corp.google.com>

On Sun, Sep 09, 2018 at 10:25:58PM -0700, Jonathan Nieder wrote:

Show 28 quoted lines

> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -353,8 +353,28 @@ static ssize_t get_content_length(void)
> >  	ssize_t val = -1;
> >  	const char *str = getenv("CONTENT_LENGTH");
> >  
> > -	if (str && !git_parse_ssize_t(str, &val))
> > -		die("failed to parse CONTENT_LENGTH: %s", str);
> > +	if (!str) {
> > +		/*
> > +		 * RFC3875 says this must mean "no body", but in practice we
> > +		 * receive chunked encodings with no CONTENT_LENGTH. Tell the
> > +		 * caller to read until EOF.
> > +		 */
> > +		val = -1;
> > +	} else if (!*str) {
> > +		/*
> > +		 * An empty length should be treated as "no body" according to
> > +		 * RFC3875, and this seems to hold in practice.
> > +		 */
> > +		val = 0;
> 
> Are there example callers that this version fixes?  Where can I read
> more, or what can I run to experience it?
> 
> For example, v2.19.0-rc0~45^2~2 (http-backend: respect CONTENT_LENGTH
> as specified by rfc3875, 2018-06-10) mentions IIS/Windows; does IIS
> make use of this distinction?

So this code is what I recommended based on my reading of the RFC, and based on my understanding of the Debian bug. But I admit I'm confused.

I thought the complaint was that this:

  CONTENT_LENGTH= git http-backend

was reading a body, when it shouldn't be. And so setting it to 0 here made sense.

But that couldn't have been what older versions were doing, since they never looked at CONTENT_LENGTH at all, and instead always read to EOF. So presumably the original problem wasn't that we tried to read a body, but that the empty string caused git_parse_ssize_t to report failure, and we called die(). Which probably should be explained by 574c513e8d (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too late for that.

So after that patch, we really do have the original behavior, and that's enough for v2.19.

But the remaining question then is: what should clients expect on an empty variable? We know what the RFC says, and we know what dulwich expected, but I'm not sure we have real world cases beyond that. So it might actually make sense to punt until we see one, though I don't mind doing what the rfc says in the meantime. And then the explanation in the commit message would be "do what the rfc says", and any test probably ought to be feeding a non-empty empty and confirming that we don't read it.

-Peff