> -----Ursprüngliche Nachricht-----
> Von: Jeff King [mailto:p...@peff.net]
> Gesendet: Dienstag, 29. März 2016 22:14
> An: Florian Manschwetus
> Cc: Chris Packham; Konstantin Khomoutov; git@vger.kernel.org
> Betreff: Re: [PATCH] Fix http-backend reading till EOF, ignoring
> CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-
> backend.exe as iis cgi
>
> On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:
>
> > > | A request-body is supplied with the request if the CONTENT_LENGTH
> > > | is not NULL. The server MUST make at least that many bytes
> > > | available for the script to read. The server MAY signal an
> > > | end-of-file condition after CONTENT_LENGTH bytes have been read or
> > > | it MAY supply extension data. Therefore, the script MUST NOT
> > > | attempt to read more than CONTENT_LENGTH bytes, even if more data
> > > | is available. However, it is not obliged to read any of the data.
> > >
> > > So yes, if Git currently reads until EOF, it's an error.
> > > The correct way would be:
> > >
> > > 1) Check to see if the CONTENT_LENGTH variable is available in the
> > > environment. If no, read nothing.
> > >
> > > 2) Otherwise read as many bytes it specifies, and no more.
> > >
> > > 1. https://www.ietf.org/rfc/rfc3875
>
> I don't think the second part of (1) will work very well if the client sends a
> chunked transfer-encoding (which git will do if the input is large). In such a
> case the server would either have to buffer the entire input to find its
> length,
> or stream the data to the CGI without setting $CONTENT_LENGTH. At least
> some servers choose the latter (including Apache).
>
> > diff --git a/http-backend.c b/http-backend.c index 8870a26..94976df
> > 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const
> char *name)
> > */
> > static ssize_t read_request(int fd, unsigned char **out) {
> > - size_t len = 0, alloc = 8192;
> > - unsigned char *buf = xmalloc(alloc);
> > + unsigned char *buf = null;
> > ...
>
> git-am complained that your patch did not apply, but after writing something
> similar locally, I found that t5551.25 hangs indefinitely.
> Which is not surprising. Most tests are doing very limited ref negotiation, so
> the content that hits read_request() here is small, and we send it in a single
> write with a content-length header. But t5551.25 uses a much bigger
> workload, which causes the client to use a chunked transfer-encoding, and
> this code to refuse to read anything (and then the protocol stalls, as we are
> waiting for the client to say something).
>
> So I think you'd want to take a missing CONTENT_LENGTH as a hint to read
> until EOF.
>
> That also raises another issue: what happens in the paths that don't hit
> read_request()? We may also process input via:
>
> - inflate_request(), if the client gzipped it; for well-formed input,
> I think we'll stop reading when the gzip stream tells us there is no
> more data, but a malformed one would have us reading until EOF,
> regardless of what $CONTENT_LENGTH says.
>
> - for input which we expect to be large (like incoming packfiles for a
> push), buffer_input will be unset, and we will pass the descriptor
> directly to a sub-program like git-index-pack. Again, for
> well-formed input it would read just the packfile, but it may
> actually continue to EOF.
>
> So I don't think your patch is covering all cases.
>
> -Peff
After additional analysis it turned out, that in the case you mentioned, at
least IIS, sets CONTENT_LENGTH to -1 resulting in the current behavior of
git-http-backend being sufficient in this situation.
Therefore I refactored the code again a bit, to match up the behavior I
currently fake by using some bash magic...
From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
From: manschwetus <manschwe...@cs-software-gmbh.de>
Date: Tue, 29 Mar 2016 12:16:21 +0200
Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
CONTENT_LENGTH, violating rfc3875
Signed-off-by: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
---
http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 8870a26..94976df 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char
*name)
*/
static ssize_t read_request(int fd, unsigned char **out)
{
- size_t len = 0, alloc = 8192;
- unsigned char *buf = xmalloc(alloc);
+ unsigned char *buf = null;
+ size_t len = 0;
+ /* get request size */
+ size_t req_len = git_env_ulong("CONTENT_LENGTH",
+ 0);
+
+ /* check request size */
+ if (max_request_buffer < req_len) {
+ die("request was larger than our maximum size (%lu);"
+ " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+ max_request_buffer);
+ }
+
+ if (req_len <= 0) {
+ *out = null;
+ return 0;
+ }
+
+ /* allocate buffer */
+ buf = xmalloc(req_len)
- if (max_request_buffer < alloc)
- max_request_buffer = alloc;
while (1) {
ssize_t cnt;
- cnt = read_in_full(fd, buf + len, alloc - len);
+ cnt = read_in_full(fd, buf + len, req_len - len);
if (cnt < 0) {
free(buf);
return -1;
@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)
/* partial read from read_in_full means we hit EOF */
len += cnt;
- if (len < alloc) {
+ if (len < req_len) {
+ /* TODO request incomplete?? */
+ /* maybe just remove this block and condition along
with the loop, */
+ /* if read_in_full is prooven reliable */
*out = buf;
return len;
+ } else {
+ /* request complete */
+ *out = buf;
+ return len;
+
}
-
- /* otherwise, grow and try again (if we can) */
- if (alloc == max_request_buffer)
- die("request was larger than our maximum size (%lu);"
- " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
- max_request_buffer);
-
- alloc = alloc_nr(alloc);
- if (alloc > max_request_buffer)
- alloc = max_request_buffer;
- REALLOC_ARRAY(buf, alloc);
}
}
--
2.7.2.windows.1
From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
From: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
Date: Wed, 30 Mar 2016 10:54:21 +0200
Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved
new variant to read_request_fix_len(...) and introduced read_request(...) as
wrapper, which decides based on value retrieved from CONTENT_LENGTH which
variant to use
Signed-off-by: Florian Manschwetus <manschwe...@cs-software-gmbh.de>
---
http-backend.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 63 insertions(+), 4 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 94976df..3aa0446 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -275,13 +275,52 @@ static struct rpc_service *select_service(const char
*name)
* hit max_request_buffer we die (we'd rather reject a
* maliciously large request than chew up infinite memory).
*/
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
+{
+ size_t len = 0, alloc = 8192;
+ unsigned char *buf = xmalloc(alloc);
+
+ if (max_request_buffer < alloc)
+ max_request_buffer = alloc;
+
+ while (1) {
+ ssize_t cnt;
+
+ cnt = read_in_full(fd, buf + len, alloc - len);
+ if (cnt < 0) {
+ free(buf);
+ return -1;
+ }
+
+ /* partial read from read_in_full means we hit EOF */
+ len += cnt;
+ if (len < alloc) {
+ *out = buf;
+ return len;
+ }
+
+ /* otherwise, grow and try again (if we can) */
+ if (alloc == max_request_buffer)
+ die("request was larger than our maximum size (%lu);"
+ " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+ max_request_buffer);
+
+ alloc = alloc_nr(alloc);
+ if (alloc > max_request_buffer)
+ alloc = max_request_buffer;
+ REALLOC_ARRAY(buf, alloc);
+ }
+}
+
+/*
+ * replacement for original read_request, now renamed to read_request_eof,
+ * honoring given content_length (req_len),
+ * provided by new wrapper function read_request
+ */
+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char
**out)
{
unsigned char *buf = null;
size_t len = 0;
- /* get request size */
- size_t req_len = git_env_ulong("CONTENT_LENGTH",
- 0);
/* check request size */
if (max_request_buffer < req_len) {
@@ -325,6 +364,26 @@ static ssize_t read_request(int fd, unsigned char **out)
}
}
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
+ * to
+ * - use old behaviour of read_request, to read until EOF
+ * => read_request_eof(...)
+ * - just read CONTENT_LENGTH-bytes, when provided
+ * => read_request_fix_len(...)
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+ /* get request size */
+ size_t req_len = git_env_ulong("CONTENT_LENGTH",
+ -1);
+ if (req_len < 0){
+ read_request_eof(fd, out);
+ } else {
+ read_request_fix_len(fd, req_len, out);
+ }
+}
+
static void inflate_request(const char *prog_name, int out, int buffer_input)
{
git_zstream stream;
--
2.7.2.windows.1
Mit freundlichen Grüßen / With kind regards
Florian Manschwetus
CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany
N�����r��y����b�X��ǧv�^�){.n�+����ا���ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf
