> -----Ursprüngliche Nachricht----- > Von: Jeff King [mailto:peff@xxxxxxxx] > Gesendet: Dienstag, 29. März 2016 22:14 > An: Florian Manschwetus > Cc: Chris Packham; Konstantin Khomoutov; git@xxxxxxxxxxxxxxx > 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 <manschwetus@xxxxxxxxxxxxxxxxxxx> 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 <manschwetus@xxxxxxxxxxxxxxxxxxx> --- 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 <manschwetus@xxxxxxxxxxxxxxxxxxx> 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 <manschwetus@xxxxxxxxxxxxxxxxxxx> --- 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��������+%������w��{.n��������n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�