Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Max Kirillov" <max@max630.net>,
	git@vger.kernel.org, "Jelmer Vernooij" <jelmer@jelmer.uk>,
	"Florian Manschwetus" <manschwetus@cs-software-gmbh.de>
Subject: Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH
Date: Mon, 10 Sep 2018 09:17:25 -0400
Message-ID: <20180910131724.GA5233@sigill.intra.peff.net> (raw)
In-Reply-To: <20180910052558.GB55941@aiede.svl.corp.google.com>

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

> > --- 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

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-06  6:10 CONTENT_LENGTH can no longer be empty Jonathan Nieder
2018-09-06 19:35 ` [PATCH] http-backend: allow empty CONTENT_LENGTH Max Kirillov
2018-09-06 21:54   ` Junio C Hamano
2018-09-07  3:27     ` Max Kirillov
2018-09-07  3:38       ` Jeff King
2018-09-07  4:20         ` Max Kirillov
2018-09-07  4:59         ` Max Kirillov
2018-09-07  9:49           ` Junio C Hamano
2018-09-08  5:41             ` Max Kirillov
2018-09-09  4:40             ` Max Kirillov
2018-09-06 22:45   ` Jonathan Nieder
2018-09-07  3:36   ` [PATCH v2] " Max Kirillov
2018-09-09  4:10     ` [PATCH v4] " Max Kirillov
2018-09-10  5:25       ` Jonathan Nieder
2018-09-10 13:17         ` Jeff King [this message]
2018-09-10 16:37           ` Junio C Hamano
2018-09-10 18:46             ` Jeff King
2018-09-10 20:53         ` [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero Max Kirillov
2018-09-10 21:22           ` Jonathan Nieder
2018-09-11  1:55             ` Jeff King
2018-09-11  2:20               ` Jonathan Nieder
2018-09-11  2:30                 ` Jeff King
2018-09-11  1:58           ` Jeff King
2018-09-11  3:42           ` [PATCH] http-backend: treat " Jonathan Nieder
2018-09-11  4:03             ` Jonathan Nieder
2018-09-11 18:15               ` Junio C Hamano
2018-09-11 18:27                 ` Junio C Hamano
2018-09-12  5:56                 ` Jeff King
2018-09-12  6:26                   ` Jonathan Nieder
2018-09-12 16:10                   ` Junio C Hamano
2018-09-11  4:18             ` Junio C Hamano
2018-09-11  4:29               ` Jonathan Nieder
2018-09-08  0:19 [PATCH v2] http-backend: allow empty CONTENT_LENGTH Jonathan Nieder
2018-09-08  5:35 ` Max Kirillov
2018-09-08  5:42 ` [PATCH v3] " Max Kirillov
2018-09-10  5:17   ` Jonathan Nieder
2018-09-10 20:36     ` Max Kirillov
2018-09-11  4:06 Jonathan Nieder
2018-09-11 20:33 ` [PATCH v2] http-backend test: make empty CONTENT_LENGTH test more realistic Max Kirillov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180910131724.GA5233@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jelmer@jelmer.uk \
    --cc=jrnieder@gmail.com \
    --cc=manschwetus@cs-software-gmbh.de \
    --cc=max@max630.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git