{"thread":{"id":"41856","subject":"[PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi","startedAt":"2016-03-29T10:38:23Z","lastAt":"2017-12-20T04:36:45Z","messageCount":50,"participants":["Florian Manschwetus","Jeff King","Max Kirillov","Eric Sunshine","Junio C Hamano","Dan Jacques","Stefan Beller","Todd Zullinger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"282037","messageId":"F0F5A56A22F20D4CB4A03BB8D6658797E261A3D6@SERVER2011.CS-SOFTWARE.local","threadId":"41856","inReplyTo":null,"subject":"[PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi","fromName":"Florian Manschwetus","fromEmail":"manschwetus@cs-software-gmbh.de","sentAt":"2016-03-29T10:38:23Z","receivedAt":"2016-03-29T10:38:23Z","isPatch":true,"sender":{"key":"manschwetus@cs-software-gmbh.de","avatar":null},"body":"-----Ursprüngliche Nachricht-----\nVon: Chris Packham [mailto:judge.packham@gmail.com] \nGesendet: Dienstag, 29. März 2016 11:28\nAn: Florian Manschwetus\nCc: Konstantin Khomoutov; git@vger.kernel.org\nBetreff: Re: Problem with git-http-backend.exe as iis cgi\n\nHi Florian\n\nOn Tue, Mar 29, 2016 at 7:01 PM, Florian Manschwetus wrote:\n> Hi,\n> I put together a first patch for the issue.\n>\n> Mit freundlichen Grüßen / With kind regards Florian Manschwetus\n>\n> E-Mail: manschwetus@cs-software-gmbh.de\n> Tel.: +49-(0)611-8908534\n>\n> CS Software Concepts and Solutions GmbH Geschäftsführer / Managing \n> director: Dr. Werner Alexi Amtsgericht Wiesbaden HRB 10004 (Commercial \n> registry) Schiersteiner Straße 31\n> D-65187 Wiesbaden\n> Germany\n> Tel.: 0611/8908555\n>\n>\n> -----Ursprüngliche Nachricht-----\n> Von: Konstantin Khomoutov [mailto:kostix+git@007spb.ru]\n> Gesendet: Donnerstag, 10. März 2016 13:55\n> An: Florian Manschwetus\n> Cc: git@vger.kernel.org\n> Betreff: Re: Problem with git-http-backend.exe as iis cgi\n>\n> On Thu, 10 Mar 2016 07:28:50 +0000\n> Florian Manschwetus wrote:\n>\n>> I tried to setup git-http-backend with iis, as iis provides proper \n>> impersonation for cgi under windows, which leads to have the \n>> filesystem access performed with the logon user, therefore the \n>> webserver doesn't need generic access to the files. I stumbled across \n>> a problem, ending up with post requests hanging forever. After some \n>> investigation I managed to get it work by wrapping the http-backend \n>> into a bash script, giving a lot of control about the environmental \n>> things, I was unable to solve within IIS configuration. The \n>> workaround, I use currently, is to use \"/bin/head -c \n>> ${CONTENT_LENGTH}\n>> | ./git-http-backend.exe\", which directly shows the issue. Git\n>> http-backend should check if CONTENT_LENGTH is set to something \n>> reasonable (e.g. >0) and should in this case read only CONTENT_LENGTH \n>> bytes from stdin, instead of reading till EOF what I suspect it is \n>> doing currently.\n>\n> The rfc [1] states in its section 4.2:\n>\n> | A request-body is supplied with the request if the CONTENT_LENGTH is \n> | not NULL. The server MUST make at least that many bytes available \n> | for the script to read. The server MAY signal an end-of-file \n> | condition after CONTENT_LENGTH bytes have been read or it MAY supply \n> | extension data. Therefore, the script MUST NOT attempt to read more \n> | than CONTENT_LENGTH bytes, even if more data is available. However, \n> | it is not obliged to read any of the data.\n>\n> So yes, if Git currently reads until EOF, it's an error.\n> The correct way would be:\n>\n> 1) Check to see if the CONTENT_LENGTH variable is available in the\n> environment. If no, read nothing.\n>\n> 2) Otherwise read as many bytes it specifies, and no more.\n>\n> 1. https://www.ietf.org/rfc/rfc3875\n\nYour 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\n\nMoin,\nI have cloned git and created a more clean patch...\n\nSigned-off-by: Florian Manschwetus \n---\n http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------\n 1 file changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 8870a26..94976df 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)\n */\n static ssize_t read_request(int fd, unsigned char **out)\n {\n-\tsize_t len = 0, alloc = 8192;\n-\tunsigned char *buf = xmalloc(alloc);\n+\tunsigned char *buf = null;\n+\tsize_t len = 0;\n+\t/* get request size */\n+\tsize_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n+\t\t\t\t\t 0);\n+\n+\t/* check request size */\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu);\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = null;\n+\t\treturn 0;\n+\t}\n+\n+\t/* allocate buffer */\n+\tbuf = xmalloc(req_len)\n \n-\tif (max_request_buffer < alloc)\n-\t\tmax_request_buffer = alloc;\n \n \twhile (1) {\n \t\tssize_t cnt;\n \n-\t\tcnt = read_in_full(fd, buf + len, alloc - len);\n+\t\tcnt = read_in_full(fd, buf + len, req_len - len);\n \t\tif (cnt < 0) {\n \t\t\tfree(buf);\n \t\t\treturn -1;\n@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)\n \n \t\t/* partial read from read_in_full means we hit EOF */\n \t\tlen += cnt;\n-\t\tif (len < alloc) {\n+\t\tif (len < req_len) {\n+\t\t\t/* TODO request incomplete?? */\n+\t\t\t/* maybe just remove this block and condition along with the loop, */\n+\t\t\t/* if read_in_full is prooven reliable */\n \t\t\t*out = buf;\n \t\t\treturn len;\n+\t\t} else {\n+\t\t\t/* request complete */\n+\t\t\t*out = buf;\n+\t\t\treturn len;\n+\t\t\t\n \t\t}\n-\n-\t\t/* otherwise, grow and try again (if we can) */\n-\t\tif (alloc == max_request_buffer)\n-\t\t\tdie(\"request was larger than our maximum size (%lu);\"\n-\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n-\t\t\t max_request_buffer);\n-\n-\t\talloc = alloc_nr(alloc);\n-\t\tif (alloc > max_request_buffer)\n-\t\t\talloc = max_request_buffer;\n-\t\tREALLOC_ARRAY(buf, alloc);\n \t}\n }\n \n@@ -701,3 +714,4 @@ int main(int argc, char **argv)\n \tcmd->imp(cmd_arg);\n \treturn 0;\n }\n+\n-- \n2.7.2.windows.1\n\n\nMit freundlichen Grüßen / With kind regards\nFlorian Manschwetus\n\nCS Software Concepts and Solutions GmbH\nGeschäftsführer / Managing director: Dr. Werner Alexi \nAmtsgericht Wiesbaden HRB 10004 (Commercial registry)\nSchiersteiner Straße 31\nD-65187 Wiesbaden\nGermany\n\n\n"},{"id":"282106","messageId":"20160329201349.GB9527@sigill.intra.peff.net","threadId":"41856","inReplyTo":"F0F5A56A22F20D4CB4A03BB8D6658797E261A3D6@SERVER2011.CS-SOFTWARE.local","subject":"Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-29T20:13:49Z","receivedAt":"2016-03-29T20:13:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:\n\n> > | A request-body is supplied with the request if the CONTENT_LENGTH is \n> > | not NULL. The server MUST make at least that many bytes available \n> > | for the script to read. The server MAY signal an end-of-file \n> > | condition after CONTENT_LENGTH bytes have been read or it MAY supply \n> > | extension data. Therefore, the script MUST NOT attempt to read more \n> > | than CONTENT_LENGTH bytes, even if more data is available. However, \n> > | it is not obliged to read any of the data.\n> >\n> > So yes, if Git currently reads until EOF, it's an error.\n> > The correct way would be:\n> >\n> > 1) Check to see if the CONTENT_LENGTH variable is available in the\n> > environment. If no, read nothing.\n> >\n> > 2) Otherwise read as many bytes it specifies, and no more.\n> >\n> > 1. https://www.ietf.org/rfc/rfc3875\n\nI don't think the second part of (1) will work very well if the client\nsends a chunked transfer-encoding (which git will do if the input is large). In\nsuch a case the server would either have to buffer the entire input to\nfind its length, or stream the data to the CGI without setting\n$CONTENT_LENGTH. At least some servers choose the latter (including\nApache).\n\n> diff --git a/http-backend.c b/http-backend.c\n> index 8870a26..94976df 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)\n> */\n> static ssize_t read_request(int fd, unsigned char **out)\n> {\n> -\tsize_t len = 0, alloc = 8192;\n> -\tunsigned char *buf = xmalloc(alloc);\n> +\tunsigned char *buf = null;\n> +\tsize_t len = 0;\n> +\t/* get request size */\n> +\tsize_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n> +\t\t\t\t\t 0);\n> +\n> +\t/* check request size */\n> +\tif (max_request_buffer < req_len) {\n> +\t\tdie(\"request was larger than our maximum size (%lu);\"\n> +\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n> +\t\t\t max_request_buffer);\n> +\t}\n> +\n> +\tif (req_len <= 0) {\n> +\t\t*out = null;\n> +\t\treturn 0;\n> +\t}\n\ngit-am complained that your patch did not apply, but after writing\nsomething similar locally, I found that t5551.25 hangs indefinitely.\nWhich is not surprising. Most tests are doing very limited ref\nnegotiation, so the content that hits read_request() here is small, and\nwe send it in a single write with a content-length header. But t5551.25\nuses a much bigger workload, which causes the client to use a chunked\ntransfer-encoding, and this code to refuse to read anything (and then\nthe protocol stalls, as we are waiting for the client to say something).\n\nSo I think you'd want to take a missing CONTENT_LENGTH as a hint to read\nuntil EOF.\n\nThat also raises another issue: what happens in the paths that don't hit\nread_request()? We may also process input via:\n\n - inflate_request(), if the client gzipped it; for well-formed input,\n I think we'll stop reading when the gzip stream tells us there is no\n more data, but a malformed one would have us reading until EOF,\n regardless of what $CONTENT_LENGTH says.\n\n - for input which we expect to be large (like incoming packfiles for a\n push), buffer_input will be unset, and we will pass the descriptor\n directly to a sub-program like git-index-pack. Again, for\n well-formed input it would read just the packfile, but it may\n actually continue to EOF.\n\nSo I don't think your patch is covering all cases.\n\n-Peff\n"},{"id":"282204","messageId":"F0F5A56A22F20D4CB4A03BB8D6658797E261AF97@SERVER2011.CS-SOFTWARE.local","threadId":"41856","inReplyTo":"20160329201349.GB9527@sigill.intra.peff.net","subject":"AW: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi","fromName":"Florian Manschwetus","fromEmail":"manschwetus@cs-software-gmbh.de","sentAt":"2016-03-30T09:08:56Z","receivedAt":"2016-03-30T09:08:56Z","isPatch":true,"sender":{"key":"manschwetus@cs-software-gmbh.de","avatar":null},"body":"> -----Ursprüngliche Nachricht-----\n> Von: Jeff King [mailto:peff@peff.net]\n> Gesendet: Dienstag, 29. März 2016 22:14\n> An: Florian Manschwetus\n> Cc: Chris Packham; Konstantin Khomoutov; git@vger.kernel.org\n> Betreff: Re: [PATCH] Fix http-backend reading till EOF, ignoring\n> CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-\n> backend.exe as iis cgi\n> \n> On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:\n> \n> > > | A request-body is supplied with the request if the CONTENT_LENGTH\n> > > | is not NULL. The server MUST make at least that many bytes\n> > > | available for the script to read. The server MAY signal an\n> > > | end-of-file condition after CONTENT_LENGTH bytes have been read or\n> > > | it MAY supply extension data. Therefore, the script MUST NOT\n> > > | attempt to read more than CONTENT_LENGTH bytes, even if more data\n> > > | is available. However, it is not obliged to read any of the data.\n> > >\n> > > So yes, if Git currently reads until EOF, it's an error.\n> > > The correct way would be:\n> > >\n> > > 1) Check to see if the CONTENT_LENGTH variable is available in the\n> > > environment. If no, read nothing.\n> > >\n> > > 2) Otherwise read as many bytes it specifies, and no more.\n> > >\n> > > 1. https://www.ietf.org/rfc/rfc3875\n> \n> I don't think the second part of (1) will work very well if the client sends a\n> chunked transfer-encoding (which git will do if the input is large). In such a\n> case the server would either have to buffer the entire input to find its length,\n> or stream the data to the CGI without setting $CONTENT_LENGTH. At least\n> some servers choose the latter (including Apache).\n> \n> > diff --git a/http-backend.c b/http-backend.c index 8870a26..94976df\n> > 100644\n> > --- a/http-backend.c\n> > +++ b/http-backend.c\n> > @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const\n> char *name)\n> > */\n> > static ssize_t read_request(int fd, unsigned char **out) {\n> > -\tsize_t len = 0, alloc = 8192;\n> > -\tunsigned char *buf = xmalloc(alloc);\n> > +\tunsigned char *buf = null;\n> > ...\n> \n> git-am complained that your patch did not apply, but after writing something\n> similar locally, I found that t5551.25 hangs indefinitely.\n> Which is not surprising. Most tests are doing very limited ref negotiation, so\n> the content that hits read_request() here is small, and we send it in a single\n> write with a content-length header. But t5551.25 uses a much bigger\n> workload, which causes the client to use a chunked transfer-encoding, and\n> this code to refuse to read anything (and then the protocol stalls, as we are\n> waiting for the client to say something).\n> \n> So I think you'd want to take a missing CONTENT_LENGTH as a hint to read\n> until EOF.\n> \n> That also raises another issue: what happens in the paths that don't hit\n> read_request()? We may also process input via:\n> \n> - inflate_request(), if the client gzipped it; for well-formed input,\n> I think we'll stop reading when the gzip stream tells us there is no\n> more data, but a malformed one would have us reading until EOF,\n> regardless of what $CONTENT_LENGTH says.\n> \n> - for input which we expect to be large (like incoming packfiles for a\n> push), buffer_input will be unset, and we will pass the descriptor\n> directly to a sub-program like git-index-pack. Again, for\n> well-formed input it would read just the packfile, but it may\n> actually continue to EOF.\n> \n> So I don't think your patch is covering all cases.\n> \n> -Peff\n\nAfter 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.\nTherefore I refactored the code again a bit, to match up the behavior I currently fake by using some bash magic...\n\nFrom ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001\nFrom: manschwetus \nDate: Tue, 29 Mar 2016 12:16:21 +0200\nSubject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring\n CONTENT_LENGTH, violating rfc3875\n\nSigned-off-by: Florian Manschwetus \n---\n http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------\n 1 file changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 8870a26..94976df 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)\n */\n static ssize_t read_request(int fd, unsigned char **out)\n {\n-\tsize_t len = 0, alloc = 8192;\n-\tunsigned char *buf = xmalloc(alloc);\n+\tunsigned char *buf = null;\n+\tsize_t len = 0;\n+\t/* get request size */\n+\tsize_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n+\t\t\t\t\t 0);\n+\n+\t/* check request size */\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu);\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = null;\n+\t\treturn 0;\n+\t}\n+\n+\t/* allocate buffer */\n+\tbuf = xmalloc(req_len)\n \n-\tif (max_request_buffer < alloc)\n-\t\tmax_request_buffer = alloc;\n \n \twhile (1) {\n \t\tssize_t cnt;\n \n-\t\tcnt = read_in_full(fd, buf + len, alloc - len);\n+\t\tcnt = read_in_full(fd, buf + len, req_len - len);\n \t\tif (cnt < 0) {\n \t\t\tfree(buf);\n \t\t\treturn -1;\n@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)\n \n \t\t/* partial read from read_in_full means we hit EOF */\n \t\tlen += cnt;\n-\t\tif (len < alloc) {\n+\t\tif (len < req_len) {\n+\t\t\t/* TODO request incomplete?? */\n+\t\t\t/* maybe just remove this block and condition along with the loop, */\n+\t\t\t/* if read_in_full is prooven reliable */\n \t\t\t*out = buf;\n \t\t\treturn len;\n+\t\t} else {\n+\t\t\t/* request complete */\n+\t\t\t*out = buf;\n+\t\t\treturn len;\n+\t\t\t\n \t\t}\n-\n-\t\t/* otherwise, grow and try again (if we can) */\n-\t\tif (alloc == max_request_buffer)\n-\t\t\tdie(\"request was larger than our maximum size (%lu);\"\n-\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n-\t\t\t max_request_buffer);\n-\n-\t\talloc = alloc_nr(alloc);\n-\t\tif (alloc > max_request_buffer)\n-\t\t\talloc = max_request_buffer;\n-\t\tREALLOC_ARRAY(buf, alloc);\n \t}\n }\n \n-- \n2.7.2.windows.1\n\n\nFrom 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001\nFrom: Florian Manschwetus \nDate: Wed, 30 Mar 2016 10:54:21 +0200\nSubject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved\n new variant to read_request_fix_len(...) and introduced read_request(...) as\n wrapper, which decides based on value retrieved from CONTENT_LENGTH which\n variant to use\n\nSigned-off-by: Florian Manschwetus \n---\n http-backend.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 94976df..3aa0446 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -275,13 +275,52 @@ static struct rpc_service *select_service(const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n+{\n+ size_t len = 0, alloc = 8192;\n+ unsigned char *buf = xmalloc(alloc);\n+\n+ if (max_request_buffer < alloc)\n+ max_request_buffer = alloc;\n+\n+ while (1) {\n+ ssize_t cnt;\n+\n+ cnt = read_in_full(fd, buf + len, alloc - len);\n+ if (cnt < 0) {\n+ free(buf);\n+ return -1;\n+ }\n+\n+ /* partial read from read_in_full means we hit EOF */\n+ len += cnt;\n+ if (len < alloc) {\n+ *out = buf;\n+ return len;\n+ }\n+\n+ /* otherwise, grow and try again (if we can) */\n+ if (alloc == max_request_buffer)\n+ die(\"request was larger than our maximum size (%lu);\"\n+ \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+ max_request_buffer);\n+\n+ alloc = alloc_nr(alloc);\n+ if (alloc > max_request_buffer)\n+ alloc = max_request_buffer;\n+ REALLOC_ARRAY(buf, alloc);\n+ }\n+}\n+\n+/*\n+ * replacement for original read_request, now renamed to read_request_eof,\n+ * honoring given content_length (req_len),\n+ * provided by new wrapper function read_request\n+ */\n+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n {\n \tunsigned char *buf = null;\n \tsize_t len = 0;\n-\t/* get request size */\n-\tsize_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n-\t\t\t\t\t 0);\n \n \t/* check request size */\n \tif (max_request_buffer < req_len) {\n@@ -325,6 +364,26 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+/**\n+ * wrapper function, whcih determines based on CONTENT_LENGTH value,\n+ * to\n+ * - use old behaviour of read_request, to read until EOF\n+ * => read_request_eof(...)\n+ * - just read CONTENT_LENGTH-bytes, when provided\n+ * => read_request_fix_len(...)\n+ */\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+ /* get request size */\n+ size_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n+ -1);\n+ if (req_len < 0){\n+ read_request_eof(fd, out);\n+ } else {\n+ read_request_fix_len(fd, req_len, out);\n+ }\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.7.2.windows.1\n\n\nMit freundlichen Grüßen / With kind regards\nFlorian Manschwetus\n \nCS Software Concepts and Solutions GmbH\nGeschäftsführer / Managing director: Dr. Werner Alexi \nAmtsgericht Wiesbaden HRB 10004 (Commercial registry)\nSchiersteiner Straße 31\nD-65187 Wiesbaden\nGermany\n\n\n\n"},{"id":"282512","messageId":"20160401235532.GA27941@sigill.intra.peff.net","threadId":"41856","inReplyTo":"F0F5A56A22F20D4CB4A03BB8D6658797E261AF97@SERVER2011.CS-SOFTWARE.local","subject":"Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-01T23:55:32Z","receivedAt":"2016-04-01T23:55:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2016 at 09:08:56AM +0000, Florian Manschwetus wrote:\n\n> After additional analysis it turned out, that in the case you\n> mentioned, at least IIS, sets CONTENT_LENGTH to -1 resulting in the\n> current behavior of git-http-backend being sufficient in this\n> situation.\n> Therefore I refactored the code again a bit, to match up the behavior\n> I currently fake by using some bash magic...\n\nOK, so I'd agree it makes sense to catch \"-1\", and read to EOF in that\ncase (or if CONTENT_LENGTH is NULL).\n\n> From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001\n> From: manschwetus \n> Date: Tue, 29 Mar 2016 12:16:21 +0200\n> Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring\n> CONTENT_LENGTH, violating rfc3875\n\nPlease send one patch per email, and these header bits should be the\nheader of your email.\n\nThough we also generally revise and re-send patches, rather than\npresenting one patch that has problems and then tacking fixes on top.\nI'll ignore the problems in this patch 1, as it looks like it's just the\noriginal one repeated.\n\n> From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001\n> From: Florian Manschwetus \n> Date: Wed, 30 Mar 2016 10:54:21 +0200\n> Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved\n> new variant to read_request_fix_len(...) and introduced read_request(...) as\n> wrapper, which decides based on value retrieved from CONTENT_LENGTH which\n> variant to use\n\nPlease use a short subject for your commit message, followed by a blank\nline and then a more explanatory body. Also, don't just describe _what_\nis happening (we can see that from the diff), but _why_. You can find\nmore similar tips in SubmittingPatches.\n\n> +/**\n> + * wrapper function, whcih determines based on CONTENT_LENGTH value,\n> + * to\n> + * - use old behaviour of read_request, to read until EOF\n> + * => read_request_eof(...)\n> + * - just read CONTENT_LENGTH-bytes, when provided\n> + * => read_request_fix_len(...)\n> + */\n> +static ssize_t read_request(int fd, unsigned char **out)\n> +{\n> + /* get request size */\n> + size_t req_len = git_env_ulong(\"CONTENT_LENGTH\",\n> + -1);\n> + if (req_len < 0){\n> + read_request_eof(fd, out);\n> + } else {\n> + read_request_fix_len(fd, req_len, out);\n> + }\n> +}\n\nI don't think \"if (req_len < 0)\" can ever trigger, because size_t is an\nunsigned type (and I do not recall what kind of integer overflow\nvalidation we do in git_env_ulong, but I suspect it may complain about\n\"-1\"). You may have to parse the variable manually rather than using\ngit_env_ulong (i.e., pick out the NULL and \"-1\" cases, and then feed the\nrest to git_parse_ulong()).\n\nAlso, a few style nits. We usually omit braces for one-line\nconditionals, and please make sure there is whitespace between the\nclosing parenthesis and the opening brace. There's some discussing in\nCodingGuidelines.\n\n-Peff\n"},{"id":"333399","messageId":"20171123234511.574-1-max@max630.net","threadId":"41856","inReplyTo":"20160401235532.GA27941@sigill.intra.peff.net","subject":"[PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-23T23:45:11Z","receivedAt":"2017-11-23T23:53:54Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"http-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. This causes hang under IIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the varibale is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nSigned-off-by: Florian Manschwetus \nAuthored-by: Florian Manschwetus \nFixed-by: Max Kirillov \nSigned-off-by: Max Kirillov \n---\nHi\n\nI came across this issue, and I think is should be good to restore the patch.\nIt is basically same but I squashed them, fixed the thing you mentioned and\nalso some trivial build failures (null -> NULL and missing return from the wrapper).\nI hope I marked it correctly in the trailers.\n config.c | 8 +++++++\n config.h | 1 +\n http-backend.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 80 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 231f9a750b..925bb65dfa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)\n \treturn val;\n }\n \n+ssize_t git_env_ssize_t(const char *k, ssize_t val)\n+{\n+\tconst char *v = getenv(k);\n+\tif (v && !git_parse_ssize_t(v, &val))\n+\t\tdie(\"failed to parse %s\", k);\n+\treturn val;\n+}\n+\n int git_config_system(void)\n {\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\ndiff --git a/config.h b/config.h\nindex 0352da117b..947695c304 100644\n--- a/config.h\n+++ b/config.h\n@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c\n extern const char *git_etc_gitconfig(void);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\n+extern ssize_t git_env_ssize_t(const char *, ssize_t);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n #if defined(__GNUC__)\ndiff --git a/http-backend.c b/http-backend.c\nindex 519025d2c3..317b99b87c 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+/*\n+ * replacement for original read_request, now renamed to read_request_eof,\n+ * honoring given content_length (req_len),\n+ * provided by new wrapper function read_request\n+ */\n+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tsize_t len = 0;\n+\n+\t/* check request size */\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu);\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\t/* allocate buffer */\n+\tbuf = xmalloc(req_len);\n+\n+\n+\twhile (1) {\n+\t\tssize_t cnt;\n+\n+\t\tcnt = read_in_full(fd, buf + len, req_len - len);\n+\t\tif (cnt < 0) {\n+\t\t\tfree(buf);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\t/* partial read from read_in_full means we hit EOF */\n+\t\tlen += cnt;\n+\t\tif (len < req_len) {\n+\t\t\t/* TODO request incomplete?? */\n+\t\t\t/* maybe just remove this block and condition along with the loop, */\n+\t\t\t/* if read_in_full is prooven reliable */\n+\t\t\t*out = buf;\n+\t\t\treturn len;\n+\t\t} else {\n+\t\t\t/* request complete */\n+\t\t\t*out = buf;\n+\t\t\treturn len;\n+\t\t\t\n+\t\t}\n+\t}\n+}\n+\n+/**\n+ * wrapper function, whcih determines based on CONTENT_LENGTH value,\n+ * to\n+ * - use old behaviour of read_request, to read until EOF\n+ * => read_request_eof(...)\n+ * - just read CONTENT_LENGTH-bytes, when provided\n+ * => read_request_fix_len(...)\n+ */\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\t/* get request size */\n+\tssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fix_len(fd, req_len, out);\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333403","messageId":"CAPig+cQEaqaOTcC=5pZZmZNs_QQQ0vBRbzczyM3ZXXi+ZHW4XA@mail.gmail.com","threadId":"41856","inReplyTo":"20171123234511.574-1-max@max630.net","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-24T01:30:39Z","receivedAt":"2017-11-24T01:30:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 23, 2017 at 6:45 PM, Max Kirillov wrote:\n> [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875\n\nThe \"RFC\" seems to be missing from the subject line of this unpolished patch.\n\n> http-backend reads whole input until EOF. However, the RFC 3875 specifies\n> that a script must read only as many bytes as specified by CONTENT_LENGTH\n> environment variable. This causes hang under IIS/Windows, for example.\n\nBy \"_this_ causes a hang\", I presume you mean \"not respecting\nCONTENT_LENGTH causes a hang\"? Perhaps that could be spelled out\nexplicitly.\n\n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the varibale is not defined, keep older behavior\n\ns/varibale/variable/\n\n> of reading until EOF because it is used to support chunked transfer-encoding.\n>\n> Signed-off-by: Florian Manschwetus \n> Authored-by: Florian Manschwetus \n> Fixed-by: Max Kirillov \n> Signed-off-by: Max Kirillov \n> ---\n> diff --git a/http-backend.c b/http-backend.c\n> @@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)\n> +/*\n> + * replacement for original read_request, now renamed to read_request_eof,\n> + * honoring given content_length (req_len),\n> + * provided by new wrapper function read_request\n> + */\n\nThis comment has value only to someone who knew what the code was like\nbefore this change, and it merely repeats what is already implied by\nthe commit message, rather than providing any valuable information\nabout this new function itself. Therefore, it should be dropped.\n\n> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n\nWrong data type: s/size_t req_len/ssize_t req_len/\n\nAlso: s/fix/fixed/\n\n> +{\n> + unsigned char *buf = NULL;\n> + size_t len = 0;\n> +\n> + /* check request size */\n\nComment merely repeats what code says, thus has no value. Please drop.\n\n> + if (max_request_buffer < req_len) {\n> + die(\"request was larger than our maximum size (%lu);\"\n> + \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n> + max_request_buffer);\n\nThis error message neglects to say what the request size was. Such\ninformation would be useful given that it suggests bumping\nGIT_HTTP_MAX_REQUEST_BUFFER to a larger value.\n\n> + }\n> +\n> + if (req_len <= 0) {\n> + *out = NULL;\n> + return 0;\n> + }\n> +\n> + /* allocate buffer */\n\nDrop valueless comment.\n\n> + buf = xmalloc(req_len);\n> +\n> +\n\nStyle: Too many blank lines.\n\n> + while (1) {\n> + ssize_t cnt;\n> +\n> + cnt = read_in_full(fd, buf + len, req_len - len);\n> + if (cnt < 0) {\n> + free(buf);\n> + return -1;\n> + }\n> +\n> + /* partial read from read_in_full means we hit EOF */\n> + len += cnt;\n> + if (len < req_len) {\n> + /* TODO request incomplete?? */\n> + /* maybe just remove this block and condition along with the loop, */\n> + /* if read_in_full is prooven reliable */\n\ns/prooven/proven/\n\n> + *out = buf;\n> + return len;\n> + } else {\n> + /* request complete */\n> + *out = buf;\n> + return len;\n> +\n> + }\n> + }\n\nWhat is the purpose of the while(1) loop? Every code path inside the\nloop returns, so it will never execute more than once. Likewise, why\nis 'len' needed?\n\nRather than writing an entirely new \"read\" function, how about just\nmodifying the existing read_request() to optionally limit the read to\na specified number of bytes?\n\n> +}\n> +\n> +/**\n> + * wrapper function, whcih determines based on CONTENT_LENGTH value,\n\ns/whcih/which/\n\nAlso, the placement of commas needs some attention.\n\n> + * to\n> + * - use old behaviour of read_request, to read until EOF\n> + * => read_request_eof(...)\n> + * - just read CONTENT_LENGTH-bytes, when provided\n> + * => read_request_fix_len(...)\n> + */\n\nWhen talking about \"old behavior\", this comment is repeating\ninformation more suitable to the commit message (and effectively\nalready covered there); information which only has value to someone\nwho knew what the old code/behavior was like. The rest of this comment\nis merely repeating what the code itself already says, thus adds no\nvalue, so should be dropped.\n\n> +static ssize_t read_request(int fd, unsigned char **out)\n> +{\n> + /* get request size */\n\nDrop valueless comment.\n\n> + ssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n> + if (req_len < 0)\n> + return read_request_eof(fd, out);\n> + else\n> + return read_request_fix_len(fd, req_len, out);\n> +}\n"},{"id":"333412","messageId":"xmqqvai0nrv9.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171123234511.574-1-max@max630.net","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-24T05:54:50Z","receivedAt":"2017-11-24T05:54:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> http-backend reads whole input until EOF. However, the RFC 3875 specifies\n> that a script must read only as many bytes as specified by CONTENT_LENGTH\n> environment variable. This causes hang under IIS/Windows, for example.\n>\n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the varibale is not defined, keep older behavior\n> of reading until EOF because it is used to support chunked transfer-encoding.\n>\n> Signed-off-by: Florian Manschwetus \n> Authored-by: Florian Manschwetus \n> Fixed-by: Max Kirillov \n> Signed-off-by: Max Kirillov \n> ---\n> ...\n> I hope I marked it correctly in the trailers.\n\nIt is probably more conventional to do it like so:\n\n From: Florian Manschwetus \n Date: \n\n http-backend reads whole input until EOF. However, the RFC 3875...\n ... chunked transfer-encoding.\n\n Signed-off-by: Florian Manschwetus \n [mk: fixed trivial build failures and stuff]\n Signed-off-by: Max Kirillov \n ---\n\n> \n> +/*\n> + * replacement for original read_request, now renamed to read_request_eof,\n> + * honoring given content_length (req_len),\n> + * provided by new wrapper function read_request\n> + */\n\nI agree with Eric's suggestion. In-code comment is read by those\nwho have the current code, without knowing/caring what it used to\nbe. \"It used to do this, but replace it with this new thing\nbecause...\" is a valuable thing to record in the log message, but\nnot here.\n"},{"id":"333424","messageId":"F0F5A56A22F20D4CB4A03BB8D66587970253F84634@SERVER2011.CS-SOFTWARE.local","threadId":"41856","inReplyTo":"xmqqvai0nrv9.fsf@gitster.mtv.corp.google.com","subject":"AW: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Florian Manschwetus","fromEmail":"manschwetus@cs-software-gmbh.de","sentAt":"2017-11-24T08:30:49Z","receivedAt":"2017-11-24T08:37:12Z","isPatch":true,"sender":{"key":"manschwetus@cs-software-gmbh.de","avatar":null},"body":"Hi All,\nthx Max for jumping in, I wasn't able to complete this due to serious lack of time.\nLater I forgot it. Great to see that this finally made it.\n\nMit freundlichen Grüßen / With kind regards\nFlorian Manschwetus\n\nE-Mail: manschwetus@cs-software-gmbh.de\nTel.: +49-(0)611-8908534\n \nCS Software Concepts and Solutions GmbH\nGeschäftsführer / Managing director: Dr. Werner Alexi \nAmtsgericht Wiesbaden HRB 10004 (Commercial registry)\nSchiersteiner Straße 31\nD-65187 Wiesbaden\nGermany\nTel.: 0611/8908555\n\n> -----Ursprüngliche Nachricht-----\n> Von: Junio C Hamano [mailto:gitster@pobox.com]\n> Gesendet: Freitag, 24. November 2017 06:55\n> An: Max Kirillov\n> Cc: Jeff King; Florian Manschwetus; Chris Packham; Konstantin Khomoutov;\n> git@vger.kernel.org\n> Betreff: Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified\n> by rfc3875\n> \n> Max Kirillov writes:\n> \n> > http-backend reads whole input until EOF. However, the RFC 3875\n> > specifies that a script must read only as many bytes as specified by\n> > CONTENT_LENGTH environment variable. This causes hang under\n> IIS/Windows, for example.\n> >\n> > Make http-backend read only CONTENT_LENGTH bytes, if it's defined,\n> > rather than the whole input until EOF. If the varibale is not defined,\n> > keep older behavior of reading until EOF because it is used to support\n> chunked transfer-encoding.\n> >\n> > Signed-off-by: Florian Manschwetus gmbh.de>\n> > Authored-by: Florian Manschwetus gmbh.de>\n> > Fixed-by: Max Kirillov \n> > Signed-off-by: Max Kirillov \n> > ---\n> > ...\n> > I hope I marked it correctly in the trailers.\n> \n> It is probably more conventional to do it like so:\n> \n> From: Florian Manschwetus \n> Date: \n> \n> http-backend reads whole input until EOF. However, the RFC 3875...\n> ... chunked transfer-encoding.\n> \n> Signed-off-by: Florian Manschwetus gmbh.de>\n> [mk: fixed trivial build failures and stuff]\n> Signed-off-by: Max Kirillov \n> ---\n> \n> >\n> > +/*\n> > + * replacement for original read_request, now renamed to\n> > +read_request_eof,\n> > + * honoring given content_length (req_len),\n> > + * provided by new wrapper function read_request */\n> \n> I agree with Eric's suggestion. In-code comment is read by those who have\n> the current code, without knowing/caring what it used to be. \"It used to do\n> this, but replace it with this new thing because...\" is a valuable thing to record\n> in the log message, but not here.\n"},{"id":"333489","messageId":"20171125214721.GA26158@jessie.local","threadId":"41856","inReplyTo":"CAPig+cQEaqaOTcC=5pZZmZNs_QQQ0vBRbzczyM3ZXXi+ZHW4XA@mail.gmail.com","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-25T21:47:21Z","receivedAt":"2017-11-25T21:47:40Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Thanks for the review. I saw only reaction of the Jeff in\nthe original thread and though that it is ok otherwise. I'm\nfixing the things you mentioned.\n\nOn Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:\n>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n> \n> Wrong data type: s/size_t req_len/ssize_t req_len/\n\nPassing negative value to the function makes no sense. I\ncould add explicit type cast to make it clear. It should be\nsafe as site_t's range is bigger, and overflown\nCONTENT_LENGTH results in die() at parsing (I have a test\nwhich verifies it)\n\n> Rather than writing an entirely new \"read\" function, how about just\n> modifying the existing read_request() to optionally limit the read to\n> a specified number of bytes?\n\nI'll check it a bit separately.\n\n-- \nMax\n"},{"id":"333497","messageId":"CAPig+cRRHepuNNva_cq2YPEDSBCO25y1mihuC52RntpJ+a+YMg@mail.gmail.com","threadId":"41856","inReplyTo":"20171125214721.GA26158@jessie.local","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-26T00:38:33Z","receivedAt":"2017-11-26T00:38:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov wrote:\n> Thanks for the review. I saw only reaction of the Jeff in\n> the original thread and though that it is ok otherwise. I'm\n> fixing the things you mentioned.\n\nThe commentary (in which you talked about restoring the patch and\nsquashing) seemed to imply that this had been posted somewhere before,\nbut it wasn't marked as \"v2\" (or whatever attempt) and lacked a URL\npointing at the previous attempt, so it was difficult to judge.\n\n> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:\n>>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n>>\n>> Wrong data type: s/size_t req_len/ssize_t req_len/\n>\n> Passing negative value to the function makes no sense. I\n> could add explicit type cast to make it clear. It should be\n> safe as site_t's range is bigger, and overflown\n> CONTENT_LENGTH results in die() at parsing (I have a test\n> which verifies it)\n\nA concern with requesting size_t bytes is that, if it does read all\nbytes, that value can't necessarily be represented by the ssize_t\nreturned from the function. Where would the cast be placed that you\nsuggest? How do other git functions deal with this sort of situation?\n"},{"id":"333498","messageId":"20171126004300.GB26158@jessie.local","threadId":"41856","inReplyTo":"CAPig+cRRHepuNNva_cq2YPEDSBCO25y1mihuC52RntpJ+a+YMg@mail.gmail.com","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T00:43:00Z","receivedAt":"2017-11-26T00:50:29Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sat, Nov 25, 2017 at 07:38:33PM -0500, Eric Sunshine wrote:\n> On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov wrote:\n>> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:\n>>>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)\n>>>\n>>> Wrong data type: s/size_t req_len/ssize_t req_len/\n>>\n>> Passing negative value to the function makes no sense. I\n>> could add explicit type cast to make it clear. It should be\n>> safe as site_t's range is bigger, and overflown\n>> CONTENT_LENGTH results in die() at parsing (I have a test\n>> which verifies it)\n> \n> A concern with requesting size_t bytes is that, if it does read all\n> bytes, that value can't necessarily be represented by the ssize_t\n> returned from the function. Where would the cast be placed that you\n> suggest? How do other git functions deal with this sort of situation?\n\nRight... ok, let it be ssize_t\n"},{"id":"333501","messageId":"20171126014719.23728-1-max@max630.net","threadId":"41856","inReplyTo":"20171123234511.574-1-max@max630.net","subject":"[PATCH v4 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:47:17Z","receivedAt":"2017-11-26T01:55:01Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"* polished style\n* add tests\n* marked the authorship more correctly\n\nMax Kirillov (2):\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases\n\n Makefile | 1 +\n config.c | 8 ++++++++\n config.h | 1 +\n http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-\n t/helper/test-print-values.c | 10 ++++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++\n 6 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-print-values.c\n\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333502","messageId":"20171126015448.24111-1-max@max630.net","threadId":"41856","inReplyTo":"20171123234511.574-1-max@max630.net","subject":"[PATCH v5 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:54:46Z","receivedAt":"2017-11-26T01:55:06Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"I seem to forgot to put the authorship lines, and also did something with\nreplies, so sending another sequence.\n\nv5:\n\n* marked the authorship more correctly\n\nv4:\n\n* polished style\n* add tests\n\nMax Kirillov (2):\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases\n\n Makefile | 1 +\n config.c | 8 ++++++++\n config.h | 1 +\n http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-\n t/helper/test-print-values.c | 10 ++++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++\n 6 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-print-values.c\n\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333503","messageId":"20171126014719.23728-3-max@max630.net","threadId":"41856","inReplyTo":"20171126014719.23728-2-max@max630.net","subject":"[PATCH v4 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:47:19Z","receivedAt":"2017-11-26T01:55:07Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Add tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data.\n (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero\n and fail. It does not seem to cause any performance issues with the default\n value of GIT_HTTP_MAX_REQUEST_BUFFER.)\n* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.\n\nSigned-off-by: Max Kirillov \n---\n Makefile | 1 +\n t/helper/test-print-values.c | 10 ++++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+)\n create mode 100644 t/helper/test-print-values.c\n\ndiff --git a/Makefile b/Makefile\nindex 461c845d33..616408b32c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-online-cpus\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-print-values\n TEST_PROGRAMS_NEED_X += test-prio-queue\n TEST_PROGRAMS_NEED_X += test-read-cache\n TEST_PROGRAMS_NEED_X += test-ref-store\ndiff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c\nnew file mode 100644\nindex 0000000000..8f7e5af319\n--- /dev/null\n+++ b/t/helper/test-print-values.c\n@@ -0,0 +1,10 @@\n+#include \n+#include \n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tif (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n+\t\tprintf(\"%zu\", (ssize_t)(-20));\n+\n+\treturn 0;\n+}\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9fafcf1945..f452090216 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n \texpect_aliased 1 //domain/data.txt\n '\n \n+# overrides existing definition for further cases\n+run_backend() {\n+\tCONTENT_LENGTH=\"${#2}\" && export CONTENT_LENGTH &&\n+\t( echo \"$2\" && cat /dev/zero ) |\n+\tQUERY_STRING=\"${1#*[?]}\" \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n+\tgit http-backend >act.out 2>act.err\n+}\n+\n+test_expect_success 'CONTENT_LENGTH set and infinite input' '\n+\tconfig http.uploadpack true &&\n+\tGET info/refs?service=git-upload-pack \"200 OK\"\t&&\n+\t! grep \"fatal:.*\" act.err &&\n+\tPOST git-upload-pack 0000 \"200 OK\" &&\n+\t! grep \"fatal:.*\" act.err\n+'\n+\n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=`\"$GIT_BUILD_DIR/t/helper/test-print-values\" \"(size_t)(-20)\"` &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n test_done\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333504","messageId":"20171126015448.24111-3-max@max630.net","threadId":"41856","inReplyTo":"20171126015448.24111-1-max@max630.net","subject":"[PATCH v5 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:54:48Z","receivedAt":"2017-11-26T01:55:10Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Add tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data.\n (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero\n and fail. It does not seem to cause any performance issues with the default\n value of GIT_HTTP_MAX_REQUEST_BUFFER.)\n* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.\n\nSigned-off-by: Max Kirillov \n---\n Makefile | 1 +\n t/helper/test-print-values.c | 10 ++++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+)\n create mode 100644 t/helper/test-print-values.c\n\ndiff --git a/Makefile b/Makefile\nindex 461c845d33..616408b32c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-online-cpus\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-print-values\n TEST_PROGRAMS_NEED_X += test-prio-queue\n TEST_PROGRAMS_NEED_X += test-read-cache\n TEST_PROGRAMS_NEED_X += test-ref-store\ndiff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c\nnew file mode 100644\nindex 0000000000..8f7e5af319\n--- /dev/null\n+++ b/t/helper/test-print-values.c\n@@ -0,0 +1,10 @@\n+#include \n+#include \n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tif (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n+\t\tprintf(\"%zu\", (ssize_t)(-20));\n+\n+\treturn 0;\n+}\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9fafcf1945..f452090216 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n \texpect_aliased 1 //domain/data.txt\n '\n \n+# overrides existing definition for further cases\n+run_backend() {\n+\tCONTENT_LENGTH=\"${#2}\" && export CONTENT_LENGTH &&\n+\t( echo \"$2\" && cat /dev/zero ) |\n+\tQUERY_STRING=\"${1#*[?]}\" \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n+\tgit http-backend >act.out 2>act.err\n+}\n+\n+test_expect_success 'CONTENT_LENGTH set and infinite input' '\n+\tconfig http.uploadpack true &&\n+\tGET info/refs?service=git-upload-pack \"200 OK\"\t&&\n+\t! grep \"fatal:.*\" act.err &&\n+\tPOST git-upload-pack 0000 \"200 OK\" &&\n+\t! grep \"fatal:.*\" act.err\n+'\n+\n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=`\"$GIT_BUILD_DIR/t/helper/test-print-values\" \"(size_t)(-20)\"` &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n test_done\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333505","messageId":"20171126015448.24111-2-max@max630.net","threadId":"41856","inReplyTo":"20171126015448.24111-1-max@max630.net","subject":"[PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:54:47Z","receivedAt":"2017-11-26T01:55:13Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Author: Florian Manschwetus \nDate: Wed, 30 Mar 2016 09:08:56 +0000\n\nhttp-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\n\nSigned-off-by: Max Kirillov \n---\n config.c | 8 ++++++++\n config.h | 1 +\n http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 231f9a750b..925bb65dfa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)\n \treturn val;\n }\n \n+ssize_t git_env_ssize_t(const char *k, ssize_t val)\n+{\n+\tconst char *v = getenv(k);\n+\tif (v && !git_parse_ssize_t(v, &val))\n+\t\tdie(\"failed to parse %s\", k);\n+\treturn val;\n+}\n+\n int git_config_system(void)\n {\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\ndiff --git a/config.h b/config.h\nindex 0352da117b..947695c304 100644\n--- a/config.h\n+++ b/config.h\n@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c\n extern const char *git_etc_gitconfig(void);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\n+extern ssize_t git_env_ssize_t(const char *, ssize_t);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n #if defined(__GNUC__)\ndiff --git a/http-backend.c b/http-backend.c\nindex 519025d2c3..b4ba83b624 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): %lu;\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer,\n+\t\t\t req_len);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t} else {\n+\t\t*out = buf;\n+\t\treturn cnt;\n+\t}\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333506","messageId":"20171126014719.23728-2-max@max630.net","threadId":"41856","inReplyTo":"20171126014719.23728-1-max@max630.net","subject":"[PATCH v4 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:47:18Z","receivedAt":"2017-11-26T01:55:16Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"http-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\n\nSigned-off-by: Max Kirillov \n---\n config.c | 8 ++++++++\n config.h | 1 +\n http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 231f9a750b..925bb65dfa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)\n \treturn val;\n }\n \n+ssize_t git_env_ssize_t(const char *k, ssize_t val)\n+{\n+\tconst char *v = getenv(k);\n+\tif (v && !git_parse_ssize_t(v, &val))\n+\t\tdie(\"failed to parse %s\", k);\n+\treturn val;\n+}\n+\n int git_config_system(void)\n {\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\ndiff --git a/config.h b/config.h\nindex 0352da117b..947695c304 100644\n--- a/config.h\n+++ b/config.h\n@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c\n extern const char *git_etc_gitconfig(void);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\n+extern ssize_t git_env_ssize_t(const char *, ssize_t);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n #if defined(__GNUC__)\ndiff --git a/http-backend.c b/http-backend.c\nindex 519025d2c3..b4ba83b624 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): %lu;\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer,\n+\t\t\t req_len);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t} else {\n+\t\t*out = buf;\n+\t\treturn cnt;\n+\t}\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333507","messageId":"20171126015015.GC26158@jessie.local","threadId":"41856","inReplyTo":"xmqqvai0nrv9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T01:50:15Z","receivedAt":"2017-11-26T01:57:43Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Nov 24, 2017 at 02:54:50PM +0900, Junio C Hamano wrote:\n> Max Kirillov writes:\n>> I hope I marked it correctly in the trailers.\n> \n> It is probably more conventional to do it like so:\n> \n> From: Florian Manschwetus \n> Date: \n> \n> Signed-off-by: Florian Manschwetus \n> [mk: fixed trivial build failures and stuff]\n> Signed-off-by: Max Kirillov \n\nThanks. Have done so\n"},{"id":"333516","messageId":"xmqqo9npitx7.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171126015448.24111-2-max@max630.net","subject":"Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-26T03:46:12Z","receivedAt":"2017-11-26T03:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> Author: Florian Manschwetus \n\nThis should read \"From: ...\";\n\n> Date: Wed, 30 Mar 2016 09:08:56 +0000\n>\n> http-backend reads whole input until EOF. However, the RFC 3875 specifies\n> that a script must read only as many bytes as specified by CONTENT_LENGTH\n> environment variable. Web server may exercise the specification by not closing\n> the script's standard input after writing content. In that case http-backend\n> would hang waiting for the input. The issue is known to happen with\n> IIS/Windows, for example.\n>\n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the variable is not defined, keep older behavior\n> of reading until EOF because it is used to support chunked transfer-encoding.\n>\n> Signed-off-by: Florian Manschwetus \n> [mk: fixed trivial build failures and polished style issues]\n>\n> Signed-off-by: Max Kirillov \n\nThere shouldn't be a blank line before your sign-off.\n\nThe above are only for future reference; I can fix them up before\nqueuing if there isn't any other issues in this patch.\n\n\n> +ssize_t git_env_ssize_t(const char *k, ssize_t val)\n> +{\n> +\tconst char *v = getenv(k);\n> +\tif (v && !git_parse_ssize_t(v, &val))\n> +\t\tdie(\"failed to parse %s\", k);\n> +\treturn val;\n> +}\n> +\n\nIf this were passing \"v\" that is a string the caller obtains from\nany source (and the callee does not care about where it came from),\nthat would be a different story, but as a public interface that is\npart of the config.c layer, \"k\" that has to be the name of the\nenvironment variable sticks out like a sore thumb.\n\nIf we were to add one more public function to the interface, I\nsuspect that exposing the existing git_parse_ssize_t() and have the\ncaller implement the above function for its use would be a much\nbetter way to go.\n"},{"id":"333522","messageId":"20171126081329.GD26158@jessie.local","threadId":"41856","inReplyTo":"xmqqo9npitx7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T08:13:30Z","receivedAt":"2017-11-26T08:13:39Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Nov 26, 2017 at 12:46:12PM +0900, Junio C Hamano wrote:\n> Max Kirillov writes:\n> > +ssize_t git_env_ssize_t(const char *k, ssize_t val)\n> > +{\n> > +\tconst char *v = getenv(k);\n> > +\tif (v && !git_parse_ssize_t(v, &val))\n> > +\t\tdie(\"failed to parse %s\", k);\n> > +\treturn val;\n> > +}\n> > +\n> \n> If this were passing \"v\" that is a string the caller obtains from\n> any source (and the callee does not care about where it came from),\n> that would be a different story, but as a public interface that is\n> part of the config.c layer, \"k\" that has to be the name of the\n> environment variable sticks out like a sore thumb.\n> \n> If we were to add one more public function to the interface, I\n> suspect that exposing the existing git_parse_ssize_t() and have the\n> caller implement the above function for its use would be a much\n> better way to go.\n\nI'm afraid I did not get the reasonsing and not fully the\ndesired change. Is this http-backend code change (compared\nto the last patch) what you mean?\n\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n \t}\n }\n \n+static ssize_t env_content_length()\n+{\n+\tconst char *str = getenv(\"CONTENT_LENGTH\");\n+\tssize_t val = -1;\n+\tif (str && !git_parse_ssize_t(str, &val))\n+\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\treturn val;\n+}\n+\n static ssize_t read_request(int fd, unsigned char **out)\n {\n-\tssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n+\tssize_t req_len = env_content_length();\n \tif (req_len < 0)\n \t\treturn read_request_eof(fd, out);\n \telse\n\n"},{"id":"333525","messageId":"xmqq3751idmp.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171126081329.GD26158@jessie.local","subject":"Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-26T09:38:06Z","receivedAt":"2017-11-26T09:38:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> I'm afraid I did not get the reasonsing and not fully the\n> desired change. Is this http-backend code change (compared\n> to the last patch) what you mean?\n\nExactly. \n\nThe fact that the parsed string happens to come from CONTENT_LENGTH\nenvironment variable is http's business, not parsers for various\ntypes of values that come in the form of strings.\n\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n> \t}\n> }\n> \n> +static ssize_t env_content_length()\n\nWe need s/length()/length(void)/; though.\n\n> +{\n> +\tconst char *str = getenv(\"CONTENT_LENGTH\");\n> +\tssize_t val = -1;\n> +\tif (str && !git_parse_ssize_t(str, &val))\n> +\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n> +\treturn val;\n> +}\n> +\n> static ssize_t read_request(int fd, unsigned char **out)\n> {\n> -\tssize_t req_len = git_env_ssize_t(\"CONTENT_LENGTH\", -1);\n> +\tssize_t req_len = env_content_length();\n> \tif (req_len < 0)\n> \t\treturn read_request_eof(fd, out);\n> \telse\n"},{"id":"333535","messageId":"20171126193813.12531-1-max@max630.net","threadId":"41856","inReplyTo":"20171126015448.24111-1-max@max630.net","subject":"[PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T19:38:11Z","receivedAt":"2017-11-26T19:38:31Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"v6:\n\nDo not implement generic git_env_ssize_t(), instead export git_parse_ssize_t() from config.c\nand hardcode the rest.\n\nFlorian Manschwetus (1):\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n\nMax Kirillov (1):\n t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases\n\n Makefile | 1 +\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++-\n t/helper/test-print-values.c | 10 ++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++\n 6 files changed, 92 insertions(+), 2 deletions(-)\n create mode 100644 t/helper/test-print-values.c\n\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333536","messageId":"20171126193813.12531-2-max@max630.net","threadId":"41856","inReplyTo":"20171126193813.12531-1-max@max630.net","subject":"[PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T19:38:12Z","receivedAt":"2017-11-26T19:38:34Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"From: Florian Manschwetus \n\nFrom: Florian Manschwetus \nDate: Wed, 30 Mar 2016 10:54:21 +0200\n\nhttp-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\nSigned-off-by: Max Kirillov \n---\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 51 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 231f9a750b..d3ec14ab74 100644\n--- a/config.c\n+++ b/config.c\n@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n-static int git_parse_ssize_t(const char *value, ssize_t *ret)\n+int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\ndiff --git a/config.h b/config.h\nindex 0352da117b..46a4989def 100644\n--- a/config.h\n+++ b/config.h\n@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);\n extern int config_with_options(config_fn_t fn, void *,\n \t\t\t struct git_config_source *config_source,\n \t\t\t const struct config_options *opts);\n+extern int git_parse_ssize_t(const char *, ssize_t *);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/http-backend.c b/http-backend.c\nindex 519025d2c3..af7dd00d70 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): %lu;\"\n+\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t max_request_buffer,\n+\t\t\t req_len);\n+\t}\n+\n+\tif (req_len <= 0) {\n+\t\t*out = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t} else {\n+\t\t*out = buf;\n+\t\treturn cnt;\n+\t}\n+}\n+\n+static ssize_t env_content_length(void)\n+{\n+\tssize_t val = -1;\n+\tconst char *str = getenv(\"CONTENT_LENGTH\");\n+\n+\tif (str && !git_parse_ssize_t(str, &val))\n+\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\treturn val;\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tssize_t req_len = env_content_length();\n+\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333537","messageId":"20171126193813.12531-3-max@max630.net","threadId":"41856","inReplyTo":"20171126193813.12531-1-max@max630.net","subject":"[PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T19:38:13Z","receivedAt":"2017-11-26T19:38:36Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Add tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data.\n (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero\n and fail. It does not seem to cause any performance issues with the default\n value of GIT_HTTP_MAX_REQUEST_BUFFER.)\n* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.\n\nSigned-off-by: Max Kirillov \n---\n Makefile | 1 +\n t/helper/test-print-values.c | 10 ++++++++++\n t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+)\n create mode 100644 t/helper/test-print-values.c\n\ndiff --git a/Makefile b/Makefile\nindex 461c845d33..616408b32c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-online-cpus\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-print-values\n TEST_PROGRAMS_NEED_X += test-prio-queue\n TEST_PROGRAMS_NEED_X += test-read-cache\n TEST_PROGRAMS_NEED_X += test-ref-store\ndiff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c\nnew file mode 100644\nindex 0000000000..8f7e5af319\n--- /dev/null\n+++ b/t/helper/test-print-values.c\n@@ -0,0 +1,10 @@\n+#include \n+#include \n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tif (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n+\t\tprintf(\"%zu\", (ssize_t)(-20));\n+\n+\treturn 0;\n+}\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9fafcf1945..f452090216 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n \texpect_aliased 1 //domain/data.txt\n '\n \n+# overrides existing definition for further cases\n+run_backend() {\n+\tCONTENT_LENGTH=\"${#2}\" && export CONTENT_LENGTH &&\n+\t( echo \"$2\" && cat /dev/zero ) |\n+\tQUERY_STRING=\"${1#*[?]}\" \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n+\tgit http-backend >act.out 2>act.err\n+}\n+\n+test_expect_success 'CONTENT_LENGTH set and infinite input' '\n+\tconfig http.uploadpack true &&\n+\tGET info/refs?service=git-upload-pack \"200 OK\"\t&&\n+\t! grep \"fatal:.*\" act.err &&\n+\tPOST git-upload-pack 0000 \"200 OK\" &&\n+\t! grep \"fatal:.*\" act.err\n+'\n+\n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=`\"$GIT_BUILD_DIR/t/helper/test-print-values\" \"(size_t)(-20)\"` &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n test_done\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"333538","messageId":"20171126193911.GE26158@jessie.local","threadId":"41856","inReplyTo":"xmqq3751idmp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T19:39:11Z","receivedAt":"2017-11-26T19:39:20Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Nov 26, 2017 at 06:38:06PM +0900, Junio C Hamano wrote:\n> Max Kirillov writes:\n>> +static ssize_t env_content_length()\n> \n> We need s/length()/length(void)/; though.\n\nthanks, fixed it\n"},{"id":"333552","messageId":"CAPig+cTuBRzoxtmp1_MQ3fUxC5LXpyFPAaEOL7z96e_Szfqn5w@mail.gmail.com","threadId":"41856","inReplyTo":"20171126193813.12531-2-max@max630.net","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-26T22:08:59Z","receivedAt":"2017-11-26T22:09:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov wrote:\n> [...]\n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the variable is not defined, keep older behavior\n> of reading until EOF because it is used to support chunked transfer-encoding.\n> [...]\n\nA few small comments below; with the possible exception of one,\nprobably none worth a re-roll...\n\n> diff --git a/http-backend.c b/http-backend.c\n> @@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)\n> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n> +{\n> + unsigned char *buf = NULL;\n> + ssize_t cnt = 0;\n> +\n> + if (max_request_buffer < req_len) {\n> + die(\"request was larger than our maximum size (%lu): %lu;\"\n> + \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n> + max_request_buffer,\n> + req_len);\n\nUnsigned format conversion '%lu' doesn't seem correct for ssize_t.\n\n> + }\n> +\n> + if (req_len <= 0) {\n> + *out = NULL;\n> + return 0;\n> + }\n> +\n> + buf = xmalloc(req_len);\n> + cnt = read_in_full(fd, buf, req_len);\n> + if (cnt < 0) {\n> + free(buf);\n> + return -1;\n> + } else {\n> + *out = buf;\n> + return cnt;\n> + }\n\nThis could have been written:\n\n if (cnt < 0) {\n free(buf);\n return -1;\n }\n *out = buf;\n return cnt;\n\nbut not worth a re-roll.\n\n> +}\n> +\n> +static ssize_t env_content_length(void)\n\nThe caller of this function doesn't care how the content length is\nbeing determined -- whether it comes from an environment variable or\nis computed some other way; it cares only about the result. Having\n\"env\" in the name ties it to checking only the environment. A more\ngeneric name, such as get_content_length(), would help to decouple the\nAPI from the implementation.\n\nNevertheless, not worth a re-roll.\n\n> +{\n> + ssize_t val = -1;\n> + const char *str = getenv(\"CONTENT_LENGTH\");\n> +\n> + if (str && !git_parse_ssize_t(str, &val))\n\ngit_parse_ssize_t() does the right thing even when 'str' is NULL, so\nthis condition could be simplified (but not worth a re-roll and may\nnot improve clarity).\n\n> + die(\"failed to parse CONTENT_LENGTH: %s\", str);\n> + return val;\n> +}\n> +\n> +static ssize_t read_request(int fd, unsigned char **out)\n> +{\n> + ssize_t req_len = env_content_length();\n\nGrabbing and parsing the value from the environment variable is\neffectively a one-liner, so env_content_length() could be dropped\naltogether, and instead (taking advantage of git_parse_ssize_t()'s\nproper NULL-handling):\n\n if (!git_parse_ssize_t(getenv(...), &req_len))\n die(...);\n\nNot worth a re-roll.\n\n> + if (req_len < 0)\n> + return read_request_eof(fd, out);\n> + else\n> + return read_request_fixed_len(fd, req_len, out);\n> +}\n"},{"id":"333553","messageId":"CAPig+cROFS+3Yon2CFr-aPUGzDY4WVbU8kHbBx68O3M8k5eeug@mail.gmail.com","threadId":"41856","inReplyTo":"20171126193813.12531-3-max@max630.net","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-26T22:18:55Z","receivedAt":"2017-11-26T22:19:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov wrote:\n> Add tests for cases:\n>\n> * CONTENT_LENGTH is set, script's stdin has more data.\n> (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero\n> and fail. It does not seem to cause any performance issues with the default\n> value of GIT_HTTP_MAX_REQUEST_BUFFER.)\n> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.\n>\n> Signed-off-by: Max Kirillov \n> ---\n> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c\n> @@ -0,0 +1,10 @@\n> +int cmd_main(int argc, const char **argv)\n> +{\n> + if (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n> + printf(\"%zu\", (ssize_t)(-20));\n> +\n> + return 0;\n\nPerhaps this should return 0 only if it gets the expected argument\n\"(size_t)(-20)\", and return an error otherwise.\n\n> +}\n> diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\n> @@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n> +test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n> + NOT_FIT_IN_SSIZE=`\"$GIT_BUILD_DIR/t/helper/test-print-values\" \"(size_t)(-20)\"` &&\n\nRather than introducing a new 'test' program, would it be possible to\nget by with just using 'printf' from the shell?\n\n % printf \"%zu\\n\" -20\n 18446744073709551596\n\n> + env \\\n> + CONTENT_TYPE=application/x-git-upload-pack-request \\\n> + QUERY_STRING=/repo.git/git-upload-pack \\\n> + PATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n> + GIT_HTTP_EXPORT_ALL=TRUE \\\n> + REQUEST_METHOD=POST \\\n> + CONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n> + git http-backend /dev/null 2>err &&\n> + grep -q \"fatal:.*CONTENT_LENGTH\" err\n> +'\n"},{"id":"333554","messageId":"20171126224051.GF26158@jessie.local","threadId":"41856","inReplyTo":"CAPig+cROFS+3Yon2CFr-aPUGzDY4WVbU8kHbBx68O3M8k5eeug@mail.gmail.com","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-26T22:40:51Z","receivedAt":"2017-11-26T22:41:01Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Nov 26, 2017 at 05:18:55PM -0500, Eric Sunshine wrote:\n> On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov wrote:\n>> +int cmd_main(int argc, const char **argv)\n>> +{\n>> + if (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n>> + printf(\"%zu\", (ssize_t)(-20));\n>> +\n>> + return 0;\n> \n> Perhaps this should return 0 only if it gets the expected argument\n> \"(size_t)(-20)\", and return an error otherwise.\n\nYes, makes sense.\n\n> Rather than introducing a new 'test' program, would it be possible to\n> get by with just using 'printf' from the shell?\n> \n> % printf \"%zu\\n\" -20\n> 18446744073709551596\n\nI thought about it, of course. But, I am not sure I can\nexclude cases when the shell's printf uses 64-bit size_t and\ngit 32-bit one, or vise-versa. Same way, I cannot say it for\nsure for any other software which I might use here instead\nof the shell's printf. The only somewhat sure way would be\nto use the same compiler, with same settings, which is used\nfor the production code.\n\nI do not exclude possibility that my reasoning above is\nwrong, either in general of specifically for git case. If\nthere are some examples where it is already used and the\nrisk of type size mismatch is prevented I could do it\nsimilarly.\n"},{"id":"333559","messageId":"xmqqbmjoh8db.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171126193813.12531-3-max@max630.net","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T00:29:20Z","receivedAt":"2017-11-27T00:29:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> Add tests for cases:\n>\n> * CONTENT_LENGTH is set, script's stdin has more data.\n> (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero\n> and fail. It does not seem to cause any performance issues with the default\n> value of GIT_HTTP_MAX_REQUEST_BUFFER.)\n> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.\n\ns/fix/fit/ you meant?\n\n> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c\n> new file mode 100644\n> index 0000000000..8f7e5af319\n> --- /dev/null\n> +++ b/t/helper/test-print-values.c\n> @@ -0,0 +1,10 @@\n> +#include \n> +#include \n> +\n> +int cmd_main(int argc, const char **argv)\n> +{\n> +\tif (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n> +\t\tprintf(\"%zu\", (ssize_t)(-20));\n> +\n> +\treturn 0;\n> +}\n\nAs far as I know, we avoid %zu (C99), as it may not be safe yet to\ndo so on all platforms.\n\ne.g. c.f. https://public-inbox.org/git/64C7D52F-9030-460C-8F61-4076F5C1DDF6@gmail.com/\n\nYou may want to double check the 1/2 of this topic, too.\n\nForcing a test command line to spell out \"(size_t)(-20)\" feels a bit\natrocious, especially given that this program is capable of ever\nshowing that string and nothing else, and it does not even diagnose\ntypos as errors.\n\nI wonder if we would want to have \"test-print-larger-than-ssize\" and\ndo something like\n\n #include \"cache.h\"\n\n int cmd_main(int ac, const char **av)\n {\n uintmax_t large = ((uintmax_t) SSIZE_MAX) + 1;\n\n printf(\"%\" PRIuMAX \"\\n\", large);\n return 0;\n }\n\nperhaps?\n\nNote that wrapper.c seems to assume that not everybody has\nSSIZE_MAX, so we might have to do something silly like\n\n\tsize_t large = ~0;\n\tlarge = ~(large & ~(large >> 1)) + 1;\n\n printf(\"%\" PRIuMAX \"\\n\", (uintmax_t) large);\n\njust to be careful (even though we now assume 2's complement),\nthough.\n"},{"id":"333571","messageId":"xmqqzi78e5dp.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171126193813.12531-1-max@max630.net","subject":"Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T04:02:10Z","receivedAt":"2017-11-27T04:02:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"To recap (other than the typofix in the proposed log message), here\nis what I would have as SQUASH??? on top of (or interspersed with)\nv6.\n\nThanks.\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 69570d16e7..2268d65731 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -324,10 +324,9 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n \tssize_t cnt = 0;\n \n \tif (max_request_buffer < req_len) {\n-\t\tdie(\"request was larger than our maximum size (%lu): %lu;\"\n-\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n-\t\t\t max_request_buffer,\n-\t\t\t req_len);\n+\t\tdie(\"request was larger than our maximum size (%lu): \"\n+\t\t \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t max_request_buffer, (uintmax_t)req_len);\n \t}\n \n \tif (req_len <= 0) {\n\ndiff --git a/Makefile b/Makefile\nindex e61f8319b3..3380f68040 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -661,7 +661,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-online-cpus\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n-TEST_PROGRAMS_NEED_X += test-print-values\n+TEST_PROGRAMS_NEED_X += test-print-larger-than-ssize\n TEST_PROGRAMS_NEED_X += test-prio-queue\n TEST_PROGRAMS_NEED_X += test-read-cache\n TEST_PROGRAMS_NEED_X += test-write-cache\n\ndiff --git a/t/helper/test-print-values.c b/t/helper/test-print-larger-than-ssize.c\nsimilarity index 31%\nrename from t/helper/test-print-values.c\nrename to t/helper/test-print-larger-than-ssize.c\nindex 8f7e5af319..b9852c493d 100644\n--- a/t/helper/test-print-values.c\n+++ b/t/helper/test-print-larger-than-ssize.c\n@@ -1,10 +1,10 @@\n-#include \n-#include \n+#include \"cache.h\"\n \n int cmd_main(int argc, const char **argv)\n {\n-\tif (argc == 2 && strcmp(argv[1], \"(size_t)(-20)\") == 0)\n-\t\tprintf(\"%zu\", (ssize_t)(-20));\n+\tsize_t large = ~0;\n \n+\tlarge = ~(large & ~(large >> 1)) + 1;\n+\tprintf(\"%\" PRIuMAX \"\\n\", (uintmax_t) large);\n \treturn 0;\n }\n\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex f452090216..112b5d6eb2 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -72,7 +72,7 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n '\n \n # overrides existing definition for further cases\n-run_backend() {\n+run_backend () {\n \tCONTENT_LENGTH=\"${#2}\" && export CONTENT_LENGTH &&\n \t( echo \"$2\" && cat /dev/zero ) |\n \tQUERY_STRING=\"${1#*[?]}\" \\\n@@ -89,7 +89,7 @@ test_expect_success 'CONTENT_LENGTH set and infinite input' '\n '\n \n test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n-\tNOT_FIT_IN_SSIZE=`\"$GIT_BUILD_DIR/t/helper/test-print-values\" \"(size_t)(-20)\"` &&\n+\tNOT_FIT_IN_SSIZE=$(\"$GIT_BUILD_DIR/t/helper/test-print-larger-than-ssize\") &&\n \tenv \\\n \t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n \t\tQUERY_STRING=/repo.git/git-upload-pack \\\n"},{"id":"333766","messageId":"20171129032214.GB32345@sigill.intra.peff.net","threadId":"41856","inReplyTo":"20171126193813.12531-2-max@max630.net","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-29T03:22:14Z","receivedAt":"2017-11-29T03:22:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 26, 2017 at 09:38:12PM +0200, Max Kirillov wrote:\n\n> From: Florian Manschwetus \n> Date: Wed, 30 Mar 2016 10:54:21 +0200\n> \n> http-backend reads whole input until EOF. However, the RFC 3875 specifies\n> that a script must read only as many bytes as specified by CONTENT_LENGTH\n> environment variable. Web server may exercise the specification by not closing\n> the script's standard input after writing content. In that case http-backend\n> would hang waiting for the input. The issue is known to happen with\n> IIS/Windows, for example.\n> \n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the variable is not defined, keep older behavior\n> of reading until EOF because it is used to support chunked transfer-encoding.\n\nI missed out on review of the earlier iterations from the past few days,\nbut I looked this over in light of the comments I made long ago in:\n\n https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/\n\nThe concerns I had there were:\n\n 1. It did the wrong thing when CONTENT_LENGTH was not present in the\n environment. Your version here gets this right.\n\n 2. I earlier was worried that this wouldn't kick in for the\n inflate_request() code path. It looks like we do use read_request()\n from inflate_request(), but only when buffer_input is true. Which\n means we wouldn't do so for receive-pack (i.e., for pushes).\n\n I'm not sure if the client would ever gzip a push request. Without\n double-checking, I suspect it would if the list of refs to update\n was sufficiently large (and at any rate, I think other clients\n potentially could do so).\n\n That _might_ be OK in practice. If the gzip stream is well-formed,\n we'd stop at its end. We'd possibly still try to read() more bytes\n than were promised, but if I understand the original problem,\n that's not that big a deal as long as we don't hang waiting for\n EOF.\n\n If the stream isn't well-formed, we'd hang waiting for more bytes\n (rather than seeing the EOF and complaining that the gzip stream is\n truncated). That's poor, but no worse than the current behavior.\n\n 3. For large inputs (like incoming packfiles), we connect the\n descriptor directly to index-pack or unpack-objects, and they try\n to read to EOF.\n\n For a well-formed pack, I _think_ this would work OK. We'd see the\n end of the pack and quit (there's a check for garbage at the end of\n the pack, but it triggers only for the non-pipe case).\n\n For a truncated input, we'd hang forever rather than report an\n error.\n\nSo I suspect there are lurking problems that may trigger in corner\ncases. That said, I don't think this pack makes any case _worse_, and it\nmay make some common ones better. So I'm not opposed, though we may be\ngiving people a false sense of security that it actually works robustly\non IIS.\n\n> ---\n> config.c | 2 +-\n> config.h | 1 +\n> http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-\n> 3 files changed, 51 insertions(+), 2 deletions(-)\n\nThe patch itself looks OK, modulo the comments already made by others.\nTwo minor observations, and a possible bug:\n\n> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n> +{\n\nI did wonder if this \"stop at n bytes\" could simply be rolled into the\nexisting read_request loop (by limiting the length we pass to\nread_in_full there). But it may be cleaner to just have a separate\nfunction. There's some repetition, but not much since we can rely on a\nsingle malloc and read_in_full() for this case.\n\n> +\tunsigned char *buf = NULL;\n> +\tssize_t cnt = 0;\n> +\n> +\tif (max_request_buffer < req_len) {\n> +\t\tdie(\"request was larger than our maximum size (%lu): %lu;\"\n> +\t\t\t \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n> +\t\t\t max_request_buffer,\n> +\t\t\t req_len);\n> +\t}\n> +\n> +\tif (req_len <= 0) {\n> +\t\t*out = NULL;\n> +\t\treturn 0;\n> +\t}\n\nI was slightly surprised by \"<= 0\" here. We should never get here with a\nnegative req_len, since we'd catch that in the read_request() wrapper\nhere. If we want to document that assumption, should this be\nassert(req_len >= 0)?\n\nI'm also puzzled about the behavior with a zero-byte CONTENT_LENGTH.\nWe'd return NULL here. But in the other read_request_eof() path, we'd\nend up with a non-NULL pointer. I'm not sure if it matters to the caller\nor not, but it seems like a potential trap.\n\nIs it even worth special-casing here? Our xmalloc and read_in_full\nwrappers should handle the zero-byte just fine. I think this whole\nconditional could just go away.\n\n-Peff\n"},{"id":"333767","messageId":"20171129032632.GC32345@sigill.intra.peff.net","threadId":"41856","inReplyTo":"20171126224051.GF26158@jessie.local","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-29T03:26:33Z","receivedAt":"2017-11-29T03:26:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:\n\n> > Rather than introducing a new 'test' program, would it be possible to\n> > get by with just using 'printf' from the shell?\n> > \n> > % printf \"%zu\\n\" -20\n> > 18446744073709551596\n> \n> I thought about it, of course. But, I am not sure I can\n> exclude cases when the shell's printf uses 64-bit size_t and\n> git 32-bit one, or vise-versa. Same way, I cannot say it for\n> sure for any other software which I might use here instead\n> of the shell's printf. The only somewhat sure way would be\n> to use the same compiler, with same settings, which is used\n> for the production code.\n> \n> I do not exclude possibility that my reasoning above is\n> wrong, either in general of specifically for git case. If\n> there are some examples where it is already used and the\n> risk of type size mismatch is prevented I could do it\n> similarly.\n\nThat's definitely something to worry about, and I have a vague\nrecollection that build differences between the shell environment and\ngit have bitten us in the past.\n\nThat said, we already have some precedent in \"git version\n--build-options\" to report sizes there. Can we do something like the\npatch below instead of adding a new test helper?\n\ndiff --git a/help.c b/help.c\nindex 88a3aeaeb9..9590eaba28 100644\n--- a/help.c\n+++ b/help.c\n@@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n \n \tif (build_options) {\n \t\tprintf(\"sizeof-long: %d\\n\", (int)sizeof(long));\n+\t\tprintf(\"sizeof-size_t: %d\\n\", (int)sizeof(size_t));\n \t\t/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */\n \t}\n \treturn 0;\n\nThat does still require you to compute size_t based on the byte-size in\nthe test script, that should be do-able.\n\n-Peff\n"},{"id":"333770","messageId":"20171129050730.GG26158@jessie.local","threadId":"41856","inReplyTo":"xmqqzi78e5dp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-29T05:07:30Z","receivedAt":"2017-11-29T05:07:41Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Nov 27, 2017 at 01:02:10PM +0900, Junio C Hamano wrote:\n> To recap (other than the typofix in the proposed log message), here\n> is what I would have as SQUASH??? on top of (or interspersed with)\n> v6.\n\nThank you. I'll update it a bit later. May/should I add\n\"Signed-off-by:\" from you?\n"},{"id":"333771","messageId":"20171129051919.GH26158@jessie.local","threadId":"41856","inReplyTo":"20171129032632.GC32345@sigill.intra.peff.net","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-11-29T05:19:19Z","receivedAt":"2017-11-29T05:19:28Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Nov 28, 2017 at 10:26:33PM -0500, Jeff King wrote:\n> On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:\n> That said, we already have some precedent in \"git version\n> --build-options\" to report sizes there. Can we do something like the\n> patch below instead of adding a new test helper?\n> \n> diff --git a/help.c b/help.c\n> index 88a3aeaeb9..9590eaba28 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n> \n> \tif (build_options) {\n> \t\tprintf(\"sizeof-long: %d\\n\", (int)sizeof(long));\n> +\t\tprintf(\"sizeof-size_t: %d\\n\", (int)sizeof(size_t));\n> \t\t/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */\n> \t}\n> \treturn 0;\n\nThank you! I knew there should have been something.\n\nIf nobody objects changing the user-visible behavior, I'll\nconsider using this.\n\nPS: I'll respond to your other reply a bit later.\n"},{"id":"333976","messageId":"xmqq374smyeo.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171129051919.GH26158@jessie.local","subject":"Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T00:46:07Z","receivedAt":"2017-12-03T00:46:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> If nobody objects changing the user-visible behavior, I'll\n> consider using this.\n\nWhat user-visible behaviour? A user tries to use a new test helper\nintroduced by the previous round and does not find it? That's OK.\n\n> PS: I'll respond to your other reply a bit later.\n\nThanks. \n"},{"id":"333977","messageId":"xmqqy3mkljq3.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171129050730.GG26158@jessie.local","subject":"Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T00:48:36Z","receivedAt":"2017-12-03T00:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> On Mon, Nov 27, 2017 at 01:02:10PM +0900, Junio C Hamano wrote:\n>> To recap (other than the typofix in the proposed log message), here\n>> is what I would have as SQUASH??? on top of (or interspersed with)\n>> v6.\n>\n> Thank you. I'll update it a bit later. May/should I add\n> \"Signed-off-by:\" from you?\n\nAs you'd be dropping the new helper, I think you will not use a\nmajor part of that message. But even if you would, a \"Helped-by:\"\nbefore your sign-off should be enough for a small \"help\" like the\none in the message you are responding.\n\nThanks.\n"},{"id":"333978","messageId":"xmqqr2sclj2q.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171129032214.GB32345@sigill.intra.peff.net","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T01:02:37Z","receivedAt":"2017-12-03T01:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 3. For large inputs (like incoming packfiles), we connect the\n> descriptor directly to index-pack or unpack-objects, and they try\n> to read to EOF.\n>\n> For a well-formed pack, I _think_ this would work OK. We'd see the\n> end of the pack and quit (there's a check for garbage at the end of\n> the pack, but it triggers only for the non-pipe case).\n>\n> For a truncated input, we'd hang forever rather than report an\n> error.\n\nHmm. index-pack and/or unpack-objects would be fed directly from\nthe incoming pipe, they are not told how many bytes to expect (by\ndesign), so they try to read to EOF, which may come after the end of\nthe pack-stream data, and they write the remaining junk to their\nstandard output IIRC.\n\nFor a well-formed pack, the above is what should heppen.\n\nI am having trouble trying to come up with a case where the input\nstream is mangled and we cannot detect where the end of the\npack-stream is without reading more than we will be fed through the\npipe, and yet we do not trigger an \"we tried to read because the data\nwe received so far is incomplete, and got an EOF\" error.\n\nWouldn't \"early EOF\" trigger in the fill() helper that these two\nprograms have (but not share X-<)?\n\n"},{"id":"333983","messageId":"20171203024958.GA31493@sigill.intra.peff.net","threadId":"41856","inReplyTo":"xmqqr2sclj2q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-03T02:49:58Z","receivedAt":"2017-12-03T02:50:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 02, 2017 at 05:02:37PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > 3. For large inputs (like incoming packfiles), we connect the\n> > descriptor directly to index-pack or unpack-objects, and they try\n> > to read to EOF.\n> >\n> > For a well-formed pack, I _think_ this would work OK. We'd see the\n> > end of the pack and quit (there's a check for garbage at the end of\n> > the pack, but it triggers only for the non-pipe case).\n> >\n> > For a truncated input, we'd hang forever rather than report an\n> > error.\n> \n> Hmm. index-pack and/or unpack-objects would be fed directly from\n> the incoming pipe, they are not told how many bytes to expect (by\n> design), so they try to read to EOF, which may come after the end of\n> the pack-stream data, and they write the remaining junk to their\n> standard output IIRC.\n> \n> For a well-formed pack, the above is what should heppen.\n\nYeah, there should be zero bytes of \"remaining junk\" in the normal\nwell-formed case. And as long as the webserver does not mind us asking\nto read() a few extra bytes, we are fine (it tells us there are no more\nbytes available right now). The original problem report with IIS was\nthat it would hang trying to read() that any final EOF, and I don't\nthink that would happen here.\n\nI wouldn't be surprised if there are webservers or situations where that\nextra read() behaves badly (e.g., because it is connected directly to\nthe client socket and the client is trying to pipeline requests or\nsomething).\n\n> I am having trouble trying to come up with a case where the input\n> stream is mangled and we cannot detect where the end of the\n> pack-stream is without reading more than we will be fed through the\n> pipe, and yet we do not trigger an \"we tried to read because the data\n> we received so far is incomplete, and got an EOF\" error.\n> \n> Wouldn't \"early EOF\" trigger in the fill() helper that these two\n> programs have (but not share X-<)?\n\nI think the original problem was the opposite of \"early EOF\": the other\nside of the pipe never gives us EOF at all. So imagine the pack is\nmangled so that the zlib stream of an object never ends, and just keeps\nasking for more data. Eventually our fill() will block trying to get\ndata that is not there. On an Apache server, the webserver would know\nthere is nothing left to send us and close() the pipe, and we'd get EOF.\nBut on IIS, I think the pipe remains open and we'd just block\nindefinitely trying to read().\n\nI don't have such a setup to test on, and it's possible I'm\nmis-interpreting or mis-remembering the discussion around the original\npatch.\n\n-Peff\n"},{"id":"333990","messageId":"xmqq1skcjqer.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171203024958.GA31493@sigill.intra.peff.net","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T06:07:08Z","receivedAt":"2017-12-03T06:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... Eventually our fill() will block trying to get data that is\n> not there. On an Apache server, the webserver would know there is\n> nothing left to send us and close() the pipe, and we'd get EOF.\n> But on IIS, I think the pipe remains open and we'd just block\n> indefinitely trying to read().\n\nAh, yeah, under that scenario, trusting content-length and trying to\nread, waiting for input that would never come, will be a problem,\nand it would probably want to get documented.\n\n"},{"id":"334032","messageId":"F0F5A56A22F20D4CB4A03BB8D66587970253F87B86@SERVER2011.CS-SOFTWARE.local","threadId":"41856","inReplyTo":"xmqq1skcjqer.fsf@gitster.mtv.corp.google.com","subject":"AW: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Florian Manschwetus","fromEmail":"manschwetus@cs-software-gmbh.de","sentAt":"2017-12-04T07:18:55Z","receivedAt":"2017-12-04T07:25:13Z","isPatch":true,"sender":{"key":"manschwetus@cs-software-gmbh.de","avatar":null},"body":"Hi All,\nI could provide a bash script I used in between to make this working with IIS, without fixing http-backend binary, maybe this helps to understand how this cases might be handled.\n\nMit freundlichen Grüßen / With kind regards\nFlorian Manschwetus\n\n\n\n> -----Ursprüngliche Nachricht-----\n> Von: Junio C Hamano [mailto:gitster@pobox.com]\n> Gesendet: Sonntag, 3. Dezember 2017 07:07\n> An: Jeff King\n> Cc: Max Kirillov; Eric Sunshine; Florian Manschwetus; Chris Packham;\n> Konstantin Khomoutov; git@vger.kernel.org\n> Betreff: Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as\n> specified by rfc3875\n> \n> Jeff King writes:\n> \n> > ... Eventually our fill() will block trying to get data that is not\n> > there. On an Apache server, the webserver would know there is nothing\n> > left to send us and close() the pipe, and we'd get EOF.\n> > But on IIS, I think the pipe remains open and we'd just block\n> > indefinitely trying to read().\n> \n> Ah, yeah, under that scenario, trusting content-length and trying to read,\n> waiting for input that would never come, will be a problem, and it would\n> probably want to get documented.\n\n"},{"id":"334067","messageId":"20171204171308.GA13332@sigill.intra.peff.net","threadId":"41856","inReplyTo":"F0F5A56A22F20D4CB4A03BB8D66587970253F87B86@SERVER2011.CS-SOFTWARE.local","subject":"Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-04T17:13:08Z","receivedAt":"2017-12-04T17:13:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 04, 2017 at 07:18:55AM +0000, Florian Manschwetus wrote:\n\n> I could provide a bash script I used in between to make this working\n> with IIS, without fixing http-backend binary, maybe this helps to\n> understand how this cases might be handled.\n\nI think it would be pretty easy to handle all cases by inserting another\nprocess in front of http-backend that just reads $CONTENT_LENGTH bytes\nand then gives an EOF to http-backend. I assume that's what your bash\nscript does. It's just that this passes the data through an extra layer\nof pipes.\n\nIf that's an acceptable tradeoff, it seems like an ideal solution from a\nmaintainability standpoint, since it skips all the tricky situations\ninside the http-backend code.\n\n-Peff\n"},{"id":"334682","messageId":"20171212161701.4272-1-dnj@google.com","threadId":"41856","inReplyTo":"xmqqy3mkljq3.fsf@gitster.mtv.corp.google.com","subject":"Need to add test artifacts to .gitignore","fromName":"Dan Jacques","fromEmail":"dnj@google.com","sentAt":"2017-12-12T16:17:01Z","receivedAt":"2017-12-12T16:17:12Z","isPatch":false,"sender":{"key":"dnj@google.com","avatar":"https://avatars.githubusercontent.com/u/9965104?v=4"},"body":"FYI, I've noticed when building from \"pu\" that neither the\n\"t/helper/test-print-values\" or \"t/helper/test-print-larger-than-ssize\"\ntesting artifacts added to Makefile in this patch series are not added to\n\"t/helper/.gitignore\" like other helpers, resulting in the testing artifact\nbeing recognized as an untracked file.\n"},{"id":"334691","messageId":"20171212190026.17671-1-sbeller@google.com","threadId":"41856","inReplyTo":"20171212161701.4272-1-dnj@google.com","subject":"[RFC PATCH] t/helper: Move sources to t/helper-src; gitignore any files in t/helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-12T19:00:26Z","receivedAt":"2017-12-12T19:00:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Compiled test helpers in t/helper are out of sync with the .gitignore\nfiles quite frequently. This can happen when new test helpers are added,\nbut the explicit .gitignore file is not updated in the same commit, or\nwhen you forget to 'make clean' before checking out a different version\nof git, as the different version may have a different explicit list of\ntest helpers to ignore.\n\nThis can be fixed by using overly broad ignore patterns, such as ignoring\nthe whole directory via '//t/helper/*' in .gitignore.\n\nHowever we do not want to have an overlap of checked source files and\nignored files, hence we'll move the the source files currently residing\nin t/helper to t/helper-src. To accommodate that we'll need to update\nthe Makefile as well to look at a different place for the source files.\n(This patch takes the hacky approach in symlinking the sources back into\nthe t/helper, which we'd want to avoid long term)\n\nSigned-off-by: Stefan Beller \n---\n Makefile | 4 +++\n t/{helper => helper-src}/.gitignore | 0\n t/{helper => helper-src}/test-chmtime.c | 0\n t/{helper => helper-src}/test-config.c | 0\n t/{helper => helper-src}/test-ctype.c | 0\n t/{helper => helper-src}/test-date.c | 0\n t/{helper => helper-src}/test-delta.c | 0\n t/{helper => helper-src}/test-dump-cache-tree.c | 0\n t/{helper => helper-src}/test-dump-split-index.c | 0\n .../test-dump-untracked-cache.c | 0\n t/{helper => helper-src}/test-fake-ssh.c | 0\n t/{helper => helper-src}/test-genrandom.c | 0\n t/{helper => helper-src}/test-hashmap.c | 0\n t/{helper => helper-src}/test-index-version.c | 0\n .../test-lazy-init-name-hash.c | 0\n t/{helper => helper-src}/test-line-buffer.c | 0\n t/{helper => helper-src}/test-match-trees.c | 0\n t/{helper => helper-src}/test-mergesort.c | 0\n t/{helper => helper-src}/test-mktemp.c | 0\n t/{helper => helper-src}/test-online-cpus.c | 0\n t/{helper => helper-src}/test-parse-options.c | 0\n t/{helper => helper-src}/test-path-utils.c | 0\n t/{helper => helper-src}/test-prio-queue.c | 0\n t/{helper => helper-src}/test-read-cache.c | 0\n t/{helper => helper-src}/test-ref-store.c | 0\n t/{helper => helper-src}/test-regex.c | 0\n t/{helper => helper-src}/test-revision-walking.c | 0\n t/{helper => helper-src}/test-run-command.c | 0\n t/{helper => helper-src}/test-scrap-cache-tree.c | 0\n t/{helper => helper-src}/test-sha1-array.c | 0\n t/{helper => helper-src}/test-sha1.c | 0\n t/{helper => helper-src}/test-sha1.sh | 0\n t/{helper => helper-src}/test-sigchain.c | 0\n t/{helper => helper-src}/test-strcmp-offset.c | 0\n t/{helper => helper-src}/test-string-list.c | 0\n t/{helper => helper-src}/test-submodule-config.c | 0\n t/{helper => helper-src}/test-subprocess.c | 0\n t/{helper => helper-src}/test-svn-fe.c | 0\n .../test-urlmatch-normalization.c | 0\n t/{helper => helper-src}/test-wildmatch.c | 0\n t/{helper => helper-src}/test-write-cache.c | 0\n t/helper/.gitignore | 39 +---------------------\n 42 files changed, 5 insertions(+), 38 deletions(-)\n copy t/{helper => helper-src}/.gitignore (100%)\n rename t/{helper => helper-src}/test-chmtime.c (100%)\n rename t/{helper => helper-src}/test-config.c (100%)\n rename t/{helper => helper-src}/test-ctype.c (100%)\n rename t/{helper => helper-src}/test-date.c (100%)\n rename t/{helper => helper-src}/test-delta.c (100%)\n rename t/{helper => helper-src}/test-dump-cache-tree.c (100%)\n rename t/{helper => helper-src}/test-dump-split-index.c (100%)\n rename t/{helper => helper-src}/test-dump-untracked-cache.c (100%)\n rename t/{helper => helper-src}/test-fake-ssh.c (100%)\n rename t/{helper => helper-src}/test-genrandom.c (100%)\n rename t/{helper => helper-src}/test-hashmap.c (100%)\n rename t/{helper => helper-src}/test-index-version.c (100%)\n rename t/{helper => helper-src}/test-lazy-init-name-hash.c (100%)\n rename t/{helper => helper-src}/test-line-buffer.c (100%)\n rename t/{helper => helper-src}/test-match-trees.c (100%)\n rename t/{helper => helper-src}/test-mergesort.c (100%)\n rename t/{helper => helper-src}/test-mktemp.c (100%)\n rename t/{helper => helper-src}/test-online-cpus.c (100%)\n rename t/{helper => helper-src}/test-parse-options.c (100%)\n rename t/{helper => helper-src}/test-path-utils.c (100%)\n rename t/{helper => helper-src}/test-prio-queue.c (100%)\n rename t/{helper => helper-src}/test-read-cache.c (100%)\n rename t/{helper => helper-src}/test-ref-store.c (100%)\n rename t/{helper => helper-src}/test-regex.c (100%)\n rename t/{helper => helper-src}/test-revision-walking.c (100%)\n rename t/{helper => helper-src}/test-run-command.c (100%)\n rename t/{helper => helper-src}/test-scrap-cache-tree.c (100%)\n rename t/{helper => helper-src}/test-sha1-array.c (100%)\n rename t/{helper => helper-src}/test-sha1.c (100%)\n rename t/{helper => helper-src}/test-sha1.sh (100%)\n rename t/{helper => helper-src}/test-sigchain.c (100%)\n rename t/{helper => helper-src}/test-strcmp-offset.c (100%)\n rename t/{helper => helper-src}/test-string-list.c (100%)\n rename t/{helper => helper-src}/test-submodule-config.c (100%)\n rename t/{helper => helper-src}/test-subprocess.c (100%)\n rename t/{helper => helper-src}/test-svn-fe.c (100%)\n rename t/{helper => helper-src}/test-urlmatch-normalization.c (100%)\n rename t/{helper => helper-src}/test-wildmatch.c (100%)\n rename t/{helper => helper-src}/test-write-cache.c (100%)\n rewrite t/helper/.gitignore (100%)\n\ndiff --git a/Makefile b/Makefile\nindex c26596c30a..477ddef820 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2454,6 +2454,10 @@ t/helper/test-svn-fe$X: $(VCSSVN_LIB)\n t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n \n+t/helper/test-%.c: t/helper-src/test-%.c\n+\t@echo $@ $<\n+\tln -s ../../$< $@\n+\n check-sha1:: t/helper/test-sha1$X\n \tt/helper/test-sha1.sh\n \ndiff --git a/t/helper/.gitignore b/t/helper-src/.gitignore\nsimilarity index 100%\ncopy from t/helper/.gitignore\ncopy to t/helper-src/.gitignore\ndiff --git a/t/helper/test-chmtime.c b/t/helper-src/test-chmtime.c\nsimilarity index 100%\nrename from t/helper/test-chmtime.c\nrename to t/helper-src/test-chmtime.c\ndiff --git a/t/helper/test-config.c b/t/helper-src/test-config.c\nsimilarity index 100%\nrename from t/helper/test-config.c\nrename to t/helper-src/test-config.c\ndiff --git a/t/helper/test-ctype.c b/t/helper-src/test-ctype.c\nsimilarity index 100%\nrename from t/helper/test-ctype.c\nrename to t/helper-src/test-ctype.c\ndiff --git a/t/helper/test-date.c b/t/helper-src/test-date.c\nsimilarity index 100%\nrename from t/helper/test-date.c\nrename to t/helper-src/test-date.c\ndiff --git a/t/helper/test-delta.c b/t/helper-src/test-delta.c\nsimilarity index 100%\nrename from t/helper/test-delta.c\nrename to t/helper-src/test-delta.c\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper-src/test-dump-cache-tree.c\nsimilarity index 100%\nrename from t/helper/test-dump-cache-tree.c\nrename to t/helper-src/test-dump-cache-tree.c\ndiff --git a/t/helper/test-dump-split-index.c b/t/helper-src/test-dump-split-index.c\nsimilarity index 100%\nrename from t/helper/test-dump-split-index.c\nrename to t/helper-src/test-dump-split-index.c\ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper-src/test-dump-untracked-cache.c\nsimilarity index 100%\nrename from t/helper/test-dump-untracked-cache.c\nrename to t/helper-src/test-dump-untracked-cache.c\ndiff --git a/t/helper/test-fake-ssh.c b/t/helper-src/test-fake-ssh.c\nsimilarity index 100%\nrename from t/helper/test-fake-ssh.c\nrename to t/helper-src/test-fake-ssh.c\ndiff --git a/t/helper/test-genrandom.c b/t/helper-src/test-genrandom.c\nsimilarity index 100%\nrename from t/helper/test-genrandom.c\nrename to t/helper-src/test-genrandom.c\ndiff --git a/t/helper/test-hashmap.c b/t/helper-src/test-hashmap.c\nsimilarity index 100%\nrename from t/helper/test-hashmap.c\nrename to t/helper-src/test-hashmap.c\ndiff --git a/t/helper/test-index-version.c b/t/helper-src/test-index-version.c\nsimilarity index 100%\nrename from t/helper/test-index-version.c\nrename to t/helper-src/test-index-version.c\ndiff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper-src/test-lazy-init-name-hash.c\nsimilarity index 100%\nrename from t/helper/test-lazy-init-name-hash.c\nrename to t/helper-src/test-lazy-init-name-hash.c\ndiff --git a/t/helper/test-line-buffer.c b/t/helper-src/test-line-buffer.c\nsimilarity index 100%\nrename from t/helper/test-line-buffer.c\nrename to t/helper-src/test-line-buffer.c\ndiff --git a/t/helper/test-match-trees.c b/t/helper-src/test-match-trees.c\nsimilarity index 100%\nrename from t/helper/test-match-trees.c\nrename to t/helper-src/test-match-trees.c\ndiff --git a/t/helper/test-mergesort.c b/t/helper-src/test-mergesort.c\nsimilarity index 100%\nrename from t/helper/test-mergesort.c\nrename to t/helper-src/test-mergesort.c\ndiff --git a/t/helper/test-mktemp.c b/t/helper-src/test-mktemp.c\nsimilarity index 100%\nrename from t/helper/test-mktemp.c\nrename to t/helper-src/test-mktemp.c\ndiff --git a/t/helper/test-online-cpus.c b/t/helper-src/test-online-cpus.c\nsimilarity index 100%\nrename from t/helper/test-online-cpus.c\nrename to t/helper-src/test-online-cpus.c\ndiff --git a/t/helper/test-parse-options.c b/t/helper-src/test-parse-options.c\nsimilarity index 100%\nrename from t/helper/test-parse-options.c\nrename to t/helper-src/test-parse-options.c\ndiff --git a/t/helper/test-path-utils.c b/t/helper-src/test-path-utils.c\nsimilarity index 100%\nrename from t/helper/test-path-utils.c\nrename to t/helper-src/test-path-utils.c\ndiff --git a/t/helper/test-prio-queue.c b/t/helper-src/test-prio-queue.c\nsimilarity index 100%\nrename from t/helper/test-prio-queue.c\nrename to t/helper-src/test-prio-queue.c\ndiff --git a/t/helper/test-read-cache.c b/t/helper-src/test-read-cache.c\nsimilarity index 100%\nrename from t/helper/test-read-cache.c\nrename to t/helper-src/test-read-cache.c\ndiff --git a/t/helper/test-ref-store.c b/t/helper-src/test-ref-store.c\nsimilarity index 100%\nrename from t/helper/test-ref-store.c\nrename to t/helper-src/test-ref-store.c\ndiff --git a/t/helper/test-regex.c b/t/helper-src/test-regex.c\nsimilarity index 100%\nrename from t/helper/test-regex.c\nrename to t/helper-src/test-regex.c\ndiff --git a/t/helper/test-revision-walking.c b/t/helper-src/test-revision-walking.c\nsimilarity index 100%\nrename from t/helper/test-revision-walking.c\nrename to t/helper-src/test-revision-walking.c\ndiff --git a/t/helper/test-run-command.c b/t/helper-src/test-run-command.c\nsimilarity index 100%\nrename from t/helper/test-run-command.c\nrename to t/helper-src/test-run-command.c\ndiff --git a/t/helper/test-scrap-cache-tree.c b/t/helper-src/test-scrap-cache-tree.c\nsimilarity index 100%\nrename from t/helper/test-scrap-cache-tree.c\nrename to t/helper-src/test-scrap-cache-tree.c\ndiff --git a/t/helper/test-sha1-array.c b/t/helper-src/test-sha1-array.c\nsimilarity index 100%\nrename from t/helper/test-sha1-array.c\nrename to t/helper-src/test-sha1-array.c\ndiff --git a/t/helper/test-sha1.c b/t/helper-src/test-sha1.c\nsimilarity index 100%\nrename from t/helper/test-sha1.c\nrename to t/helper-src/test-sha1.c\ndiff --git a/t/helper/test-sha1.sh b/t/helper-src/test-sha1.sh\nsimilarity index 100%\nrename from t/helper/test-sha1.sh\nrename to t/helper-src/test-sha1.sh\ndiff --git a/t/helper/test-sigchain.c b/t/helper-src/test-sigchain.c\nsimilarity index 100%\nrename from t/helper/test-sigchain.c\nrename to t/helper-src/test-sigchain.c\ndiff --git a/t/helper/test-strcmp-offset.c b/t/helper-src/test-strcmp-offset.c\nsimilarity index 100%\nrename from t/helper/test-strcmp-offset.c\nrename to t/helper-src/test-strcmp-offset.c\ndiff --git a/t/helper/test-string-list.c b/t/helper-src/test-string-list.c\nsimilarity index 100%\nrename from t/helper/test-string-list.c\nrename to t/helper-src/test-string-list.c\ndiff --git a/t/helper/test-submodule-config.c b/t/helper-src/test-submodule-config.c\nsimilarity index 100%\nrename from t/helper/test-submodule-config.c\nrename to t/helper-src/test-submodule-config.c\ndiff --git a/t/helper/test-subprocess.c b/t/helper-src/test-subprocess.c\nsimilarity index 100%\nrename from t/helper/test-subprocess.c\nrename to t/helper-src/test-subprocess.c\ndiff --git a/t/helper/test-svn-fe.c b/t/helper-src/test-svn-fe.c\nsimilarity index 100%\nrename from t/helper/test-svn-fe.c\nrename to t/helper-src/test-svn-fe.c\ndiff --git a/t/helper/test-urlmatch-normalization.c b/t/helper-src/test-urlmatch-normalization.c\nsimilarity index 100%\nrename from t/helper/test-urlmatch-normalization.c\nrename to t/helper-src/test-urlmatch-normalization.c\ndiff --git a/t/helper/test-wildmatch.c b/t/helper-src/test-wildmatch.c\nsimilarity index 100%\nrename from t/helper/test-wildmatch.c\nrename to t/helper-src/test-wildmatch.c\ndiff --git a/t/helper/test-write-cache.c b/t/helper-src/test-write-cache.c\nsimilarity index 100%\nrename from t/helper/test-write-cache.c\nrename to t/helper-src/test-write-cache.c\ndiff --git a/t/helper/.gitignore b/t/helper/.gitignore\ndissimilarity index 100%\nindex 7c9d28a834..72e8ffc0db 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -1,38 +1 @@\n-/test-chmtime\n-/test-ctype\n-/test-config\n-/test-date\n-/test-delta\n-/test-dump-cache-tree\n-/test-dump-split-index\n-/test-dump-untracked-cache\n-/test-fake-ssh\n-/test-scrap-cache-tree\n-/test-genrandom\n-/test-hashmap\n-/test-index-version\n-/test-lazy-init-name-hash\n-/test-line-buffer\n-/test-match-trees\n-/test-mergesort\n-/test-mktemp\n-/test-online-cpus\n-/test-parse-options\n-/test-path-utils\n-/test-prio-queue\n-/test-read-cache\n-/test-ref-store\n-/test-regex\n-/test-revision-walking\n-/test-run-command\n-/test-sha1\n-/test-sha1-array\n-/test-sigchain\n-/test-strcmp-offset\n-/test-string-list\n-/test-submodule-config\n-/test-subprocess\n-/test-svn-fe\n-/test-urlmatch-normalization\n-/test-wildmatch\n-/test-write-cache\n+*\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334704","messageId":"xmqqbmj31zve.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171212190026.17671-1-sbeller@google.com","subject":"Re: [RFC PATCH] t/helper: Move sources to t/helper-src; gitignore any files in t/helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T19:59:33Z","receivedAt":"2017-12-12T19:59:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Compiled test helpers in t/helper are out of sync with the .gitignore\n> files quite frequently. This can happen when new test helpers are added,\n> but the explicit .gitignore file is not updated in the same commit, or\n> when you forget to 'make clean' before checking out a different version\n> of git, as the different version may have a different explicit list of\n> test helpers to ignore.\n>\n> This can be fixed by using overly broad ignore patterns, such as ignoring\n> the whole directory via '//t/helper/*' in .gitignore.\n\nIf we ignore everything but resurrect *.[ch] with negative exclude\nrules, can we do the same without moving things around?\n"},{"id":"334711","messageId":"20171212205606.75581-1-sbeller@google.com","threadId":"41856","inReplyTo":"xmqqbmj31zve.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] t/helper: ignore everything but sources","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-12T20:56:06Z","receivedAt":"2017-12-12T20:56:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Compiled test helpers in t/helper are out of sync with the .gitignore\nfiles quite frequently. This can happen when new test helpers are added,\nbut the explicit .gitignore file is not updated in the same commit, or\nwhen you forget to 'make clean' before checking out a different version\nof git, as the different version may have a different explicit list of\ntest helpers to ignore.\n\nFix this by having an overly broad ignore pattern in that directory:\nAnything, except C and shell source, will be ignored.\n\nSigned-off-by: Stefan Beller \n---\n\n> If we ignore everything but resurrect *.[ch] with negative exclude\n> rules, can we do the same without moving things around?\n\nYes, there is also one lonely shell script in there, which also needs\nexclusion.\n\n I guess we want to test this overly broad pattern in a sub directory,\n as I could imagine that at the top level directory we'd have more cases\n to think through (I used to put untracked files into the top level dir,\n but I do not do it anymore).\n \nThanks,\nStefan\n\n t/helper/.gitignore | 43 +++----------------------------------------\n 1 file changed, 3 insertions(+), 40 deletions(-)\n\ndiff --git a/t/helper/.gitignore b/t/helper/.gitignore\nindex d02f9b39ac..ee1e39fd08 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -1,40 +1,3 @@\n-/test-chmtime\n-/test-ctype\n-/test-config\n-/test-date\n-/test-delta\n-/test-drop-caches\n-/test-dump-cache-tree\n-/test-dump-fsmonitor\n-/test-dump-split-index\n-/test-dump-untracked-cache\n-/test-fake-ssh\n-/test-scrap-cache-tree\n-/test-genrandom\n-/test-hashmap\n-/test-index-version\n-/test-lazy-init-name-hash\n-/test-line-buffer\n-/test-match-trees\n-/test-mergesort\n-/test-mktemp\n-/test-online-cpus\n-/test-parse-options\n-/test-path-utils\n-/test-prio-queue\n-/test-read-cache\n-/test-ref-store\n-/test-regex\n-/test-revision-walking\n-/test-run-command\n-/test-sha1\n-/test-sha1-array\n-/test-sigchain\n-/test-strcmp-offset\n-/test-string-list\n-/test-submodule-config\n-/test-subprocess\n-/test-svn-fe\n-/test-urlmatch-normalization\n-/test-wildmatch\n-/test-write-cache\n+*\n+!.sh\n+!.[ch]\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334712","messageId":"xmqqr2rzzmf4.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171212205606.75581-1-sbeller@google.com","subject":"Re: [PATCH] t/helper: ignore everything but sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T21:06:07Z","receivedAt":"2017-12-12T21:06:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Yes, there is also one lonely shell script in there, which also needs\n> exclusion.\n\nThanks for catching them.\n\n> +*\n> +!.sh\n> +!.[ch]\n\nI'd use this instead, though.\n\n-- >8 --\n*\n!*.sh\n!*.[ch]\n!*.gitignore\n-- 8< --\n\nIn a dirty repository full of crufts but without any local\nmodifications, if you do\n\n $ git rm --cached -r t/helper\n $ git add t/helper\n\nyou should be able to make your index identical to HEAD. The\nversion that was posted did not resurrect .gitignore and none\nof the source files, but the replaced one should.\n"},{"id":"334713","messageId":"20171212210623.GV3693@zaya.teonanacatl.net","threadId":"41856","inReplyTo":"20171212205606.75581-1-sbeller@google.com","subject":"Re: [PATCH] t/helper: ignore everything but sources","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-12-12T21:06:23Z","receivedAt":"2017-12-12T21:06:33Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi Stefan,\n\nStefan Beller wrote:\n>> If we ignore everything but resurrect *.[ch] with negative exclude\n>> rules, can we do the same without moving things around?\n> \n> Yes, there is also one lonely shell script in there, which also needs\n> exclusion.\n\nThere aren't currently any .h files, but I suppose it doesn't hurt to\ninclude that pattern to be safer for the future.\n\n> +*\n> +!.sh\n> +!.[ch]\n\nThe ! patterns are missing a '*'. I think it should be:\n\n*\n!*.[ch]\n!*.sh\n\nDoes it make sense to also include !.gitignore as well?\nIt's already committed, so it's not ignored. But perhaps\nhaving it listed will save someone from getting their repo\ninto a state where local changes to .gitignore aren't picked\nup (I know that's a bit of a stretch).\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nHow much does it cost to entice a dope-smoking UNIX system guru to\nDayton?\n -- Brian Boyle, UNIX/WORLD's First Annual Salary Survey\n\n"},{"id":"334776","messageId":"20171213201242.115601-1-sbeller@google.com","threadId":"41856","inReplyTo":"xmqqr2rzzmf4.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] t/helper: ignore everything but sources","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-13T20:12:42Z","receivedAt":"2017-12-13T20:12:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Compiled test helpers in t/helper are out of sync with the .gitignore\nfiles quite frequently. This can happen when new test helpers are added,\nbut the explicit .gitignore file is not updated in the same commit, or\nwhen you forget to 'make clean' before checking out a different version\nof git, as the different version may have a different explicit list of\ntest helpers to ignore.\n\nFix this by having an overly broad ignore pattern in that directory:\nAnything, except C and shell source, will be ignored.\n\nSigned-off-by: Stefan Beller \n---\n\nThanks Todd and Junio for mentioning the .gitignore file.\n\n\n t/helper/.gitignore | 44 ++++----------------------------------------\n 1 file changed, 4 insertions(+), 40 deletions(-)\n\ndiff --git a/t/helper/.gitignore b/t/helper/.gitignore\nindex d02f9b39ac..5b540625af 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -1,40 +1,4 @@\n-/test-chmtime\n-/test-ctype\n-/test-config\n-/test-date\n-/test-delta\n-/test-drop-caches\n-/test-dump-cache-tree\n-/test-dump-fsmonitor\n-/test-dump-split-index\n-/test-dump-untracked-cache\n-/test-fake-ssh\n-/test-scrap-cache-tree\n-/test-genrandom\n-/test-hashmap\n-/test-index-version\n-/test-lazy-init-name-hash\n-/test-line-buffer\n-/test-match-trees\n-/test-mergesort\n-/test-mktemp\n-/test-online-cpus\n-/test-parse-options\n-/test-path-utils\n-/test-prio-queue\n-/test-read-cache\n-/test-ref-store\n-/test-regex\n-/test-revision-walking\n-/test-run-command\n-/test-sha1\n-/test-sha1-array\n-/test-sigchain\n-/test-strcmp-offset\n-/test-string-list\n-/test-submodule-config\n-/test-subprocess\n-/test-svn-fe\n-/test-urlmatch-normalization\n-/test-wildmatch\n-/test-write-cache\n+*\n+!*.[ch]\n+!*.sh\n+!.gitignore\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"335046","messageId":"xmqqo9mutlgy.fsf@gitster.mtv.corp.google.com","threadId":"41856","inReplyTo":"20171126193813.12531-1-max@max630.net","subject":"Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-19T22:13:33Z","receivedAt":"2017-12-19T22:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> v6:\n>\n> Do not implement generic git_env_ssize_t(), instead export git_parse_ssize_t() from config.c\n> and hardcode the rest.\n>\n> Florian Manschwetus (1):\n> http-backend: respect CONTENT_LENGTH as specified by rfc3875\n>\n> Max Kirillov (1):\n> t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases\n>\n> Makefile | 1 +\n> config.c | 2 +-\n> config.h | 1 +\n> http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++-\n> t/helper/test-print-values.c | 10 ++++++++\n> t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++\n> 6 files changed, 92 insertions(+), 2 deletions(-)\n> create mode 100644 t/helper/test-print-values.c\n\nSo... is there going to be an update (or has there been one and I\nmissed it)?\n\nThanks.\n"},{"id":"335063","messageId":"20171220043021.GA20508@jessie.local","threadId":"41856","inReplyTo":"xmqqo9mutlgy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-12-20T04:30:21Z","receivedAt":"2017-12-20T04:36:45Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Dec 19, 2017 at 02:13:33PM -0800, Junio C Hamano wrote:\n> So... is there going to be an update (or has there been one and I\n> missed it)?\n\nYes there it! I wanted to add tests for the cases Jeff\nmentioned. It is almost done, I just need to check I did not\nmiss some note.\n"}]}