git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Max Kirillov <max@max630.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Florian Manschwetus <manschwetus@cs-software-gmbh.de>,
	Chris Packham <judge.packham@gmail.com>,
	Konstantin Khomoutov <kostix+git@007spb.ru>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875
Date: Tue, 28 Nov 2017 22:22:14 -0500	[thread overview]
Message-ID: <20171129032214.GB32345@sigill.intra.peff.net> (raw)
In-Reply-To: <20171126193813.12531-2-max@max630.net>

On Sun, Nov 26, 2017 at 09:38:12PM +0200, Max Kirillov wrote:

> From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Date: Wed, 30 Mar 2016 10:54:21 +0200
> 
> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. Web server may exercise the specification by not closing
> the script's standard input after writing content. In that case http-backend
> would hang waiting for the input. The issue is known to happen with
> IIS/Windows, for example.
> 
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.

I missed out on review of the earlier iterations from the past few days,
but I looked this over in light of the comments I made long ago in:

  https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/

The concerns I had there were:

  1. It did the wrong thing when CONTENT_LENGTH was not present in the
     environment. Your version here gets this right.

  2. I earlier was worried that this wouldn't kick in for the
     inflate_request() code path. It looks like we do use read_request()
     from inflate_request(), but only when buffer_input is true. Which
     means we wouldn't do so for receive-pack (i.e., for pushes).

     I'm not sure if the client would ever gzip a push request. Without
     double-checking, I suspect it would if the list of refs to update
     was sufficiently large (and at any rate, I think other clients
     potentially could do so).

     That _might_ be OK in practice. If the gzip stream is well-formed,
     we'd stop at its end. We'd possibly still try to read() more bytes
     than were promised, but if I understand the original problem,
     that's not that big a deal as long as we don't hang waiting for
     EOF.

     If the stream isn't well-formed, we'd hang waiting for more bytes
     (rather than seeing the EOF and complaining that the gzip stream is
     truncated). That's poor, but no worse than the current behavior.

  3. For large inputs (like incoming packfiles), we connect the
     descriptor directly to index-pack or unpack-objects, and they try
     to read to EOF.

     For a well-formed pack, I _think_ this would work OK. We'd see the
     end of the pack and quit (there's a check for garbage at the end of
     the pack, but it triggers only for the non-pipe case).

     For a truncated input, we'd hang forever rather than report an
     error.

So I suspect there are lurking problems that may trigger in corner
cases. That said, I don't think this pack makes any case _worse_, and it
may make some common ones better. So I'm not opposed, though we may be
giving people a false sense of security that it actually works robustly
on IIS.

> ---
>  config.c       |  2 +-
>  config.h       |  1 +
>  http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 51 insertions(+), 2 deletions(-)

The patch itself looks OK, modulo the comments already made by others.
Two minor observations, and a possible bug:

> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
> +{

I did wonder if this "stop at n bytes" could simply be rolled into the
existing read_request loop (by limiting the length we pass to
read_in_full there). But it may be cleaner to just have a separate
function. There's some repetition, but not much since we can rely on a
single malloc and read_in_full() for this case.

> +	unsigned char *buf = NULL;
> +	ssize_t cnt = 0;
> +
> +	if (max_request_buffer < req_len) {
> +		die("request was larger than our maximum size (%lu): %lu;"
> +			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +			    max_request_buffer,
> +			    req_len);
> +	}
> +
> +	if (req_len <= 0) {
> +		*out = NULL;
> +		return 0;
> +	}

I was slightly surprised by "<= 0" here. We should never get here with a
negative req_len, since we'd catch that in the read_request() wrapper
here. If we want to document that assumption, should this be
assert(req_len >= 0)?

I'm also puzzled about the behavior with a zero-byte CONTENT_LENGTH.
We'd return NULL here. But in the other read_request_eof() path, we'd
end up with a non-NULL pointer. I'm not sure if it matters to the caller
or not, but it seems like a potential trap.

Is it even worth special-casing here? Our xmalloc and read_in_full
wrappers should handle the zero-byte just fine. I think this whole
conditional could just go away.

-Peff

  parent reply	other threads:[~2017-11-29  3:22 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-29 10:38 [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi Florian Manschwetus
2016-03-29 20:13 ` Jeff King
2016-03-30  9:08   ` AW: " Florian Manschwetus
2016-04-01 23:55     ` Jeff King
2017-11-23 23:45       ` [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875 Max Kirillov
2017-11-24  1:30         ` Eric Sunshine
2017-11-25 21:47           ` Max Kirillov
2017-11-26  0:38             ` Eric Sunshine
2017-11-26  0:43               ` Max Kirillov
2017-11-24  5:54         ` Junio C Hamano
2017-11-24  8:30           ` AW: " Florian Manschwetus
2017-11-26  1:50           ` Max Kirillov
2017-11-26  1:47         ` [PATCH v4 0/2] " Max Kirillov
2017-11-26  1:47           ` [PATCH v4 1/2] " Max Kirillov
2017-11-26  1:47             ` [PATCH v4 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases Max Kirillov
2017-11-26  1:54         ` [PATCH v5 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875 Max Kirillov
2017-11-26  1:54           ` [PATCH v5 1/2] " Max Kirillov
2017-11-26  3:46             ` Junio C Hamano
2017-11-26  8:13               ` Max Kirillov
2017-11-26  9:38                 ` Junio C Hamano
2017-11-26 19:39                   ` Max Kirillov
2017-11-26  1:54           ` [PATCH v5 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases Max Kirillov
2017-11-26 19:38           ` [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875 Max Kirillov
2017-11-26 19:38             ` [PATCH v6 1/2] " Max Kirillov
2017-11-26 22:08               ` Eric Sunshine
2017-11-29  3:22               ` Jeff King [this message]
2017-12-03  1:02                 ` Junio C Hamano
2017-12-03  2:49                   ` Jeff King
2017-12-03  6:07                     ` Junio C Hamano
2017-12-04  7:18                       ` AW: " Florian Manschwetus
2017-12-04 17:13                         ` Jeff King
2017-11-26 19:38             ` [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases Max Kirillov
2017-11-26 22:18               ` Eric Sunshine
2017-11-26 22:40                 ` Max Kirillov
2017-11-29  3:26                   ` Jeff King
2017-11-29  5:19                     ` Max Kirillov
2017-12-03  0:46                       ` Junio C Hamano
2017-11-27  0:29               ` Junio C Hamano
2017-11-27  4:02             ` [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875 Junio C Hamano
2017-11-29  5:07               ` Max Kirillov
2017-12-03  0:48                 ` Junio C Hamano
2017-12-12 16:17                   ` Need to add test artifacts to .gitignore Dan Jacques
2017-12-12 19:00                     ` [RFC PATCH] t/helper: Move sources to t/helper-src; gitignore any files in t/helper Stefan Beller
2017-12-12 19:59                       ` Junio C Hamano
2017-12-12 20:56                         ` [PATCH] t/helper: ignore everything but sources Stefan Beller
2017-12-12 21:06                           ` Junio C Hamano
2017-12-13 20:12                             ` Stefan Beller
2017-12-12 21:06                           ` Todd Zullinger
2017-12-19 22:13             ` [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875 Junio C Hamano
2017-12-20  4:30               ` 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20171129032214.GB32345@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=judge.packham@gmail.com \
    --cc=kostix+git@007spb.ru \
    --cc=manschwetus@cs-software-gmbh.de \
    --cc=max@max630.net \
    --cc=sunshine@sunshineco.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).