-----Ursprüngliche Nachricht-----
Von: Chris Packham [mailto:judge.packham@gmail.com]
Gesendet: Dienstag, 29. März 2016 11:28
An: Florian Manschwetus
Cc: Konstantin Khomoutov; git@vger.kernel.org
Betreff: Re: Problem with git-http-backend.exe as iis cgi
Hi Florian
On Tue, Mar 29, 2016 at 7:01 PM, Florian Manschwetus [off-list ref] wrote:
Hi,
I put together a first patch for the issue.
Mit freundlichen Grüßen / With kind regards Florian Manschwetus
E-Mail: manschwetus@cs-software-gmbh.de
Tel.: +49-(0)611-8908534
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
Tel.: 0611/8908555
-----Ursprüngliche Nachricht-----
Von: Konstantin Khomoutov [mailto:kostix+git@007spb.ru]
Gesendet: Donnerstag, 10. März 2016 13:55
An: Florian Manschwetus
Cc: git@vger.kernel.org
Betreff: Re: Problem with git-http-backend.exe as iis cgi
On Thu, 10 Mar 2016 07:28:50 +0000
Florian Manschwetus [off-list ref] wrote:
quoted
I tried to setup git-http-backend with iis, as iis provides proper
impersonation for cgi under windows, which leads to have the
filesystem access performed with the logon user, therefore the
webserver doesn't need generic access to the files. I stumbled across
a problem, ending up with post requests hanging forever. After some
investigation I managed to get it work by wrapping the http-backend
into a bash script, giving a lot of control about the environmental
things, I was unable to solve within IIS configuration. The
workaround, I use currently, is to use "/bin/head -c
${CONTENT_LENGTH}
| ./git-http-backend.exe", which directly shows the issue. Git
http-backend should check if CONTENT_LENGTH is set to something
reasonable (e.g. >0) and should in this case read only CONTENT_LENGTH
bytes from stdin, instead of reading till EOF what I suspect it is
doing currently.
The rfc [1] states in its section 4.2:
| 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
Your patch description seems well thought out but if you want someone to notice it you should have a read of https://git.kernel.org/cgit/git/git.git/tree/Documentation/SubmittingPatches
Moin,
I have cloned git and created a more clean patch...
Signed-off-by: Florian Manschwetus <redacted>
---
http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)*/staticssize_tread_request(intfd,unsignedchar**out){-size_tlen=0,alloc=8192;-unsignedchar*buf=xmalloc(alloc);+unsignedchar*buf=null;+size_tlen=0;+/* get request size */+size_treq_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;+return0;+}++/* allocate buffer */+buf=xmalloc(req_len)-if(max_request_buffer<alloc)-max_request_buffer=alloc;while(1){ssize_tcnt;-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;returnlen;+}else{+/* request complete */+*out=buf;+returnlen;+}--/* 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);}}
@@ -701,3 +714,4 @@ int main(int argc, char **argv)cmd->imp(cmd_arg);return0;}+
--
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
From: Jeff King <hidden> Date: 2016-06-15 23:09:06
On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:
quoted
| 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).
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)*/staticssize_tread_request(intfd,unsignedchar**out){-size_tlen=0,alloc=8192;-unsignedchar*buf=xmalloc(alloc);+unsignedchar*buf=null;+size_tlen=0;+/* get request size */+size_treq_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;+return0;+}
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
-----Ursprüngliche Nachricht-----
Von: Jeff King [mailto:peff@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:
quoted
quoted
| 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).
quoted
diff --git a/http-backend.c b/http-backend.c index 8870a26..94976df
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 <redacted>
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 <redacted>
---
http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)*/staticssize_tread_request(intfd,unsignedchar**out){-size_tlen=0,alloc=8192;-unsignedchar*buf=xmalloc(alloc);+unsignedchar*buf=null;+size_tlen=0;+/* get request size */+size_treq_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;+return0;+}++/* allocate buffer */+buf=xmalloc(req_len)-if(max_request_buffer<alloc)-max_request_buffer=alloc;while(1){ssize_tcnt;-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;returnlen;+}else{+/* request complete */+*out=buf;+returnlen;+}--/* 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@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 <manschwetus@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
From: Jeff King <hidden> Date: 2016-06-16 02:18:38
On Wed, Mar 30, 2016 at 09:08:56AM +0000, Florian Manschwetus wrote:
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...
OK, so I'd agree it makes sense to catch "-1", and read to EOF in that
case (or if CONTENT_LENGTH is NULL).
From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
From: manschwetus <redacted>
Date: Tue, 29 Mar 2016 12:16:21 +0200
Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
CONTENT_LENGTH, violating rfc3875
Please send one patch per email, and these header bits should be the
header of your email.
Though we also generally revise and re-send patches, rather than
presenting one patch that has problems and then tacking fixes on top.
I'll ignore the problems in this patch 1, as it looks like it's just the
original one repeated.
From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
From: Florian Manschwetus <redacted>
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
Please use a short subject for your commit message, followed by a blank
line and then a more explanatory body. Also, don't just describe _what_
is happening (we can see that from the diff), but _why_. You can find
more similar tips in SubmittingPatches.
+/**
+ * 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);
+ }
+}
I don't think "if (req_len < 0)" can ever trigger, because size_t is an
unsigned type (and I do not recall what kind of integer overflow
validation we do in git_env_ulong, but I suspect it may complain about
"-1"). You may have to parse the variable manually rather than using
git_env_ulong (i.e., pick out the NULL and "-1" cases, and then feed the
rest to git_parse_ulong()).
Also, a few style nits. We usually omit braces for one-line
conditionals, and please make sure there is whitespace between the
closing parenthesis and the opening brace. There's some discussing in
CodingGuidelines.
-Peff
From: Max Kirillov <hidden> Date: 2017-11-23 23:53:54
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. This causes hang under 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 varibale is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.
Signed-off-by: Florian Manschwetus <redacted>
Authored-by: Florian Manschwetus [off-list ref]
Fixed-by: Max Kirillov [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
Hi
I came across this issue, and I think is should be good to restore the patch.
It is basically same but I squashed them, fixed the thing you mentioned and
also some trivial build failures (null -> NULL and missing return from the wrapper).
I hope I marked it correctly in the trailers.
config.c | 8 +++++++
config.h | 1 +
http-backend.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 80 insertions(+), 1 deletion(-)
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)returnval;}+ssize_tgit_env_ssize_t(constchar*k,ssize_tval)+{+constchar*v=getenv(k);+if(v&&!git_parse_ssize_t(v,&val))+die("failed to parse %s",k);+returnval;+}+intgit_config_system(void){return!git_env_bool("GIT_CONFIG_NOSYSTEM",0);
@@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)}}+/*+*replacementfororiginalread_request,nowrenamedtoread_request_eof,+*honoringgivencontent_length(req_len),+*providedbynewwrapperfunctionread_request+*/+staticssize_tread_request_fix_len(intfd,size_treq_len,unsignedchar**out)+{+unsignedchar*buf=NULL;+size_tlen=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;+return0;+}++/* allocate buffer */+buf=xmalloc(req_len);+++while(1){+ssize_tcnt;++cnt=read_in_full(fd,buf+len,req_len-len);+if(cnt<0){+free(buf);+return-1;+}++/* partial read from read_in_full means we hit EOF */+len+=cnt;+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;+returnlen;+}else{+/* request complete */+*out=buf;+returnlen;++}+}+}++/**+*wrapperfunction,whcihdeterminesbasedonCONTENT_LENGTHvalue,+*to+*-useoldbehaviourofread_request,toreaduntilEOF+*=>read_request_eof(...)+*-justreadCONTENT_LENGTH-bytes,whenprovided+*=>read_request_fix_len(...)+*/+staticssize_tread_request(intfd,unsignedchar**out)+{+/* get request size */+ssize_treq_len=git_env_ssize_t("CONTENT_LENGTH",-1);+if(req_len<0)+returnread_request_eof(fd,out);+else+returnread_request_fix_len(fd,req_len,out);+}+staticvoidinflate_request(constchar*prog_name,intout,intbuffer_input){git_zstreamstream;
From: Eric Sunshine <hidden> Date: 2017-11-24 01:30:45
On Thu, Nov 23, 2017 at 6:45 PM, Max Kirillov [off-list ref] wrote:
[PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875
The "RFC" seems to be missing from the subject line of this unpolished patch.
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. This causes hang under IIS/Windows, for example.
By "_this_ causes a hang", I presume you mean "not respecting
CONTENT_LENGTH causes a hang"? Perhaps that could be spelled out
explicitly.
Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the varibale is not defined, keep older behavior
s/varibale/variable/
quoted hunk
of reading until EOF because it is used to support chunked transfer-encoding.
Signed-off-by: Florian Manschwetus <redacted>
Authored-by: Florian Manschwetus [off-list ref]
Fixed-by: Max Kirillov [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
diff --git a/http-backend.c b/http-backend.c
@@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)+/*+ * replacement for original read_request, now renamed to read_request_eof,+ * honoring given content_length (req_len),+ * provided by new wrapper function read_request+ */
This comment has value only to someone who knew what the code was like
before this change, and it merely repeats what is already implied by
the commit message, rather than providing any valuable information
about this new function itself. Therefore, it should be dropped.
Comment merely repeats what code says, thus has no value. Please drop.
+ 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);
This error message neglects to say what the request size was. Such
information would be useful given that it suggests bumping
GIT_HTTP_MAX_REQUEST_BUFFER to a larger value.
+ while (1) {
+ ssize_t cnt;
+
+ cnt = read_in_full(fd, buf + len, req_len - len);
+ if (cnt < 0) {
+ free(buf);
+ return -1;
+ }
+
+ /* partial read from read_in_full means we hit EOF */
+ len += cnt;
+ 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 */
What is the purpose of the while(1) loop? Every code path inside the
loop returns, so it will never execute more than once. Likewise, why
is 'len' needed?
Rather than writing an entirely new "read" function, how about just
modifying the existing read_request() to optionally limit the read to
a specified number of bytes?
+}
+
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
s/whcih/which/
Also, the placement of commas needs some attention.
+ * 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(...)
+ */
When talking about "old behavior", this comment is repeating
information more suitable to the commit message (and effectively
already covered there); information which only has value to someone
who knew what the old code/behavior was like. The rest of this comment
is merely repeating what the code itself already says, thus adds no
value, so should be dropped.
From: Max Kirillov <hidden> Date: 2017-11-25 21:47:40
Thanks for the review. I saw only reaction of the Jeff in
the original thread and though that it is ok otherwise. I'm
fixing the things you mentioned.
On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
Wrong data type: s/size_t req_len/ssize_t req_len/
Passing negative value to the function makes no sense. I
could add explicit type cast to make it clear. It should be
safe as site_t's range is bigger, and overflown
CONTENT_LENGTH results in die() at parsing (I have a test
which verifies it)
Rather than writing an entirely new "read" function, how about just
modifying the existing read_request() to optionally limit the read to
a specified number of bytes?
From: Eric Sunshine <hidden> Date: 2017-11-26 00:38:39
On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov [off-list ref] wrote:
Thanks for the review. I saw only reaction of the Jeff in
the original thread and though that it is ok otherwise. I'm
fixing the things you mentioned.
The commentary (in which you talked about restoring the patch and
squashing) seemed to imply that this had been posted somewhere before,
but it wasn't marked as "v2" (or whatever attempt) and lacked a URL
pointing at the previous attempt, so it was difficult to judge.
On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
Wrong data type: s/size_t req_len/ssize_t req_len/
Passing negative value to the function makes no sense. I
could add explicit type cast to make it clear. It should be
safe as site_t's range is bigger, and overflown
CONTENT_LENGTH results in die() at parsing (I have a test
which verifies it)
A concern with requesting size_t bytes is that, if it does read all
bytes, that value can't necessarily be represented by the ssize_t
returned from the function. Where would the cast be placed that you
suggest? How do other git functions deal with this sort of situation?
Wrong data type: s/size_t req_len/ssize_t req_len/
Passing negative value to the function makes no sense. I
could add explicit type cast to make it clear. It should be
safe as site_t's range is bigger, and overflown
CONTENT_LENGTH results in die() at parsing (I have a test
which verifies it)
A concern with requesting size_t bytes is that, if it does read all
bytes, that value can't necessarily be represented by the ssize_t
returned from the function. Where would the cast be placed that you
suggest? How do other git functions deal with this sort of situation?
From: Max Kirillov <hidden> Date: 2017-11-26 01:55:07
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov <redacted>
---
Makefile | 1 +
t/helper/test-print-values.c | 10 ++++++++++
t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 41 insertions(+)
create mode 100644 t/helper/test-print-values.c
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' 'expect_aliased1//domain/data.txt'+# overrides existing definition for further cases+run_backend(){+CONTENT_LENGTH="${#2}"&&exportCONTENT_LENGTH&&+(echo"$2"&&cat/dev/zero)|+QUERY_STRING="${1#*[?]}"\+PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}"\+githttp-backend>act.out2>act.err+}++test_expect_success'CONTENT_LENGTH set and infinite input''+confighttp.uploadpacktrue&&+GETinfo/refs?service=git-upload-pack"200 OK"&&+!grep"fatal:.*"act.err&&+POSTgit-upload-pack0000"200 OK"&&+!grep"fatal:.*"act.err+'++test_expect_success'CONTENT_LENGTH overflow ssite_t''+NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values""(size_t)(-20)"`&&+env\+CONTENT_TYPE=application/x-git-upload-pack-request\+QUERY_STRING=/repo.git/git-upload-pack\+PATH_TRANSLATED="$PWD"/.git/git-upload-pack\+GIT_HTTP_EXPORT_ALL=TRUE\+REQUEST_METHOD=POST\+CONTENT_LENGTH="$NOT_FIT_IN_SSIZE"\+githttp-backend</dev/zero>/dev/null2>err&&+grep-q"fatal:.*CONTENT_LENGTH"err+'+ test_done
From: Max Kirillov <hidden> Date: 2017-11-26 01:55:10
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov <redacted>
---
Makefile | 1 +
t/helper/test-print-values.c | 10 ++++++++++
t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 41 insertions(+)
create mode 100644 t/helper/test-print-values.c
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' 'expect_aliased1//domain/data.txt'+# overrides existing definition for further cases+run_backend(){+CONTENT_LENGTH="${#2}"&&exportCONTENT_LENGTH&&+(echo"$2"&&cat/dev/zero)|+QUERY_STRING="${1#*[?]}"\+PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}"\+githttp-backend>act.out2>act.err+}++test_expect_success'CONTENT_LENGTH set and infinite input''+confighttp.uploadpacktrue&&+GETinfo/refs?service=git-upload-pack"200 OK"&&+!grep"fatal:.*"act.err&&+POSTgit-upload-pack0000"200 OK"&&+!grep"fatal:.*"act.err+'++test_expect_success'CONTENT_LENGTH overflow ssite_t''+NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values""(size_t)(-20)"`&&+env\+CONTENT_TYPE=application/x-git-upload-pack-request\+QUERY_STRING=/repo.git/git-upload-pack\+PATH_TRANSLATED="$PWD"/.git/git-upload-pack\+GIT_HTTP_EXPORT_ALL=TRUE\+REQUEST_METHOD=POST\+CONTENT_LENGTH="$NOT_FIT_IN_SSIZE"\+githttp-backend</dev/zero>/dev/null2>err&&+grep-q"fatal:.*CONTENT_LENGTH"err+'+ test_done
From: Max Kirillov <hidden> Date: 2017-11-26 01:55:13
Author: Florian Manschwetus [off-list ref]
Date: Wed, 30 Mar 2016 09:08:56 +0000
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.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 8 ++++++++
config.h | 1 +
http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 47 insertions(+), 1 deletion(-)
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)returnval;}+ssize_tgit_env_ssize_t(constchar*k,ssize_tval)+{+constchar*v=getenv(k);+if(v&&!git_parse_ssize_t(v,&val))+die("failed to parse %s",k);+returnval;+}+intgit_config_system(void){return!git_env_bool("GIT_CONFIG_NOSYSTEM",0);
From: Max Kirillov <hidden> Date: 2017-11-26 01:55:16
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.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 8 ++++++++
config.h | 1 +
http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 47 insertions(+), 1 deletion(-)
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)returnval;}+ssize_tgit_env_ssize_t(constchar*k,ssize_tval)+{+constchar*v=getenv(k);+if(v&&!git_parse_ssize_t(v,&val))+die("failed to parse %s",k);+returnval;+}+intgit_config_system(void){return!git_env_bool("GIT_CONFIG_NOSYSTEM",0);
From: Max Kirillov <hidden> Date: 2017-11-26 19:38:34
From: Florian Manschwetus <redacted>
From: Florian Manschwetus <redacted>
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.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 51 insertions(+), 2 deletions(-)
From: Max Kirillov <hidden> Date: 2017-11-26 19:38:36
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov <redacted>
---
Makefile | 1 +
t/helper/test-print-values.c | 10 ++++++++++
t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 41 insertions(+)
create mode 100644 t/helper/test-print-values.c
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' 'expect_aliased1//domain/data.txt'+# overrides existing definition for further cases+run_backend(){+CONTENT_LENGTH="${#2}"&&exportCONTENT_LENGTH&&+(echo"$2"&&cat/dev/zero)|+QUERY_STRING="${1#*[?]}"\+PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}"\+githttp-backend>act.out2>act.err+}++test_expect_success'CONTENT_LENGTH set and infinite input''+confighttp.uploadpacktrue&&+GETinfo/refs?service=git-upload-pack"200 OK"&&+!grep"fatal:.*"act.err&&+POSTgit-upload-pack0000"200 OK"&&+!grep"fatal:.*"act.err+'++test_expect_success'CONTENT_LENGTH overflow ssite_t''+NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values""(size_t)(-20)"`&&+env\+CONTENT_TYPE=application/x-git-upload-pack-request\+QUERY_STRING=/repo.git/git-upload-pack\+PATH_TRANSLATED="$PWD"/.git/git-upload-pack\+GIT_HTTP_EXPORT_ALL=TRUE\+REQUEST_METHOD=POST\+CONTENT_LENGTH="$NOT_FIT_IN_SSIZE"\+githttp-backend</dev/zero>/dev/null2>err&&+grep-q"fatal:.*CONTENT_LENGTH"err+'+ test_done
From: Eric Sunshine <hidden> Date: 2017-11-26 22:09:08
On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov [off-list ref] wrote:
[...]
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.
[...]
A few small comments below; with the possible exception of one,
probably none worth a re-roll...
This could have been written:
if (cnt < 0) {
free(buf);
return -1;
}
*out = buf;
return cnt;
but not worth a re-roll.
+}
+
+static ssize_t env_content_length(void)
The caller of this function doesn't care how the content length is
being determined -- whether it comes from an environment variable or
is computed some other way; it cares only about the result. Having
"env" in the name ties it to checking only the environment. A more
generic name, such as get_content_length(), would help to decouple the
API from the implementation.
Nevertheless, not worth a re-roll.
+{
+ ssize_t val = -1;
+ const char *str = getenv("CONTENT_LENGTH");
+
+ if (str && !git_parse_ssize_t(str, &val))
git_parse_ssize_t() does the right thing even when 'str' is NULL, so
this condition could be simplified (but not worth a re-roll and may
not improve clarity).
Grabbing and parsing the value from the environment variable is
effectively a one-liner, so env_content_length() could be dropped
altogether, and instead (taking advantage of git_parse_ssize_t()'s
proper NULL-handling):
if (!git_parse_ssize_t(getenv(...), &req_len))
die(...);
Not worth a re-roll.
From: Eric Sunshine <hidden> Date: 2017-11-26 22:19:01
On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov [off-list ref] wrote:
quoted hunk
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data.
(Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
and fail. It does not seem to cause any performance issues with the default
value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
Signed-off-by: Max Kirillov <redacted>
---
Rather than introducing a new 'test' program, would it be possible to
get by with just using 'printf' from the shell?
% printf "%zu\n" -20
18446744073709551596
Perhaps this should return 0 only if it gets the expected argument
"(size_t)(-20)", and return an error otherwise.
Yes, makes sense.
Rather than introducing a new 'test' program, would it be possible to
get by with just using 'printf' from the shell?
% printf "%zu\n" -20
18446744073709551596
I thought about it, of course. But, I am not sure I can
exclude cases when the shell's printf uses 64-bit size_t and
git 32-bit one, or vise-versa. Same way, I cannot say it for
sure for any other software which I might use here instead
of the shell's printf. The only somewhat sure way would be
to use the same compiler, with same settings, which is used
for the production code.
I do not exclude possibility that my reasoning above is
wrong, either in general of specifically for git case. If
there are some examples where it is already used and the
risk of type size mismatch is prevented I could do it
similarly.
From: Jeff King <hidden> Date: 2017-11-29 03:22:22
On Sun, Nov 26, 2017 at 09:38:12PM +0200, Max Kirillov wrote:
From: Florian Manschwetus <redacted>
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.
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.
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
From: Jeff King <hidden> Date: 2017-11-29 03:26:52
On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:
quoted
Rather than introducing a new 'test' program, would it be possible to
get by with just using 'printf' from the shell?
% printf "%zu\n" -20
18446744073709551596
I thought about it, of course. But, I am not sure I can
exclude cases when the shell's printf uses 64-bit size_t and
git 32-bit one, or vise-versa. Same way, I cannot say it for
sure for any other software which I might use here instead
of the shell's printf. The only somewhat sure way would be
to use the same compiler, with same settings, which is used
for the production code.
I do not exclude possibility that my reasoning above is
wrong, either in general of specifically for git case. If
there are some examples where it is already used and the
risk of type size mismatch is prevented I could do it
similarly.
That's definitely something to worry about, and I have a vague
recollection that build differences between the shell environment and
git have bitten us in the past.
That said, we already have some precedent in "git version
--build-options" to report sizes there. Can we do something like the
patch below instead of adding a new test helper?
From: Max Kirillov <hidden> Date: 2017-11-29 05:19:28
On Tue, Nov 28, 2017 at 10:26:33PM -0500, Jeff King wrote:
quoted hunk
On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:
That said, we already have some precedent in "git version
--build-options" to report sizes there. Can we do something like the
patch below instead of adding a new test helper?
@@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)if(build_options){printf("sizeof-long: %d\n",(int)sizeof(long));+printf("sizeof-size_t: %d\n",(int)sizeof(size_t));/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */}return0;
Thank you! I knew there should have been something.
If nobody objects changing the user-visible behavior, I'll
consider using this.
PS: I'll respond to your other reply a bit later.
From: Max Kirillov <hidden> Date: 2018-06-02 21:39:37
It's been time. Thank you for parience.
Changes:
* did most of the changes proposed
* rebase to newer master (latest conflicting change is addition of combined test helper)
* make tests which cover, hopefully, all cases.
* handle incorectly truncated input also in receive-pack. Considering the complications
pointed out by Jeff, it just filters the input in the frontend process. I hope it
is acceptable thing to do.
Max Kirillov (2):
http-backend: respect CONTENT_LENGTH as specified by rfc3875
http-backend: respect CONTENT_LENGTH for receive-pack
Makefile | 1 +
config.c | 2 +-
config.h | 1 +
http-backend.c | 86 +++++++++++--
t/helper/test-print-larger-than-ssize.c | 11 ++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t5560-http-backend-noserver.sh | 13 ++
t/t5562-http-backend-content-length.sh | 155 ++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 30 +++++
10 files changed, 291 insertions(+), 10 deletions(-)
create mode 100644 t/helper/test-print-larger-than-ssize.c
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
--
2.17.0.1185.g782057d875
From: Max Kirillov <hidden> Date: 2018-06-02 21:39:37
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.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 44 insertions(+), 2 deletions(-)
From: Max Kirillov <hidden> Date: 2018-06-02 21:39:38
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data, with all combinations
of variations: fetch or push, plain or compressed body, correct or truncated
input.
* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
Makefile | 1 +
http-backend.c | 49 ++++++--
t/helper/test-print-larger-than-ssize.c | 11 ++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t5560-http-backend-noserver.sh | 13 ++
t/t5562-http-backend-content-length.sh | 155 ++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 30 +++++
8 files changed, 250 insertions(+), 11 deletions(-)
create mode 100644 t/helper/test-print-larger-than-ssize.c
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
@@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)if(full_request)n=0;/* nothing left to read */else-n=read_request(0,&full_request);+n=read_request(0,&full_request,req_len);stream.next_in=full_request;}else{-n=xread(0,in_buf,sizeof(in_buf));+ssize_tbuffer_len;+if(req_remaining_len<0||req_remaining_len>sizeof(in_buf))+buffer_len=sizeof(in_buf);+else+buffer_len=req_remaining_len;+n=xread(0,in_buf,buffer_len);stream.next_in=in_buf;+if(req_remaining_len>=0)+req_remaining_len-=n;}if(n<=0)
@@ -416,10 +422,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)free(full_request);}-staticvoidcopy_request(constchar*prog_name,intout)+staticvoidcopy_request(constchar*prog_name,intout,ssize_treq_len){unsignedchar*buf;-ssize_tn=read_request(0,&buf);+ssize_tn=read_request(0,&buf,req_len);if(n<0)die_errno("error reading request body");if(write_in_full(out,buf,n)<0)
@@ -0,0 +1,30 @@+#!/usr/bin/perl+use5.008;+usestrict;+usewarnings;++my$body_filename=$ARGV[0];+my@command=@ARGV[1..$#ARGV];++#readdata+my$body_size=-s$body_filename;+$ENV{"CONTENT_LENGTH"}=$body_size;+open(my$body_fh,"<",$body_filename)ordie"Cannot open $body_filename: $!";+my$body_data;+definedread($body_fh,$body_data,$body_size)ordie"Cannot read $body_filename: $!";+close($body_fh);++my$exited=0;+$SIG{"CHLD"}=sub{+$exited=1;+};++#writedata+my$pid=open(my$out,"|-",@command);+definedsyswrite($out,$body_data)ordie"Cannot write data: $!";++sleep1;#isinterruptedbySIGCHLD+if(!$exited){+close($out);+die"Command did not exit after reading whole body";+}
From: Jeff King <hidden> Date: 2018-06-04 03:44:07
On Sun, Jun 03, 2018 at 12:27:48AM +0300, Max Kirillov wrote:
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.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 44 insertions(+), 2 deletions(-)
This first patch looks good to me, though it may be worth mentioning in
the commit message that we're only handling the buffered-input side here
(that is obvious to anybody reading this whole series now, but it may
help out people digging in the history later).
-Peff
From: Jeff King <hidden> Date: 2018-06-04 04:44:14
On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
I think this approach is reasonable. It's basically converting the
known-length case to a read-to-eof case for the sub-program, which
should paper over any problems of this type. And it's what we really
_want_ the web server to be doing in the first place.
Since this is slightly less efficient, and because it only matters if
the web server does not already close the pipe, should this have a
run-time configuration knob, even if it defaults to
safe-but-slightly-slower?
I admit I don't overly care that much myself (the only large-scale Git
server deployment I am personally familiar with does not use
git-http-backend at all), but it might be nice to leave an escape hatch.
There are a few things in the patch worth fixing, but overall I think it
looks like a pretty good direction. Comments inline.
Minor nit, but it might have been nice to build in this infrastructure
in the first patch, rather than refactoring it here. It would also make
it much more obvious that the first one is not handling some cases,
since we'd have "req_len" but not pass it to all of the code paths. ;)
quoted hunk
@@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int out, int buffer_input) if (full_request) n = 0; /* nothing left to read */ else- n = read_request(0, &full_request);+ n = read_request(0, &full_request, req_len); stream.next_in = full_request; } else {- n = xread(0, in_buf, sizeof(in_buf));+ ssize_t buffer_len;+ if (req_remaining_len < 0 || req_remaining_len > sizeof(in_buf))+ buffer_len = sizeof(in_buf);+ else+ buffer_len = req_remaining_len;+ n = xread(0, in_buf, buffer_len); stream.next_in = in_buf;+ if (req_remaining_len >= 0)+ req_remaining_len -= n; }
What happens here if xread() returns an error? We probably don't want to
modify req_remaining_len (it probably doesn't matter since we'd report
the errot after this, but it feels funny not to check here).
I was going to complain that we usually start our error messages with a
lowercase, but this program seems to be an exception. So here you've
followed the local custom, which is OK.
We don't necessarily know why the write failed. If it's EPIPE, then yes,
the program probably did abort. But all we know is that write() failed.
We should probably say something more generic like:
die_errno("unable to write to '%s'");
or similar.
I'm not sure if these messages should be marked for translation. If so,
you'd want test_i18ngrep here.
We also generally avoid "-q" to grep. If the script is in non-verbose
mode it will go to /dev/null anyway, and in verbose mode it's useful to
see (possibly ditto for the /dev/null redirection of stdout above, but I
think that might actually spew a binary packfile if the test fails,
which we'd rather avoid).
Why is the too-large CONTENT_LENGTH test in another file? I'd have
thought it would go well here, based on the description.
+verify_http_result() {
+ # sometimes there is fatal error buit the result is still 200
+ if grep 'fatal:' act.err
+ then
+ return 1
+ fi
+
+ if ! grep "Status" act.out >act
+ then
+ printf "Status: 200 OK\r\n" >act
+ fi
+ printf "Status: $1\r\n" >exp &&
+ test_cmp exp act
+}
200 with a fatal error sounds non-ideal. But I think it's unavoidable in
some cases where we see write failures, etc.
I think this env (and the earlier one) are not strictly necessary, as
you could just use shell one-shot variables. But I'm OK with them as an
abundance of caution, since in theory a caller could use a shell
function rather than a real command here (in which case one-shot
variables do the wrong thing).
This depends on the size of the hash. That's always 40 for now, but is
something that may change soon.
We already have a packetize() helper; could we use it here?
(Looking at the definition of that helper, it's actually kind of
expensive in terms of number of processes. We could perhaps convert it
to perl and do it all in a single process, but that's orthogonal to your
series).
+gzip -k fetch_body
We don't unconditionally rely on gzip elsewhere. The test blocks using
it (and the ones that depend on them) should be marked with the GZIP
prerequisite.
We can into portability problems with "head -c", but I think they were
mostly with different buffering behavior (i.e., reading more than 10
bytes). And that would be OK in this setting, since nobody is going to
read the rest of the input after us.
So it's probably OK, but we could use "test_copy_bytes 10" here if it
isn't.
Usually test_must_fail on a checking function like this is a sign that
the check is not as robust as we'd like. If the function checks two
things "A && B", then checking test_must_fail will only let us know
"!A || !B", but you probably want to check both.
The usual solution is for verify_http_result to take an optional "!" in
the first parameter and invert its sense. Or to just split it into two
separate functions.
(We'd also generally not use test_must_fail with a non-git command, and
just use a simple "! verify_http_result"; that would apply equally if
gets split into two commands).
This will persist after the test finishes. Try:
test_config http.receivepack true
which will clean up after the test finishes. Alternatively, since I
think you'd want this whole script to run with http.receivepack set,
this could be part of the repository setup in the earlier steps (and
then _don't_ use test_config, because the whole point is for it to
persist).
Should this "git branch -D" go into a "test_when_finished" block closer
to when it is created?
+# write data
+my $pid = open(my $out, "|-", @command);
+defined syswrite($out, $body_data) or die "Cannot write data: $!";
I assume perl's syswrite() has the usual write() pitfalls, like
sometimes returning without writing all of the bytes. Could this just
be:
print $out $body_date;
?
+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+ close($out);
+ die "Command did not exit after reading whole body";
+}
A sleep like this is a recipe for having the test fail when the system
is under heavy load and it takes the sub-process more than a second to
return (and the SIGCHLD to get delivered).
Normally I'd suggest wait() or pause(), but I think the intent is to
sleep because in the failure case we'd never see the signal, and just
hang? If so, then perhaps we should give a much higher sleep, like 60
seconds. That will mean the test eventually does report failure, but
should be much less likely to cause a false negative. And if we do get
the signal (which we'd usually expect), then we exit immediately.
Also, do we need to protect ourselves against other signals being
delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
it going to erroneously end the sleep and say "nope, no exited signal"?
My read through the tests was mostly looking for mechanical problems. I
didn't give much though to whether we were getting full coverage, and
now it's my bed-time here. So I'll leave that for later (or somebody
else).
-Peff
From: Max Kirillov <hidden> Date: 2018-06-04 22:25:35
On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:
Thanks for the comments, I will do the things you proposed,
or try to and get back later if there are any issues. Some
notes below.
On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:
Since this is slightly less efficient, and because it only matters if
the web server does not already close the pipe, should this have a
run-time configuration knob, even if it defaults to
safe-but-slightly-slower?
Personally, I of course don't want this. Also, I don't think
the difference is much noticeable. But you can never be sure
without trying. I'll try to measure some numbers.
We don't necessarily know why the write failed. If it's EPIPE, then yes,
the program probably did abort. But all we know is that write() failed.
We should probably say something more generic like:
die_errno("unable to write to '%s'");
or similar.
Actually, it is already 3rd same error in this file. Maybe
deserve some refactoring. I will change the message also.
This depends on the size of the hash. That's always 40 for now, but is
something that may change soon.
We already have a packetize() helper; could we use it here?
Could you point me to it? I cannot find it.
My understanfing is that the current protocol assumes
40 symbols hash, so another hash length would be another
protocol, and since it's manually forged here it would
anyway has to be changeda.
Usually test_must_fail on a checking function like this is a sign that
the check is not as robust as we'd like. If the function checks two
things "A && B", then checking test_must_fail will only let us know
"!A || !B", but you probably want to check both.
Well here I just want to know that the request has failed,
and we already know that it can fail in different ways,
but the test is not going to differentiate those ways.
(We'd also generally not use test_must_fail with a non-git command, and
just use a simple "! verify_http_result"; that would apply equally if
gets split into two commands).
Will use ! there.
quoted
+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+ close($out);
+ die "Command did not exit after reading whole body";
+}
...
Also, do we need to protect ourselves against other signals being
delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
it going to erroneously end the sleep and say "nope, no exited signal"?
I'll check, but what could I do? Should I add blocking other
signals there?
From: Max Kirillov <hidden> Date: 2018-06-10 15:13:18
As explained in [1], we should not assume the reason why the writing has
failed, and even if the reason is that child has existed not the reason
why it have done so. So instead just say that writing has failed.
[1] https://public-inbox.org/git/20180604044408.GD14451@sigill.intra.peff.net/
Signed-off-by: Max Kirillov <redacted>
---
http-backend.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
From: Max Kirillov <hidden> Date: 2018-06-10 15:13:19
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.
This commit only fixes buffered input, whcih reads whole body before
processign it. Non-buffered input is going to be fixed in subsequent commit.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 49 insertions(+), 8 deletions(-)
@@ -326,7 +326,46 @@ static ssize_t read_request(int fd, unsigned char **out)}}-staticvoidinflate_request(constchar*prog_name,intout,intbuffer_input)+staticssize_tread_request_fixed_len(intfd,ssize_treq_len,unsignedchar**out)+{+unsignedchar*buf=NULL;+ssize_tcnt=0;++if(max_request_buffer<req_len){+die("request was larger than our maximum size (%lu): "+"%"PRIuMAX"; try setting GIT_HTTP_MAX_REQUEST_BUFFER",+max_request_buffer,(uintmax_t)req_len);+}++buf=xmalloc(req_len);+cnt=read_in_full(fd,buf,req_len);+if(cnt<0){+free(buf);+return-1;+}+*out=buf;+returncnt;+}++staticssize_tget_content_length(void)+{+ssize_tval=-1;+constchar*str=getenv("CONTENT_LENGTH");++if(str&&!git_parse_ssize_t(str,&val))+die("failed to parse CONTENT_LENGTH: %s",str);+returnval;+}++staticssize_tread_request(intfd,unsignedchar**out,ssize_treq_len)+{+if(req_len<0)+returnread_request_eof(fd,out);+else+returnread_request_fixed_len(fd,req_len,out);+}++staticvoidinflate_request(constchar*prog_name,intout,intbuffer_input,ssize_treq_len){git_zstreamstream;unsignedchar*full_request=NULL;
@@ -344,7 +383,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)if(full_request)n=0;/* nothing left to read */else-n=read_request(0,&full_request);+n=read_request(0,&full_request,req_len);stream.next_in=full_request;}else{n=xread(0,in_buf,sizeof(in_buf));
@@ -380,10 +419,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)free(full_request);}-staticvoidcopy_request(constchar*prog_name,intout)+staticvoidcopy_request(constchar*prog_name,intout,ssize_treq_len){unsignedchar*buf;-ssize_tn=read_request(0,&buf);+ssize_tn=read_request(0,&buf,req_len);if(n<0)die_errno("error reading request body");write_to_child(out,buf,n,prog_name);
From: Max Kirillov <hidden> Date: 2018-06-10 15:13:24
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data, with all combinations
of variations: fetch or push, plain or compressed body, correct or truncated
input.
* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
help.c | 1 +
http-backend.c | 32 ++++-
t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 37 ++++++
4 files changed, 237 insertions(+), 2 deletions(-)
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
@@ -419,6 +419,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)elseprintf("no commit associated with this build\n");printf("sizeof-long: %d\n",(int)sizeof(long));+printf("sizeof-size_t: %d\n",(int)sizeof(size_t));/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */}return0;
@@ -0,0 +1,37 @@+#!/usr/bin/perl+use5.008;+usestrict;+usewarnings;++my$body_filename=$ARGV[0];+my@command=@ARGV[1..$#ARGV];++#readdata+my$body_size=-s$body_filename;+$ENV{"CONTENT_LENGTH"}=$body_size;+open(my$body_fh,"<",$body_filename)ordie"Cannot open $body_filename: $!";+my$body_data;+definedread($body_fh,$body_data,$body_size)ordie"Cannot read $body_filename: $!";+close($body_fh);++my$exited=0;+$SIG{"CHLD"}=sub{+$exited=1;+};++#writedata+my$pid=open(my$out,"|-",@command);+{+#disablebufferingat$out+my$old_selected=select;+select$out;+$|=1;+select$old_selected;+}+print$out$body_dataordie"Cannot write data: $!";++sleep60;#isinterruptedbySIGCHLD+if(!$exited){+close($out);+die"Command did not exit after reading whole body";+}
From: Max Kirillov <hidden> Date: 2018-06-10 15:13:44
On Tue, Jun 05, 2018 at 01:18:08AM +0300, Max Kirillov wrote:
On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:
quoted
Since this is slightly less efficient, and because it only matters if
the web server does not already close the pipe, should this have a
run-time configuration knob, even if it defaults to
safe-but-slightly-slower?
Personally, I of course don't want this. Also, I don't think
the difference is much noticeable. But you can never be sure
without trying. I'll try to measure some numbers.
It seems to be challenging to see any effect at my system.
At least not with any real operation because changing
references needs IO and index-pack needs CPU so. I'll try
it some more.
quoted
We should probably say something more generic like:
die_errno("unable to write to '%s'");
or similar.
Actually, it is already 3rd same error in this file. Maybe
deserve some refactoring. I will change the message also.
Extracted the writing and refactoring to a single function,
also fixed the message.
This depends on the size of the hash. That's always 40 for now, but is
something that may change soon.
We already have a packetize() helper; could we use it here?
Could you point me to it? I cannot find it.
Sorry, misread it as packetSize. Found and used.
quoted
Also, do we need to protect ourselves against other signals being
delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
it going to erroneously end the sleep and say "nope, no exited signal"?
I'll check, but what could I do? Should I add blocking other
signals there?
In my Linux I don't see the signal. Except that, there seem to
be not that many ignored signals. Anyway, I don't see what
could be done bout it.
I'm not sure if these messages should be marked for translation. If so,
you'd want test_i18ngrep here.
Message localization does not seem to be used in
http-backend at all. It makes sense - server-side software
probably does not know who is the user on the other side, if
the message gets to the user at all. So, I think the
message should not be translated.
I'm not sure if these messages should be marked for translation. If so,
you'd want test_i18ngrep here.
Message localization does not seem to be used in
http-backend at all. It makes sense - server-side software
probably does not know who is the user on the other side, if
the message gets to the user at all. So, I think the
message should not be translated.
OK. I think there's been talk of localizing "fatal:", but whoever does
that patch would have to deal with fallout all over the test-suite. I
don't think we need to worry about it yet.
-Peff
From: Jeff King <hidden> Date: 2018-06-11 09:18:18
On Tue, Jun 05, 2018 at 01:18:08AM +0300, Max Kirillov wrote:
quoted
On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:
Since this is slightly less efficient, and because it only matters if
the web server does not already close the pipe, should this have a
run-time configuration knob, even if it defaults to
safe-but-slightly-slower?
Personally, I of course don't want this. Also, I don't think
the difference is much noticeable. But you can never be sure
without trying. I'll try to measure some numbers.
I don't know if it will matter or not. I just wonder if we want to leave
an escape hatch for people who might. I could take or leave it.
Actually, it is already 3rd same error in this file. Maybe
deserve some refactoring. I will change the message also.
Thanks, that kind of related cleanup is very welcome.
quoted
We generally prefer to have all commands, even ones we don't expect to
fail, inside test_expect blocks (e.g., with a "setup" description).
Will the defined variables get to the next test? I'll try to
do as you describe.
Yes, the tests are all run as evals. So as long as you don't open a
subshell yourself, any changes you make to process state will persist.
Usually test_must_fail on a checking function like this is a sign that
the check is not as robust as we'd like. If the function checks two
things "A && B", then checking test_must_fail will only let us know
"!A || !B", but you probably want to check both.
Well here I just want to know that the request has failed,
and we already know that it can fail in different ways,
but the test is not going to differentiate those ways.
OK, looking over your verify_http_result function, I _think_ we are OK
here, because the only && is against a printf, which we wouldn't really
expect to fail.
quoted
quoted
+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+ close($out);
+ die "Command did not exit after reading whole body";
+}
quoted
Also, do we need to protect ourselves against other signals being
delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
it going to erroneously end the sleep and say "nope, no exited signal"?
I'll check, but what could I do? Should I add blocking other
signals there?
I think a more robust check may be to waitpid() on the child for up to N
seconds. Something like this:
$SIG{ALRM} = sub {
kill(9, $pid);
die "command did not exit after reading whole body"
};
alarm(60);
waitpid($pid, 0);
alarm(0);
That should exit immediately if $pid does, and otherwise die after
exactly 60 seconds. Perl's waitpid implementation will restart
automatically if it gets another signal.
-Peff
From: Jeff King <hidden> Date: 2018-06-11 09:24:45
On Mon, Jun 11, 2018 at 05:18:13AM -0400, Jeff King wrote:
quoted
quoted
quoted
+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+ close($out);
+ die "Command did not exit after reading whole body";
+}
quoted
Also, do we need to protect ourselves against other signals being
delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
it going to erroneously end the sleep and say "nope, no exited signal"?
I'll check, but what could I do? Should I add blocking other
signals there?
I think a more robust check may be to waitpid() on the child for up to N
seconds. Something like this:
$SIG{ALRM} = sub {
kill(9, $pid);
die "command did not exit after reading whole body"
};
alarm(60);
waitpid($pid, 0);
alarm(0);
That should exit immediately if $pid does, and otherwise die after
exactly 60 seconds. Perl's waitpid implementation will restart
automatically if it gets another signal.
I tried your original, delivering some signals to it. I think it
actually is OK, too, because perl's sleep() implementation will also
restart for something like SIGWINCH.
E.g., stracing looks like this:
nanosleep({tv_sec=60, tv_nsec=0}, {tv_sec=57, tv_nsec=791891377}) = ? ERESTART_RESTARTBLOCK (Interrupted by signal)
--- SIGWINCH {si_signo=SIGWINCH, si_code=SI_KERNEL} ---
restart_syscall(<... resuming interrupted nanosleep ...>
-Peff
From: SZEDER Gábor <hidden> Date: 2018-07-25 12:14:45
[Hrm, this time with hopefully proper In-Reply-To: header.
Sorry for the double post.]
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data, with all combinations
of variations: fetch or push, plain or compressed body, correct or truncated
input.
* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
help.c | 1 +
http-backend.c | 32 ++++-
t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 37 ++++++
4 files changed, 237 insertions(+), 2 deletions(-)
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
@@ -0,0 +1,169 @@+#!/bin/sh++test_description='test git-http-backend respects CONTENT_LENGTH'+../test-lib.sh++test_lazy_prereqGZIP'gzip --version'++verify_http_result(){+# sometimes there is fatal error buit the result is still 200
s/buit/but/
+ if grep 'fatal:' act.err
+ then
+ return 1
+ fi
I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.
What does that "sometimes" in the above comment mean, and how often
does such a failure happen? I see these patches are in 'pu' for over
a month now, so based on the number of reflog entries since then it
happened once from about 30-35 builds on Travis CI so far.
I don't really like the idea of adding a bunch of flaky test cases...
we have enough of them already, unfortunately.
Don't save the standard error of the whole shell function.
When running the test with /bin/sh and '-x' tracing, then the trace of
commands executed in the function will be included in the standard
error as well, which may interfere with later verification (though in
this case it doesn't seem like it would cause any issues).
Please limit the redirections to the relevant command's output. AFAICT
all invocations of 'test_http_env' in these tests have their stdout and
stderr redirected to the same pair of files, so perhaps you could
simply move all these redirections inside the function.
If this command were to print a "fatal: ..." message to its standard
error, then ...
+ ! verify_http_result "200 OK"
... this function would return error (because of that 'if grep fatal:
...' statement) without even looking at the status, but the test would
still succeed. Is that really the desired behavior here?
@@ -0,0 +1,37 @@+#!/usr/bin/perl+use5.008;+usestrict;+usewarnings;++my$body_filename=$ARGV[0];+my@command=@ARGV[1..$#ARGV];++#readdata+my$body_size=-s$body_filename;+$ENV{"CONTENT_LENGTH"}=$body_size;+open(my$body_fh,"<",$body_filename)ordie"Cannot open $body_filename: $!";+my$body_data;+definedread($body_fh,$body_data,$body_size)ordie"Cannot read $body_filename: $!";+close($body_fh);++my$exited=0;+$SIG{"CHLD"}=sub{+$exited=1;+};++#writedata+my$pid=open(my$out,"|-",@command);+{+#disablebufferingat$out+my$old_selected=select;+select$out;+$|=1;+select$old_selected;+}+print$out$body_dataordie"Cannot write data: $!";++sleep60;#isinterruptedbySIGCHLD+if(!$exited){+close($out);+die"Command did not exit after reading whole body";+}
From: Max Kirillov <hidden> Date: 2018-07-25 14:51:08
On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:
quoted
+ # sometimes there is fatal error buit the result is still 200
s/buit/but/
Thanks, will fix
quoted
+ if grep 'fatal:' act.err
+ then
+ return 1
+ fi
I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.
Did it happen once or repeated? It is rather strange, that
one shoud not fail. Which OS it was?
There have been doubds that a random incoming signal can
trigger such a failure.
What does that "sometimes" in the above comment mean, and how often
does such a failure happen? I see these patches are in 'pu' for over
a month now, so based on the number of reflog entries since then it
happened once from about 30-35 builds on Travis CI so far.
"sometimes" here means "for some kinds of fatal error
failure", there is nothing random in it.
Don't save the standard error of the whole shell function.
When running the test with /bin/sh and '-x' tracing, then the trace of
commands executed in the function will be included in the standard
error as well, which may interfere with later verification (though in
this case it doesn't seem like it would cause any issues).
Please limit the redirections to the relevant command's output. AFAICT
all invocations of 'test_http_env' in these tests have their stdout and
stderr redirected to the same pair of files, so perhaps you could
simply move all these redirections inside the function.
Thanks, I'll try to fix it
quoted
+ ! verify_http_result "200 OK"
... this function would return error (because of that 'if grep fatal:
...' statement) without even looking at the status, but the test would
still succeed. Is that really the desired behavior here?
Yes, it is a desired behavior. A failure is expected here,
and the failure does not show up as non-200 status, as
described above.
From: SZEDER Gábor <hidden> Date: 2018-07-25 18:41:46
On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov [off-list ref] wrote:
On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:
quoted
quoted
+ # sometimes there is fatal error buit the result is still 200
quoted
quoted
+ if grep 'fatal:' act.err
+ then
+ return 1
+ fi
I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.
Did it happen once or repeated? It is rather strange, that
one shoud not fail. Which OS it was?
Only once, so far. It was one of my OSX build jobs on Travis CI, but
I don't know what OSX version is used.
'act.err' contained this (which will get line-wrapped, I'm afraid):
++handler_type=receive
++shift
++env CONTENT_TYPE=application/x-git-receive-pack-request
QUERY_STRING=/repo.git/git-receive-pack
'PATH_TRANSLATED=/Users/travis/t/trash
dir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE
REQUEST_METHOD=POST
/Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl
push_body git http-backend
<...128 zero bytes...>fatal: the remote end hung up unexpectedly
I couldn't reproduce it on my Linux box.
There have been doubds that a random incoming signal can
trigger such a failure.
quoted
What does that "sometimes" in the above comment mean, and how often
does such a failure happen? I see these patches are in 'pu' for over
a month now, so based on the number of reflog entries since then it
happened once from about 30-35 builds on Travis CI so far.
"sometimes" here means "for some kinds of fatal error
failure", there is nothing random in it.
quoted
quoted
+ ! verify_http_result "200 OK"
... this function would return error (because of that 'if grep fatal:
...' statement) without even looking at the status, but the test would
still succeed. Is that really the desired behavior here?
Yes, it is a desired behavior. A failure is expected here,
and the failure does not show up as non-200 status, as
described above.
OK, then I misunderstood that comment.
Perhaps a different wording could make it slightly better? E.g. "In
some of these tests ..." instead of that "sometimes". Dunno.
From: Max Kirillov <hidden> Date: 2018-07-26 04:40:13
On Wed, Jul 25, 2018 at 08:41:31PM +0200, SZEDER Gábor wrote:
On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov [off-list ref] wrote:
quoted
quoted
I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.
Did it happen once or repeated? It is rather strange, that
one shoud not fail. Which OS it was?
Only once, so far. It was one of my OSX build jobs on Travis CI, but
I don't know what OSX version is used.
'act.err' contained this (which will get line-wrapped, I'm afraid):
++handler_type=receive
++shift
++env CONTENT_TYPE=application/x-git-receive-pack-request
QUERY_STRING=/repo.git/git-receive-pack
'PATH_TRANSLATED=/Users/travis/t/trash
dir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE
REQUEST_METHOD=POST
/Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl
push_body git http-backend
<...128 zero bytes...>fatal: the remote end hung up unexpectedly
I couldn't reproduce it on my Linux box.
The only reason for this I could imagine is some perl
utility failure to feed the body to git http-backend.
I could not reproduce it either, but if such things happen
often again maybe should concider C helper instead. Though
I'm afraid I easily can make more mistakes in it than perl
interpreter authors.
I'll make the other changes, and sofar just hope it would
not happen again.
From: Max Kirillov <hidden> Date: 2018-07-27 03:49:09
* fix the gzip usage as suggested in https://public-inbox.org/git/xmqqk1quvegh.fsf@gitster-ct.c.googlers.com/
* better explanation of why status check is needed
* redirect only the helper call, not the whole shell function, also move more into the shell function
Max Kirillov (3):
http-backend: cleanup writing to child process
http-backend: respect CONTENT_LENGTH as specified by rfc3875
http-backend: respect CONTENT_LENGTH for receive-pack
config.c | 2 +-
config.h | 1 +
help.c | 1 +
http-backend.c | 100 +++++++++++++---
t/t5562-http-backend-content-length.sh | 155 +++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 37 ++++++
6 files changed, 281 insertions(+), 15 deletions(-)
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
--
2.17.0.1185.g782057d875
From: Max Kirillov <hidden> Date: 2018-07-27 03:49:12
As explained in [1], we should not assume the reason why the writing has
failed, and even if the reason is that child has existed not the reason
why it have done so. So instead just say that writing has failed.
[1] https://public-inbox.org/git/20180604044408.GD14451@sigill.intra.peff.net/
Signed-off-by: Max Kirillov <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
http-backend.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
From: Max Kirillov <hidden> Date: 2018-07-27 03:49:15
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.
This commit only fixes buffered input, whcih reads whole body before
processign it. Non-buffered input is going to be fixed in subsequent commit.
Signed-off-by: Florian Manschwetus <redacted>
[mk: fixed trivial build failures and polished style issues]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
config.c | 2 +-
config.h | 1 +
http-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 49 insertions(+), 8 deletions(-)
@@ -327,7 +327,46 @@ static ssize_t read_request(int fd, unsigned char **out)}}-staticvoidinflate_request(constchar*prog_name,intout,intbuffer_input)+staticssize_tread_request_fixed_len(intfd,ssize_treq_len,unsignedchar**out)+{+unsignedchar*buf=NULL;+ssize_tcnt=0;++if(max_request_buffer<req_len){+die("request was larger than our maximum size (%lu): "+"%"PRIuMAX"; try setting GIT_HTTP_MAX_REQUEST_BUFFER",+max_request_buffer,(uintmax_t)req_len);+}++buf=xmalloc(req_len);+cnt=read_in_full(fd,buf,req_len);+if(cnt<0){+free(buf);+return-1;+}+*out=buf;+returncnt;+}++staticssize_tget_content_length(void)+{+ssize_tval=-1;+constchar*str=getenv("CONTENT_LENGTH");++if(str&&!git_parse_ssize_t(str,&val))+die("failed to parse CONTENT_LENGTH: %s",str);+returnval;+}++staticssize_tread_request(intfd,unsignedchar**out,ssize_treq_len)+{+if(req_len<0)+returnread_request_eof(fd,out);+else+returnread_request_fixed_len(fd,req_len,out);+}++staticvoidinflate_request(constchar*prog_name,intout,intbuffer_input,ssize_treq_len){git_zstreamstream;unsignedchar*full_request=NULL;
@@ -345,7 +384,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)if(full_request)n=0;/* nothing left to read */else-n=read_request(0,&full_request);+n=read_request(0,&full_request,req_len);stream.next_in=full_request;}else{n=xread(0,in_buf,sizeof(in_buf));
@@ -381,10 +420,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)free(full_request);}-staticvoidcopy_request(constchar*prog_name,intout)+staticvoidcopy_request(constchar*prog_name,intout,ssize_treq_len){unsignedchar*buf;-ssize_tn=read_request(0,&buf);+ssize_tn=read_request(0,&buf,req_len);if(n<0)die_errno("error reading request body");write_to_child(out,buf,n,prog_name);
From: Max Kirillov <hidden> Date: 2018-07-27 03:49:18
Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
As it gets complicated to correctly track the data length, instead transfer
the data through parent process and cut the pipe as the specified length is
reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
directly to the forked commands.
Add tests for cases:
* CONTENT_LENGTH is set, script's stdin has more data, with all combinations
of variations: fetch or push, plain or compressed body, correct or truncated
input.
* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Max Kirillov <redacted>
---
help.c | 1 +
http-backend.c | 32 ++++-
t/t5562-http-backend-content-length.sh | 155 +++++++++++++++++++++++++
t/t5562/invoke-with-content-length.pl | 37 ++++++
4 files changed, 223 insertions(+), 2 deletions(-)
create mode 100755 t/t5562-http-backend-content-length.sh
create mode 100755 t/t5562/invoke-with-content-length.pl
@@ -609,6 +609,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)elseprintf("no commit associated with this build\n");printf("sizeof-long: %d\n",(int)sizeof(long));+printf("sizeof-size_t: %d\n",(int)sizeof(size_t));/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */}return0;
@@ -0,0 +1,37 @@+#!/usr/bin/perl+use5.008;+usestrict;+usewarnings;++my$body_filename=$ARGV[0];+my@command=@ARGV[1..$#ARGV];++#readdata+my$body_size=-s$body_filename;+$ENV{"CONTENT_LENGTH"}=$body_size;+open(my$body_fh,"<",$body_filename)ordie"Cannot open $body_filename: $!";+my$body_data;+definedread($body_fh,$body_data,$body_size)ordie"Cannot read $body_filename: $!";+close($body_fh);++my$exited=0;+$SIG{"CHLD"}=sub{+$exited=1;+};++#writedata+my$pid=open(my$out,"|-",@command);+{+#disablebufferingat$out+my$old_selected=select;+select$out;+$|=1;+select$old_selected;+}+print$out$body_dataordie"Cannot write data: $!";++sleep60;#isinterruptedbySIGCHLD+if(!$exited){+close($out);+die"Command did not exit after reading whole body";+}