Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Max Kirillov <max@max630.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Jelmer Vernooij" <jelmer@jelmer.uk>,
	git@vger.kernel.org
Subject: Re: [PATCH] http-backend: allow empty CONTENT_LENGTH
Date: Thu, 6 Sep 2018 23:38:31 -0400
Message-ID: <20180907033831.GB1383@sigill.intra.peff.net> (raw)
In-Reply-To: <20180907032740.GA20545@jessie.local>

On Fri, Sep 07, 2018 at 06:27:40AM +0300, Max Kirillov wrote:

> On Thu, Sep 06, 2018 at 02:54:18PM -0700, Junio C Hamano wrote:
> > Max Kirillov <max@max630.net> writes:
> >> This should fix it. I'm not sure should it treat it as 0 or "-1"
> >> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as "-1"
> >> So keep the existing behavior as much as possible
> > 
> > I am not sure what you mean by the above, between 0 and -1.  The
> > code signals the caller of get_content_length() that req_len is -1
> > which is used as a sign to read through to the EOF, so it appears to
> > me that the code treats missing content-length (i.e. str == NULL
> > case) as "-1".
> 
> I made a mistake in this, it should be "if I try to treat missing
> CONTENT_LENGTH as 0". This, as far as I understand, what the
> RFC specifies.
> 
> That is, after the following change, the test "large fetch-pack
> requests can be split across POSTs" from t5551 starts faliing:
> 
> -- >8 --
> @@ -353,8 +353,12 @@ static ssize_t get_content_length(void)
>         ssize_t val = -1;
>         const char *str = getenv("CONTENT_LENGTH");
>  
> -       if (str && *str && !git_parse_ssize_t(str, &val))
> -               die("failed to parse CONTENT_LENGTH: %s", str);
> +       if (str && *str) {
> +               if (!git_parse_ssize_t(str, &val))
> +                       die("failed to parse CONTENT_LENGTH: %s", str);
> +       } else
> +               val = 0;
> +

Right, I'm pretty sure it is a problem if you treat a missing
CONTENT_LENGTH as "present, but zero". Because chunked encodings from
apache really do want us to read until EOF.

My understanding from Jelmer's report is that a present-but-empty
variable should be counted as "0" to mean "do not read any body bytes".
That matches my reading of RFC 3875, which says:

  If no data is attached, then NULL (or unset).

(and earlier they explicitly define NULL as the empty string). That
said, we do not do what they say for the "unset" case. And cannot
without breaking chunked encoding from apache. So I don't know how much
we want to follow that rfc to the letter, but at least it makes sense to
me to revert this case back to what Git used to do, and what the rfc
says.

In other words, I think the logic we want is:

  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;
  } else {
	/*
	 * We have a CONTENT_LENGTH; trust what's in it as long as it
	 * can be parsed.
	 */
	if (!git_parse_ssize_t(str, &val))
	        die(...);
  }

-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 [this message]
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
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=20180907033831.GB1383@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jelmer@jelmer.uk \
    --cc=jrnieder@gmail.com \
    --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