Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 11, 2018, 04:03 UTC
Message-ID: <20180911040343.GC20518@aiede.svl.corp.google.com>
In-Reply-To: <20180911034227.GB20518@aiede.svl.corp.google.com>

Kicking off the reviews: ;-)

Jonathan Nieder wrote:

Show 5 quoted lines

> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o
>  
>  static ssize_t get_content_length(void)

[...]

Show 5 quoted lines

> +		/*
> +		 * According to RFC 3875, an empty or missing
> +		 * CONTENT_LENGTH means "no body", but RFC 3875
> +		 * precedes HTTP/1.1 and chunked encoding. Apache and
> +		 * its imitators leave CONTENT_LENGTH unset for

Which imitators?  Maybe this should just say "Apache leaves [...]".

Show 5 quoted lines

> +		 * chunked requests, for which we should use EOF to
> +		 * detect the end of the request.
> +		 */
> +		str = getenv("HTTP_TRANSFER_ENCODING");
> +		if (str && !strcmp(str, "chunked"))

RFC 2616 says Transfer-Encoding is a list of transfer-codings applied, in the order that they were applied, and that "chunked" is always applied last. That means a transfer-encoding like

	Transfer-Encoding: identity chunked

would be permitted, or e.g.

	Transfer-Encoding: gzip chunked

Does that means we should be using a check like

	str && (!strcmp(str, "chunked") || ends_with(str, " chunked"))

That said, a quick search of codesearch.debian.net mostly finds examples using straight comparison, so maybe the patch is fine as-is.

Thanks, Jonathan