[PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Florian Manschwetus]

-----Ursprüngliche Nachricht-----
Von: Chris Packham [mailto:judge.packham@gmail.com] 
Gesendet: Dienstag, 29. März 2016 11:28
An: Florian Manschwetus
Cc: Konstantin Khomoutov; git@vger.kernel.org
Betreff: Re: Problem with git-http-backend.exe as iis cgi

Hi Florian

On Tue, Mar 29, 2016 at 7:01 PM, Florian Manschwetus <manschwetus@cs-software-gmbh.de> wrote:
> Hi,
> I put together a first patch for the issue.
>
> Mit freundlichen Grüßen / With kind regards Florian Manschwetus
>
> E-Mail: manschwetus@cs-software-gmbh.de
> Tel.: +49-(0)611-8908534
>
> CS Software Concepts and Solutions GmbH Geschäftsführer / Managing 
> director: Dr. Werner Alexi Amtsgericht Wiesbaden HRB 10004 (Commercial 
> registry) Schiersteiner Straße 31
> D-65187 Wiesbaden
> Germany
> Tel.: 0611/8908555
>
>
> -----Ursprüngliche Nachricht-----
> Von: Konstantin Khomoutov [mailto:kostix+git@007spb.ru]
> Gesendet: Donnerstag, 10. März 2016 13:55
> An: Florian Manschwetus
> Cc: git@vger.kernel.org
> Betreff: Re: Problem with git-http-backend.exe as iis cgi
>
> On Thu, 10 Mar 2016 07:28:50 +0000
> Florian Manschwetus <manschwetus@cs-software-gmbh.de> wrote:
>
>> I tried to setup git-http-backend with iis, as iis provides proper 
>> impersonation for cgi under windows, which leads to have the 
>> filesystem access performed with the logon user, therefore the 
>> webserver doesn't need generic access to the files. I stumbled across 
>> a problem, ending up with post requests hanging forever. After some 
>> investigation I managed to get it work by wrapping the http-backend 
>> into a bash script, giving a lot of control about the environmental 
>> things, I was unable to solve within IIS configuration. The 
>> workaround, I use currently, is to use "/bin/head -c 
>> ${CONTENT_LENGTH}
>> | ./git-http-backend.exe", which directly shows the issue. Git
>> http-backend should check if CONTENT_LENGTH is set to something 
>> reasonable (e.g. >0) and should in this case read only CONTENT_LENGTH 
>> bytes from stdin, instead of reading till EOF what I suspect it is 
>> doing currently.
>
> The rfc [1] states in its section 4.2:
>
> | A request-body is supplied with the request if the CONTENT_LENGTH is 
> | not NULL.  The server MUST make at least that many bytes available 
> | for the script to read.  The server MAY signal an end-of-file 
> | condition after CONTENT_LENGTH bytes have been read or it MAY supply 
> | extension data.  Therefore, the script MUST NOT attempt to read more 
> | than CONTENT_LENGTH bytes, even if more data is available.  However, 
> | it is not obliged to read any of the data.
>
> So yes, if Git currently reads until EOF, it's an error.
> The correct way would be:
>
> 1) Check to see if the CONTENT_LENGTH variable is available in the
>    environment.  If no, read nothing.
>
> 2) Otherwise read as many bytes it specifies, and no more.
>
> 1. https://www.ietf.org/rfc/rfc3875

Your patch description seems well thought out but if you want someone to notice it you should have a read of https://git.kernel.org/cgit/git/git.git/tree/Documentation/SubmittingPatches

Moin,
I have cloned git and created a more clean patch...

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
---
 http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..94976df 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)
  */
 static ssize_t read_request(int fd, unsigned char **out)
 {
-	size_t len = 0, alloc = 8192;
-	unsigned char *buf = xmalloc(alloc);
+	unsigned char *buf = null;
+	size_t len = 0;
+	/* get request size */
+	size_t req_len = git_env_ulong("CONTENT_LENGTH",
+					   0);
+
+	/* check request size */
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu);"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer);
+	}
+
+	if (req_len <= 0) {
+		*out = null;
+		return 0;
+	}
+
+	/* allocate buffer */
+	buf = xmalloc(req_len)
 
-	if (max_request_buffer < alloc)
-		max_request_buffer = alloc;
 
 	while (1) {
 		ssize_t cnt;
 
-		cnt = read_in_full(fd, buf + len, alloc - len);
+		cnt = read_in_full(fd, buf + len, req_len - len);
 		if (cnt < 0) {
 			free(buf);
 			return -1;
@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)
 
 		/* partial read from read_in_full means we hit EOF */
 		len += cnt;
-		if (len < alloc) {
+		if (len < req_len) {
+			/* TODO request incomplete?? */
+			/* maybe just remove this block and condition along with the loop, */
+			/* if read_in_full is prooven reliable */
 			*out = buf;
 			return len;
+		} else {
+			/* request complete */
+			*out = buf;
+			return len;
+			
 		}
-
-		/* otherwise, grow and try again (if we can) */
-		if (alloc == max_request_buffer)
-			die("request was larger than our maximum size (%lu);"
-			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
-			    max_request_buffer);
-
-		alloc = alloc_nr(alloc);
-		if (alloc > max_request_buffer)
-			alloc = max_request_buffer;
-		REALLOC_ARRAY(buf, alloc);
 	}
 }
 
@@ -701,3 +714,4 @@ int main(int argc, char **argv)
 	cmd->imp(cmd_arg);
 	return 0;
 }
+
-- 
2.7.2.windows.1


Mit freundlichen Grüßen / With kind regards
Florian Manschwetus

CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi 
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany

Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Jeff King]

On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:

> > | A request-body is supplied with the request if the CONTENT_LENGTH is 
> > | not NULL.  The server MUST make at least that many bytes available 
> > | for the script to read.  The server MAY signal an end-of-file 
> > | condition after CONTENT_LENGTH bytes have been read or it MAY supply 
> > | extension data.  Therefore, the script MUST NOT attempt to read more 
> > | than CONTENT_LENGTH bytes, even if more data is available.  However, 
> > | it is not obliged to read any of the data.
> >
> > So yes, if Git currently reads until EOF, it's an error.
> > The correct way would be:
> >
> > 1) Check to see if the CONTENT_LENGTH variable is available in the
> >    environment.  If no, read nothing.
> >
> > 2) Otherwise read as many bytes it specifies, and no more.
> >
> > 1. https://www.ietf.org/rfc/rfc3875

I don't think the second part of (1) will work very well if the client
sends a chunked transfer-encoding (which git will do if the input is large). In
such a case the server would either have to buffer the entire input to
find its length, or stream the data to the CGI without setting
$CONTENT_LENGTH. At least some servers choose the latter (including
Apache).

> diff --git a/http-backend.c b/http-backend.c
> index 8870a26..94976df 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)
>   */
>  static ssize_t read_request(int fd, unsigned char **out)
>  {
> -	size_t len = 0, alloc = 8192;
> -	unsigned char *buf = xmalloc(alloc);
> +	unsigned char *buf = null;
> +	size_t len = 0;
> +	/* get request size */
> +	size_t req_len = git_env_ulong("CONTENT_LENGTH",
> +					   0);
> +
> +	/* check request size */
> +	if (max_request_buffer < req_len) {
> +		die("request was larger than our maximum size (%lu);"
> +			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +			    max_request_buffer);
> +	}
> +
> +	if (req_len <= 0) {
> +		*out = null;
> +		return 0;
> +	}

git-am complained that your patch did not apply, but after writing
something similar locally, I found that t5551.25 hangs indefinitely.
Which is not surprising. Most tests are doing very limited ref
negotiation, so the content that hits read_request() here is small, and
we send it in a single write with a content-length header. But t5551.25
uses a much bigger workload, which causes the client to use a chunked
transfer-encoding, and this code to refuse to read anything (and then
the protocol stalls, as we are waiting for the client to say something).

So I think you'd want to take a missing CONTENT_LENGTH as a hint to read
until EOF.

That also raises another issue: what happens in the paths that don't hit
read_request()? We may also process input via:

  - inflate_request(), if the client gzipped it; for well-formed input,
    I think we'll stop reading when the gzip stream tells us there is no
    more data, but a malformed one would have us reading until EOF,
    regardless of what $CONTENT_LENGTH says.

  - for input which we expect to be large (like incoming packfiles for a
    push), buffer_input will be unset, and we will pass the descriptor
    directly to a sub-program like git-index-pack. Again, for
    well-formed input it would read just the packfile, but it may
    actually continue to EOF.

So I don't think your patch is covering all cases.

-Peff

AW: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Florian Manschwetus]

> -----Ursprüngliche Nachricht-----
> Von: Jeff King [mailto:peff@peff.net]
> Gesendet: Dienstag, 29. März 2016 22:14
> An: Florian Manschwetus
> Cc: Chris Packham; Konstantin Khomoutov; git@vger.kernel.org
> Betreff: Re: [PATCH] Fix http-backend reading till EOF, ignoring
> CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-
> backend.exe as iis cgi
> 
> On Tue, Mar 29, 2016 at 10:38:23AM +0000, Florian Manschwetus wrote:
> 
> > > | A request-body is supplied with the request if the CONTENT_LENGTH
> > > | is not NULL.  The server MUST make at least that many bytes
> > > | available for the script to read.  The server MAY signal an
> > > | end-of-file condition after CONTENT_LENGTH bytes have been read or
> > > | it MAY supply extension data.  Therefore, the script MUST NOT
> > > | attempt to read more than CONTENT_LENGTH bytes, even if more data
> > > | is available.  However, it is not obliged to read any of the data.
> > >
> > > So yes, if Git currently reads until EOF, it's an error.
> > > The correct way would be:
> > >
> > > 1) Check to see if the CONTENT_LENGTH variable is available in the
> > >    environment.  If no, read nothing.
> > >
> > > 2) Otherwise read as many bytes it specifies, and no more.
> > >
> > > 1. https://www.ietf.org/rfc/rfc3875
> 
> I don't think the second part of (1) will work very well if the client sends a
> chunked transfer-encoding (which git will do if the input is large). In such a
> case the server would either have to buffer the entire input to find its length,
> or stream the data to the CGI without setting $CONTENT_LENGTH. At least
> some servers choose the latter (including Apache).
> 
> > diff --git a/http-backend.c b/http-backend.c index 8870a26..94976df
> > 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const
> char *name)
> >   */
> >  static ssize_t read_request(int fd, unsigned char **out)  {
> > -	size_t len = 0, alloc = 8192;
> > -	unsigned char *buf = xmalloc(alloc);
> > +	unsigned char *buf = null;
> > ...
> 
> git-am complained that your patch did not apply, but after writing something
> similar locally, I found that t5551.25 hangs indefinitely.
> Which is not surprising. Most tests are doing very limited ref negotiation, so
> the content that hits read_request() here is small, and we send it in a single
> write with a content-length header. But t5551.25 uses a much bigger
> workload, which causes the client to use a chunked transfer-encoding, and
> this code to refuse to read anything (and then the protocol stalls, as we are
> waiting for the client to say something).
> 
> So I think you'd want to take a missing CONTENT_LENGTH as a hint to read
> until EOF.
> 
> That also raises another issue: what happens in the paths that don't hit
> read_request()? We may also process input via:
> 
>   - inflate_request(), if the client gzipped it; for well-formed input,
>     I think we'll stop reading when the gzip stream tells us there is no
>     more data, but a malformed one would have us reading until EOF,
>     regardless of what $CONTENT_LENGTH says.
> 
>   - for input which we expect to be large (like incoming packfiles for a
>     push), buffer_input will be unset, and we will pass the descriptor
>     directly to a sub-program like git-index-pack. Again, for
>     well-formed input it would read just the packfile, but it may
>     actually continue to EOF.
> 
> So I don't think your patch is covering all cases.
> 
> -Peff

After additional analysis it turned out, that in the case you mentioned, at least IIS, sets CONTENT_LENGTH to -1 resulting in the current behavior of git-http-backend being sufficient in this situation.
Therefore I refactored the code again a bit, to match up the behavior I currently fake by using some bash magic...

From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
From: manschwetus <manschwetus@cs-software-gmbh.de>
Date: Tue, 29 Mar 2016 12:16:21 +0200
Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
 CONTENT_LENGTH, violating rfc3875

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
---
 http-backend.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..94976df 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char *name)
  */
 static ssize_t read_request(int fd, unsigned char **out)
 {
-	size_t len = 0, alloc = 8192;
-	unsigned char *buf = xmalloc(alloc);
+	unsigned char *buf = null;
+	size_t len = 0;
+	/* get request size */
+	size_t req_len = git_env_ulong("CONTENT_LENGTH",
+					   0);
+
+	/* check request size */
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu);"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer);
+	}
+
+	if (req_len <= 0) {
+		*out = null;
+		return 0;
+	}
+
+	/* allocate buffer */
+	buf = xmalloc(req_len)
 
-	if (max_request_buffer < alloc)
-		max_request_buffer = alloc;
 
 	while (1) {
 		ssize_t cnt;
 
-		cnt = read_in_full(fd, buf + len, alloc - len);
+		cnt = read_in_full(fd, buf + len, req_len - len);
 		if (cnt < 0) {
 			free(buf);
 			return -1;
@@ -294,21 +310,18 @@ static ssize_t read_request(int fd, unsigned char **out)
 
 		/* partial read from read_in_full means we hit EOF */
 		len += cnt;
-		if (len < alloc) {
+		if (len < req_len) {
+			/* TODO request incomplete?? */
+			/* maybe just remove this block and condition along with the loop, */
+			/* if read_in_full is prooven reliable */
 			*out = buf;
 			return len;
+		} else {
+			/* request complete */
+			*out = buf;
+			return len;
+			
 		}
-
-		/* otherwise, grow and try again (if we can) */
-		if (alloc == max_request_buffer)
-			die("request was larger than our maximum size (%lu);"
-			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
-			    max_request_buffer);
-
-		alloc = alloc_nr(alloc);
-		if (alloc > max_request_buffer)
-			alloc = max_request_buffer;
-		REALLOC_ARRAY(buf, alloc);
 	}
 }
 
-- 
2.7.2.windows.1


From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Date: Wed, 30 Mar 2016 10:54:21 +0200
Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved
 new variant to read_request_fix_len(...) and introduced read_request(...) as
 wrapper, which decides based on value retrieved from CONTENT_LENGTH which
 variant to use

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
---
 http-backend.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 94976df..3aa0446 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -275,13 +275,52 @@ static struct rpc_service *select_service(const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
+{
+        size_t len = 0, alloc = 8192;
+        unsigned char *buf = xmalloc(alloc);
+
+        if (max_request_buffer < alloc)
+                max_request_buffer = alloc;
+
+        while (1) {
+                ssize_t cnt;
+
+                cnt = read_in_full(fd, buf + len, alloc - len);
+                if (cnt < 0) {
+                        free(buf);
+                        return -1;
+                }
+
+                /* partial read from read_in_full means we hit EOF */
+                len += cnt;
+                if (len < alloc) {
+                        *out = buf;
+                        return len;
+                }
+
+                /* otherwise, grow and try again (if we can) */
+                if (alloc == max_request_buffer)
+                        die("request was larger than our maximum size (%lu);"
+                            " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+                            max_request_buffer);
+
+                alloc = alloc_nr(alloc);
+                if (alloc > max_request_buffer)
+                        alloc = max_request_buffer;
+                REALLOC_ARRAY(buf, alloc);
+        }
+}
+
+/*
+ * replacement for original read_request, now renamed to read_request_eof,
+ * honoring given content_length (req_len),
+ * provided by new wrapper function read_request
+ */
+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
 {
 	unsigned char *buf = null;
 	size_t len = 0;
-	/* get request size */
-	size_t req_len = git_env_ulong("CONTENT_LENGTH",
-					   0);
 
 	/* check request size */
 	if (max_request_buffer < req_len) {
@@ -325,6 +364,26 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
+ * to
+ * - use old behaviour of read_request, to read until EOF
+ * => read_request_eof(...)
+ * - just read CONTENT_LENGTH-bytes, when provided
+ * => read_request_fix_len(...)
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+        /* get request size */
+        size_t req_len = git_env_ulong("CONTENT_LENGTH",
+                                           -1);
+        if (req_len < 0){
+          read_request_eof(fd, out);
+        } else {
+          read_request_fix_len(fd, req_len, out);
+        }
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.7.2.windows.1


Mit freundlichen Grüßen / With kind regards
Florian Manschwetus
 
CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi 
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany

Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Jeff King]

On Wed, Mar 30, 2016 at 09:08:56AM +0000, Florian Manschwetus wrote:

> After additional analysis it turned out, that in the case you
> mentioned, at least IIS, sets CONTENT_LENGTH to -1 resulting in the
> current behavior of git-http-backend being sufficient in this
> situation.
> Therefore I refactored the code again a bit, to match up the behavior
> I currently fake by using some bash magic...

OK, so I'd agree it makes sense to catch "-1", and read to EOF in that
case (or if CONTENT_LENGTH is NULL).

> From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
> From: manschwetus <manschwetus@cs-software-gmbh.de>
> Date: Tue, 29 Mar 2016 12:16:21 +0200
> Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
>  CONTENT_LENGTH, violating rfc3875

Please send one patch per email, and these header bits should be the
header of your email.

Though we also generally revise and re-send patches, rather than
presenting one patch that has problems and then tacking fixes on top.
I'll ignore the problems in this patch 1, as it looks like it's just the
original one repeated.

> From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
> From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Date: Wed, 30 Mar 2016 10:54:21 +0200
> Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved
>  new variant to read_request_fix_len(...) and introduced read_request(...) as
>  wrapper, which decides based on value retrieved from CONTENT_LENGTH which
>  variant to use

Please use a short subject for your commit message, followed by a blank
line and then a more explanatory body. Also, don't just describe _what_
is happening (we can see that from the diff), but _why_. You can find
more similar tips in SubmittingPatches.

> +/**
> + * wrapper function, whcih determines based on CONTENT_LENGTH value,
> + * to
> + * - use old behaviour of read_request, to read until EOF
> + * => read_request_eof(...)
> + * - just read CONTENT_LENGTH-bytes, when provided
> + * => read_request_fix_len(...)
> + */
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +        /* get request size */
> +        size_t req_len = git_env_ulong("CONTENT_LENGTH",
> +                                           -1);
> +        if (req_len < 0){
> +          read_request_eof(fd, out);
> +        } else {
> +          read_request_fix_len(fd, req_len, out);
> +        }
> +}

I don't think "if (req_len < 0)" can ever trigger, because size_t is an
unsigned type (and I do not recall what kind of integer overflow
validation we do in git_env_ulong, but I suspect it may complain about
"-1"). You may have to parse the variable manually rather than using
git_env_ulong (i.e., pick out the NULL and "-1" cases, and then feed the
rest to git_parse_ulong()).

Also, a few style nits. We usually omit braces for one-line
conditionals, and please make sure there is whitespace between the
closing parenthesis and the opening brace. There's some discussing in
CodingGuidelines.

-Peff

[PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. This causes hang under IIS/Windows, for example.

Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the varibale is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Authored-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Fixed-by: Max Kirillov <max@max630.net>
Signed-off-by: Max Kirillov <max@max630.net>
---
Hi

I came across this issue, and I think is should be good to restore the patch.
It is basically same but I squashed them, fixed the thing you mentioned and
also some trivial build failures (null -> NULL and missing return from the wrapper).
I hope I marked it correctly in the trailers.
 config.c       |  8 +++++++
 config.h       |  1 +
 http-backend.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
 	return val;
 }
 
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+	const char *v = getenv(k);
+	if (v && !git_parse_ssize_t(v, &val))
+		die("failed to parse %s", k);
+	return val;
+}
+
 int git_config_system(void)
 {
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
 extern const char *git_etc_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
+extern ssize_t git_env_ssize_t(const char *, ssize_t);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..317b99b87c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
 	size_t len = 0, alloc = 8192;
 	unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+/*
+ * replacement for original read_request, now renamed to read_request_eof,
+ * honoring given content_length (req_len),
+ * provided by new wrapper function read_request
+ */
+static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
+{
+	unsigned char *buf = NULL;
+	size_t len = 0;
+
+	/* check request size */
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu);"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer);
+	}
+
+	if (req_len <= 0) {
+		*out = NULL;
+		return 0;
+	}
+
+	/* allocate buffer */
+	buf = xmalloc(req_len);
+
+
+	while (1) {
+		ssize_t cnt;
+
+		cnt = read_in_full(fd, buf + len, req_len - len);
+		if (cnt < 0) {
+			free(buf);
+			return -1;
+		}
+
+		/* partial read from read_in_full means we hit EOF */
+		len += cnt;
+		if (len < req_len) {
+			/* TODO request incomplete?? */
+			/* maybe just remove this block and condition along with the loop, */
+			/* if read_in_full is prooven reliable */
+			*out = buf;
+			return len;
+		} else {
+			/* request complete */
+			*out = buf;
+			return len;
+			
+		}
+	}
+}
+
+/**
+ * wrapper function, whcih determines based on CONTENT_LENGTH value,
+ * to
+ * - use old behaviour of read_request, to read until EOF
+ * => read_request_eof(...)
+ * - just read CONTENT_LENGTH-bytes, when provided
+ * => read_request_fix_len(...)
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	/* get request size */
+	ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+	if (req_len < 0)
+		return read_request_eof(fd, out);
+	else
+		return read_request_fix_len(fd, req_len, out);
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.11.0.1122.gc3fec58.dirty

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Thu, Nov 23, 2017 at 6:45 PM, Max Kirillov <max@max630.net> wrote:
> [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

The "RFC" seems to be missing from the subject line of this unpolished patch.

> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. This causes hang under IIS/Windows, for example.

By "_this_ causes a hang", I presume you mean "not respecting
CONTENT_LENGTH causes a hang"? Perhaps that could be spelled out
explicitly.

> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the varibale is not defined, keep older behavior

s/varibale/variable/

> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Authored-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Fixed-by: Max Kirillov <max@max630.net>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,76 @@ static ssize_t read_request(int fd, unsigned char **out)
> +/*
> + * replacement for original read_request, now renamed to read_request_eof,
> + * honoring given content_length (req_len),
> + * provided by new wrapper function read_request
> + */

This comment has value only to someone who knew what the code was like
before this change, and it merely repeats what is already implied by
the commit message, rather than providing any valuable information
about this new function itself. Therefore, it should be dropped.

> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)

Wrong data type: s/size_t req_len/ssize_t req_len/

Also: s/fix/fixed/

> +{
> +       unsigned char *buf = NULL;
> +       size_t len = 0;
> +
> +       /* check request size */

Comment merely repeats what code says, thus has no value. Please drop.

> +       if (max_request_buffer < req_len) {
> +               die("request was larger than our maximum size (%lu);"
> +                           " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +                           max_request_buffer);

This error message neglects to say what the request size was. Such
information would be useful given that it suggests bumping
GIT_HTTP_MAX_REQUEST_BUFFER to a larger value.

> +       }
> +
> +       if (req_len <= 0) {
> +               *out = NULL;
> +               return 0;
> +       }
> +
> +       /* allocate buffer */

Drop valueless comment.

> +       buf = xmalloc(req_len);
> +
> +

Style: Too many blank lines.

> +       while (1) {
> +               ssize_t cnt;
> +
> +               cnt = read_in_full(fd, buf + len, req_len - len);
> +               if (cnt < 0) {
> +                       free(buf);
> +                       return -1;
> +               }
> +
> +               /* partial read from read_in_full means we hit EOF */
> +               len += cnt;
> +               if (len < req_len) {
> +                       /* TODO request incomplete?? */
> +                       /* maybe just remove this block and condition along with the loop, */
> +                       /* if read_in_full is prooven reliable */

s/prooven/proven/

> +                       *out = buf;
> +                       return len;
> +               } else {
> +                       /* request complete */
> +                       *out = buf;
> +                       return len;
> +
> +               }
> +       }

What is the purpose of the while(1) loop? Every code path inside the
loop returns, so it will never execute more than once. Likewise, why
is 'len' needed?

Rather than writing an entirely new "read" function, how about just
modifying the existing read_request() to optionally limit the read to
a specified number of bytes?

> +}
> +
> +/**
> + * wrapper function, whcih determines based on CONTENT_LENGTH value,

s/whcih/which/

Also, the placement of commas needs some attention.

> + * to
> + * - use old behaviour of read_request, to read until EOF
> + * => read_request_eof(...)
> + * - just read CONTENT_LENGTH-bytes, when provided
> + * => read_request_fix_len(...)
> + */

When talking about "old behavior", this comment is repeating
information more suitable to the commit message (and effectively
already covered there); information which only has value to someone
who knew what the old code/behavior was like. The rest of this comment
is merely repeating what the code itself already says, thus adds no
value, so should be dropped.

> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +       /* get request size */

Drop valueless comment.

> +       ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
> +       if (req_len < 0)
> +               return read_request_eof(fd, out);
> +       else
> +               return read_request_fix_len(fd, req_len, out);
> +}

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. This causes hang under IIS/Windows, for example.
>
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the varibale is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Authored-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> Fixed-by: Max Kirillov <max@max630.net>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> ...
> I hope I marked it correctly in the trailers.

It is probably more conventional to do it like so:

    From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
    Date: <original date of Florian's patch series>

    http-backend reads whole input until EOF. However, the RFC 3875...
    ... chunked transfer-encoding.

    Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
    [mk: fixed trivial build failures and stuff]
    Signed-off-by: Max Kirillov <max@max630.net>
    ---

>  
> +/*
> + * replacement for original read_request, now renamed to read_request_eof,
> + * honoring given content_length (req_len),
> + * provided by new wrapper function read_request
> + */

I agree with Eric's suggestion.  In-code comment is read by those
who have the current code, without knowing/caring what it used to
be.  "It used to do this, but replace it with this new thing
because..." is a valuable thing to record in the log message, but
not here.

AW: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Florian Manschwetus]

Hi All,
thx Max for jumping in, I wasn't able to complete this due to serious lack of time.
Later I forgot it. Great to see that this finally made it.

Mit freundlichen Grüßen / With kind regards
Florian Manschwetus

E-Mail: manschwetus@cs-software-gmbh.de
Tel.: +49-(0)611-8908534
 
CS Software Concepts and Solutions GmbH
Geschäftsführer / Managing director: Dr. Werner Alexi 
Amtsgericht Wiesbaden HRB 10004 (Commercial registry)
Schiersteiner Straße 31
D-65187 Wiesbaden
Germany
Tel.: 0611/8908555

> -----Ursprüngliche Nachricht-----
> Von: Junio C Hamano [mailto:gitster@pobox.com]
> Gesendet: Freitag, 24. November 2017 06:55
> An: Max Kirillov
> Cc: Jeff King; Florian Manschwetus; Chris Packham; Konstantin Khomoutov;
> git@vger.kernel.org
> Betreff: Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified
> by rfc3875
> 
> Max Kirillov <max@max630.net> writes:
> 
> > http-backend reads whole input until EOF. However, the RFC 3875
> > specifies that a script must read only as many bytes as specified by
> > CONTENT_LENGTH environment variable. This causes hang under
> IIS/Windows, for example.
> >
> > Make http-backend read only CONTENT_LENGTH bytes, if it's defined,
> > rather than the whole input until EOF. If the varibale is not defined,
> > keep older behavior of reading until EOF because it is used to support
> chunked transfer-encoding.
> >
> > Signed-off-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
> > Authored-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
> > Fixed-by: Max Kirillov <max@max630.net>
> > Signed-off-by: Max Kirillov <max@max630.net>
> > ---
> > ...
> > I hope I marked it correctly in the trailers.
> 
> It is probably more conventional to do it like so:
> 
>     From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
>     Date: <original date of Florian's patch series>
> 
>     http-backend reads whole input until EOF. However, the RFC 3875...
>     ... chunked transfer-encoding.
> 
>     Signed-off-by: Florian Manschwetus <manschwetus@cs-software-
> gmbh.de>
>     [mk: fixed trivial build failures and stuff]
>     Signed-off-by: Max Kirillov <max@max630.net>
>     ---
> 
> >
> > +/*
> > + * replacement for original read_request, now renamed to
> > +read_request_eof,
> > + * honoring given content_length (req_len),
> > + * provided by new wrapper function read_request  */
> 
> I agree with Eric's suggestion.  In-code comment is read by those who have
> the current code, without knowing/caring what it used to be.  "It used to do
> this, but replace it with this new thing because..." is a valuable thing to record
> in the log message, but not here.

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

Thanks for the review. I saw only reaction of the Jeff in
the original thread and though that it is ok otherwise. I'm
fixing the things you mentioned.

On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
> 
> Wrong data type: s/size_t req_len/ssize_t req_len/

Passing negative value to the function makes no sense. I
could add explicit type cast to make it clear. It should be
safe as site_t's range is bigger, and overflown
CONTENT_LENGTH results in die() at parsing (I have a test
which verifies it)

> Rather than writing an entirely new "read" function, how about just
> modifying the existing read_request() to optionally limit the read to
> a specified number of bytes?

I'll check it a bit separately.

-- 
Max

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov <max@max630.net> wrote:
> Thanks for the review. I saw only reaction of the Jeff in
> the original thread and though that it is ok otherwise. I'm
> fixing the things you mentioned.

The commentary (in which you talked about restoring the patch and
squashing) seemed to imply that this had been posted somewhere before,
but it wasn't marked as "v2" (or whatever attempt) and lacked a URL
pointing at the previous attempt, so it was difficult to judge.

> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
>>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
>>
>> Wrong data type: s/size_t req_len/ssize_t req_len/
>
> Passing negative value to the function makes no sense. I
> could add explicit type cast to make it clear. It should be
> safe as site_t's range is bigger, and overflown
> CONTENT_LENGTH results in die() at parsing (I have a test
> which verifies it)

A concern with requesting size_t bytes is that, if it does read all
bytes, that value can't necessarily be represented by the ssize_t
returned from the function. Where would the cast be placed that you
suggest? How do other git functions deal with this sort of situation?

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sat, Nov 25, 2017 at 07:38:33PM -0500, Eric Sunshine wrote:
> On Sat, Nov 25, 2017 at 4:47 PM, Max Kirillov <max@max630.net> wrote:
>> On Thu, Nov 23, 2017 at 08:30:39PM -0500, Eric Sunshine wrote:
>>>> +static ssize_t read_request_fix_len(int fd, size_t req_len, unsigned char **out)
>>>
>>> Wrong data type: s/size_t req_len/ssize_t req_len/
>>
>> Passing negative value to the function makes no sense. I
>> could add explicit type cast to make it clear. It should be
>> safe as site_t's range is bigger, and overflown
>> CONTENT_LENGTH results in die() at parsing (I have a test
>> which verifies it)
> 
> A concern with requesting size_t bytes is that, if it does read all
> bytes, that value can't necessarily be represented by the ssize_t
> returned from the function. Where would the cast be placed that you
> suggest? How do other git functions deal with this sort of situation?

Right... ok, let it be ssize_t

[PATCH v4 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

* polished style
* add tests
* marked the authorship more correctly

Max Kirillov (2):
  http-backend: respect CONTENT_LENGTH as specified by rfc3875
  t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

 Makefile                         |  1 +
 config.c                         |  8 ++++++++
 config.h                         |  1 +
 http-backend.c                   | 39 ++++++++++++++++++++++++++++++++++++++-
 t/helper/test-print-values.c     | 10 ++++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
 6 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-print-values.c

-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v4 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.

Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
[mk: fixed trivial build failures and polished style issues]

Signed-off-by: Max Kirillov <max@max630.net>
---
 config.c       |  8 ++++++++
 config.h       |  1 +
 http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
 	return val;
 }
 
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+	const char *v = getenv(k);
+	if (v && !git_parse_ssize_t(v, &val))
+		die("failed to parse %s", k);
+	return val;
+}
+
 int git_config_system(void)
 {
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
 extern const char *git_etc_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
+extern ssize_t git_env_ssize_t(const char *, ssize_t);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..b4ba83b624 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
 	size_t len = 0, alloc = 8192;
 	unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
+{
+	unsigned char *buf = NULL;
+	ssize_t cnt = 0;
+
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu): %lu;"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer,
+			    req_len);
+	}
+
+	if (req_len <= 0) {
+		*out = NULL;
+		return 0;
+	}
+
+	buf = xmalloc(req_len);
+	cnt = read_in_full(fd, buf, req_len);
+	if (cnt < 0) {
+		free(buf);
+		return -1;
+	} else {
+		*out = buf;
+		return cnt;
+	}
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+	if (req_len < 0)
+		return read_request_eof(fd, out);
+	else
+		return read_request_fixed_len(fd, req_len, out);
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v4 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

Add tests for cases:

* CONTENT_LENGTH is set, script's stdin has more data.
  (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
  and fail. It does not seem to cause any performance issues with the default
  value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

Signed-off-by: Max Kirillov <max@max630.net>
---
 Makefile                         |  1 +
 t/helper/test-print-values.c     | 10 ++++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 t/helper/test-print-values.c

diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 0000000000..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <string.h>
+
+int cmd_main(int argc, const char **argv)
+{
+	if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+		printf("%zu", (ssize_t)(-20));
+
+	return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
 	expect_aliased 1 //domain/data.txt
 '
 
+# overrides existing definition for further cases
+run_backend() {
+	CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+	( echo "$2" && cat /dev/zero ) |
+	QUERY_STRING="${1#*[?]}" \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+	git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+	config http.uploadpack true &&
+	GET info/refs?service=git-upload-pack "200 OK"	&&
+	! grep "fatal:.*" act.err &&
+	POST git-upload-pack 0000 "200 OK" &&
+	! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+	NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" "(size_t)(-20)"` &&
+	env \
+		CONTENT_TYPE=application/x-git-upload-pack-request \
+		QUERY_STRING=/repo.git/git-upload-pack \
+		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+		GIT_HTTP_EXPORT_ALL=TRUE \
+		REQUEST_METHOD=POST \
+		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+		git http-backend </dev/zero >/dev/null 2>err &&
+	grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
 test_done
-- 
2.11.0.1122.gc3fec58.dirty

Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Fri, Nov 24, 2017 at 02:54:50PM +0900, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:
>> I hope I marked it correctly in the trailers.
> 
> It is probably more conventional to do it like so:
> 
>     From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
>     Date: <original date of Florian's patch series>
> 
>     Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
>     [mk: fixed trivial build failures and stuff]
>     Signed-off-by: Max Kirillov <max@max630.net>

Thanks. Have done so

[PATCH v5 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

I seem to forgot to put the authorship lines, and also did something with
replies, so sending another sequence.

v5:

* marked the authorship more correctly

v4:

* polished style
* add tests

Max Kirillov (2):
  http-backend: respect CONTENT_LENGTH as specified by rfc3875
  t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

 Makefile                         |  1 +
 config.c                         |  8 ++++++++
 config.h                         |  1 +
 http-backend.c                   | 39 ++++++++++++++++++++++++++++++++++++++-
 t/helper/test-print-values.c     | 10 ++++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
 6 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-print-values.c

-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

Author: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Date: Wed, 30 Mar 2016 09:08:56 +0000

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.

Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
[mk: fixed trivial build failures and polished style issues]

Signed-off-by: Max Kirillov <max@max630.net>
---
 config.c       |  8 ++++++++
 config.h       |  1 +
 http-backend.c | 39 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 231f9a750b..925bb65dfa 100644
--- a/config.c
+++ b/config.c
@@ -1525,6 +1525,14 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
 	return val;
 }
 
+ssize_t git_env_ssize_t(const char *k, ssize_t val)
+{
+	const char *v = getenv(k);
+	if (v && !git_parse_ssize_t(v, &val))
+		die("failed to parse %s", k);
+	return val;
+}
+
 int git_config_system(void)
 {
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 0352da117b..947695c304 100644
--- a/config.h
+++ b/config.h
@@ -74,6 +74,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c
 extern const char *git_etc_gitconfig(void);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
+extern ssize_t git_env_ssize_t(const char *, ssize_t);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__)
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..b4ba83b624 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
 	size_t len = 0, alloc = 8192;
 	unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,43 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
+{
+	unsigned char *buf = NULL;
+	ssize_t cnt = 0;
+
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu): %lu;"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer,
+			    req_len);
+	}
+
+	if (req_len <= 0) {
+		*out = NULL;
+		return 0;
+	}
+
+	buf = xmalloc(req_len);
+	cnt = read_in_full(fd, buf, req_len);
+	if (cnt < 0) {
+		free(buf);
+		return -1;
+	} else {
+		*out = buf;
+		return cnt;
+	}
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+	if (req_len < 0)
+		return read_request_eof(fd, out);
+	else
+		return read_request_fixed_len(fd, req_len, out);
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v5 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

Add tests for cases:

* CONTENT_LENGTH is set, script's stdin has more data.
  (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
  and fail. It does not seem to cause any performance issues with the default
  value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

Signed-off-by: Max Kirillov <max@max630.net>
---
 Makefile                         |  1 +
 t/helper/test-print-values.c     | 10 ++++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 t/helper/test-print-values.c

diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 0000000000..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <string.h>
+
+int cmd_main(int argc, const char **argv)
+{
+	if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+		printf("%zu", (ssize_t)(-20));
+
+	return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
 	expect_aliased 1 //domain/data.txt
 '
 
+# overrides existing definition for further cases
+run_backend() {
+	CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+	( echo "$2" && cat /dev/zero ) |
+	QUERY_STRING="${1#*[?]}" \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+	git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+	config http.uploadpack true &&
+	GET info/refs?service=git-upload-pack "200 OK"	&&
+	! grep "fatal:.*" act.err &&
+	POST git-upload-pack 0000 "200 OK" &&
+	! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+	NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" "(size_t)(-20)"` &&
+	env \
+		CONTENT_TYPE=application/x-git-upload-pack-request \
+		QUERY_STRING=/repo.git/git-upload-pack \
+		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+		GIT_HTTP_EXPORT_ALL=TRUE \
+		REQUEST_METHOD=POST \
+		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+		git http-backend </dev/zero >/dev/null 2>err &&
+	grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
 test_done
-- 
2.11.0.1122.gc3fec58.dirty

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> Author: Florian Manschwetus <manschwetus@cs-software-gmbh.de>

This should read "From: ...";

> Date: Wed, 30 Mar 2016 09:08:56 +0000
>
> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. Web server may exercise the specification by not closing
> the script's standard input after writing content. In that case http-backend
> would hang waiting for the input. The issue is known to happen with
> IIS/Windows, for example.
>
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
>
> Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
> [mk: fixed trivial build failures and polished style issues]
>
> Signed-off-by: Max Kirillov <max@max630.net>

There shouldn't be a blank line before your sign-off.

The above are only for future reference; I can fix them up before
queuing if there isn't any other issues in this patch.


> +ssize_t git_env_ssize_t(const char *k, ssize_t val)
> +{
> +	const char *v = getenv(k);
> +	if (v && !git_parse_ssize_t(v, &val))
> +		die("failed to parse %s", k);
> +	return val;
> +}
> +

If this were passing "v" that is a string the caller obtains from
any source (and the callee does not care about where it came from),
that would be a different story, but as a public interface that is
part of the config.c layer, "k" that has to be the name of the
environment variable sticks out like a sore thumb.

If we were to add one more public function to the interface, I
suspect that exposing the existing git_parse_ssize_t() and have the
caller implement the above function for its use would be a much
better way to go.

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sun, Nov 26, 2017 at 12:46:12PM +0900, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:
> > +ssize_t git_env_ssize_t(const char *k, ssize_t val)
> > +{
> > +	const char *v = getenv(k);
> > +	if (v && !git_parse_ssize_t(v, &val))
> > +		die("failed to parse %s", k);
> > +	return val;
> > +}
> > +
> 
> If this were passing "v" that is a string the caller obtains from
> any source (and the callee does not care about where it came from),
> that would be a different story, but as a public interface that is
> part of the config.c layer, "k" that has to be the name of the
> environment variable sticks out like a sore thumb.
> 
> If we were to add one more public function to the interface, I
> suspect that exposing the existing git_parse_ssize_t() and have the
> caller implement the above function for its use would be a much
> better way to go.

I'm afraid I did not get the reasonsing and not fully the
desired change. Is this http-backend code change (compared
to the last patch) what you mean?

--- a/http-backend.c
+++ b/http-backend.c
@@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o
 	}
 }
 
+static ssize_t env_content_length()
+{
+	const char *str = getenv("CONTENT_LENGTH");
+	ssize_t val = -1;
+	if (str && !git_parse_ssize_t(str, &val))
+		die("failed to parse CONTENT_LENGTH: %s", str);
+	return val;
+}
+
 static ssize_t read_request(int fd, unsigned char **out)
 {
-	ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
+	ssize_t req_len = env_content_length();
 	if (req_len < 0)
 		return read_request_eof(fd, out);
 	else

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> I'm afraid I did not get the reasonsing and not fully the
> desired change. Is this http-backend code change (compared
> to the last patch) what you mean?

Exactly.  

The fact that the parsed string happens to come from CONTENT_LENGTH
environment variable is http's business, not parsers for various
types of values that come in the form of strings.

> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -346,9 +346,18 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o
>  	}
>  }
>  
> +static ssize_t env_content_length()

We need s/length()/length(void)/; though.

> +{
> +	const char *str = getenv("CONTENT_LENGTH");
> +	ssize_t val = -1;
> +	if (str && !git_parse_ssize_t(str, &val))
> +		die("failed to parse CONTENT_LENGTH: %s", str);
> +	return val;
> +}
> +
>  static ssize_t read_request(int fd, unsigned char **out)
>  {
> -	ssize_t req_len = git_env_ssize_t("CONTENT_LENGTH", -1);
> +	ssize_t req_len = env_content_length();
>  	if (req_len < 0)
>  		return read_request_eof(fd, out);
>  	else

[PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

v6:

Do not implement generic git_env_ssize_t(), instead export git_parse_ssize_t() from config.c
and hardcode the rest.

Florian Manschwetus (1):
  http-backend: respect CONTENT_LENGTH as specified by rfc3875

Max Kirillov (1):
  t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

 Makefile                         |  1 +
 config.c                         |  2 +-
 config.h                         |  1 +
 http-backend.c                   | 50 +++++++++++++++++++++++++++++++++++++++-
 t/helper/test-print-values.c     | 10 ++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++
 6 files changed, 92 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-print-values.c

-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>

From: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
Date: Wed, 30 Mar 2016 10:54:21 +0200

http-backend reads whole input until EOF. However, the RFC 3875 specifies
that a script must read only as many bytes as specified by CONTENT_LENGTH
environment variable. Web server may exercise the specification by not closing
the script's standard input after writing content. In that case http-backend
would hang waiting for the input. The issue is known to happen with
IIS/Windows, for example.

Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
the whole input until EOF. If the variable is not defined, keep older behavior
of reading until EOF because it is used to support chunked transfer-encoding.

Signed-off-by: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
[mk: fixed trivial build failures and polished style issues]
Signed-off-by: Max Kirillov <max@max630.net>
---
 config.c       |  2 +-
 config.h       |  1 +
 http-backend.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 231f9a750b..d3ec14ab74 100644
--- a/config.c
+++ b/config.c
@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 	return 1;
 }
 
-static int git_parse_ssize_t(const char *value, ssize_t *ret)
+int git_parse_ssize_t(const char *value, ssize_t *ret)
 {
 	intmax_t tmp;
 	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
diff --git a/config.h b/config.h
index 0352da117b..46a4989def 100644
--- a/config.h
+++ b/config.h
@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);
 extern int config_with_options(config_fn_t fn, void *,
 			       struct git_config_source *config_source,
 			       const struct config_options *opts);
+extern int git_parse_ssize_t(const char *, ssize_t *);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_parse_maybe_bool(const char *);
 extern int git_config_int(const char *, const char *);
diff --git a/http-backend.c b/http-backend.c
index 519025d2c3..af7dd00d70 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -280,7 +280,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)
  * hit max_request_buffer we die (we'd rather reject a
  * maliciously large request than chew up infinite memory).
  */
-static ssize_t read_request(int fd, unsigned char **out)
+static ssize_t read_request_eof(int fd, unsigned char **out)
 {
 	size_t len = 0, alloc = 8192;
 	unsigned char *buf = xmalloc(alloc);
@@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
 	}
 }
 
+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
+{
+	unsigned char *buf = NULL;
+	ssize_t cnt = 0;
+
+	if (max_request_buffer < req_len) {
+		die("request was larger than our maximum size (%lu): %lu;"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer,
+			    req_len);
+	}
+
+	if (req_len <= 0) {
+		*out = NULL;
+		return 0;
+	}
+
+	buf = xmalloc(req_len);
+	cnt = read_in_full(fd, buf, req_len);
+	if (cnt < 0) {
+		free(buf);
+		return -1;
+	} else {
+		*out = buf;
+		return cnt;
+	}
+}
+
+static ssize_t env_content_length(void)
+{
+	ssize_t val = -1;
+	const char *str = getenv("CONTENT_LENGTH");
+
+	if (str && !git_parse_ssize_t(str, &val))
+		die("failed to parse CONTENT_LENGTH: %s", str);
+	return val;
+}
+
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	ssize_t req_len = env_content_length();
+
+	if (req_len < 0)
+		return read_request_eof(fd, out);
+	else
+		return read_request_fixed_len(fd, req_len, out);
+}
+
 static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
-- 
2.11.0.1122.gc3fec58.dirty

[PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

Add tests for cases:

* CONTENT_LENGTH is set, script's stdin has more data.
  (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
  and fail. It does not seem to cause any performance issues with the default
  value of GIT_HTTP_MAX_REQUEST_BUFFER.)
* CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

Signed-off-by: Max Kirillov <max@max630.net>
---
 Makefile                         |  1 +
 t/helper/test-print-values.c     | 10 ++++++++++
 t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 t/helper/test-print-values.c

diff --git a/Makefile b/Makefile
index 461c845d33..616408b32c 100644
--- a/Makefile
+++ b/Makefile
@@ -653,6 +653,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-print-values
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-ref-store
diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
new file mode 100644
index 0000000000..8f7e5af319
--- /dev/null
+++ b/t/helper/test-print-values.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <string.h>
+
+int cmd_main(int argc, const char **argv)
+{
+	if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
+		printf("%zu", (ssize_t)(-20));
+
+	return 0;
+}
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9fafcf1945..f452090216 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
 	expect_aliased 1 //domain/data.txt
 '
 
+# overrides existing definition for further cases
+run_backend() {
+	CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
+	( echo "$2" && cat /dev/zero ) |
+	QUERY_STRING="${1#*[?]}" \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
+	git http-backend >act.out 2>act.err
+}
+
+test_expect_success 'CONTENT_LENGTH set and infinite input' '
+	config http.uploadpack true &&
+	GET info/refs?service=git-upload-pack "200 OK"	&&
+	! grep "fatal:.*" act.err &&
+	POST git-upload-pack 0000 "200 OK" &&
+	! grep "fatal:.*" act.err
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+	NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" "(size_t)(-20)"` &&
+	env \
+		CONTENT_TYPE=application/x-git-upload-pack-request \
+		QUERY_STRING=/repo.git/git-upload-pack \
+		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
+		GIT_HTTP_EXPORT_ALL=TRUE \
+		REQUEST_METHOD=POST \
+		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
+		git http-backend </dev/zero >/dev/null 2>err &&
+	grep -q "fatal:.*CONTENT_LENGTH" err
+'
+
 test_done
-- 
2.11.0.1122.gc3fec58.dirty

Re: [PATCH v5 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Sun, Nov 26, 2017 at 06:38:06PM +0900, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:
>> +static ssize_t env_content_length()
> 
> We need s/length()/length(void)/; though.

thanks, fixed it

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Eric Sunshine]

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:
> [...]
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
> [...]

A few small comments below; with the possible exception of one,
probably none worth a re-roll...

> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)
> +{
> +       unsigned char *buf = NULL;
> +       ssize_t cnt = 0;
> +
> +       if (max_request_buffer < req_len) {
> +               die("request was larger than our maximum size (%lu): %lu;"
> +                           " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> +                           max_request_buffer,
> +                           req_len);

Unsigned format conversion '%lu' doesn't seem correct for ssize_t.

> +       }
> +
> +       if (req_len <= 0) {
> +               *out = NULL;
> +               return 0;
> +       }
> +
> +       buf = xmalloc(req_len);
> +       cnt = read_in_full(fd, buf, req_len);
> +       if (cnt < 0) {
> +               free(buf);
> +               return -1;
> +       } else {
> +               *out = buf;
> +               return cnt;
> +       }

This could have been written:

    if (cnt < 0) {
        free(buf);
        return -1;
    }
    *out = buf;
    return cnt;

but not worth a re-roll.

> +}
> +
> +static ssize_t env_content_length(void)

The caller of this function doesn't care how the content length is
being determined -- whether it comes from an environment variable or
is computed some other way; it cares only about the result. Having
"env" in the name ties it to checking only the environment. A more
generic name, such as get_content_length(), would help to decouple the
API from the implementation.

Nevertheless, not worth a re-roll.

> +{
> +       ssize_t val = -1;
> +       const char *str = getenv("CONTENT_LENGTH");
> +
> +       if (str && !git_parse_ssize_t(str, &val))

git_parse_ssize_t() does the right thing even when 'str' is NULL, so
this condition could be simplified (but not worth a re-roll and may
not improve clarity).

> +               die("failed to parse CONTENT_LENGTH: %s", str);
> +       return val;
> +}
> +
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +       ssize_t req_len = env_content_length();

Grabbing and parsing the value from the environment variable is
effectively a one-liner, so env_content_length() could be dropped
altogether, and instead (taking advantage of git_parse_ssize_t()'s
proper NULL-handling):

    if (!git_parse_ssize_t(getenv(...), &req_len))
        die(...);

Not worth a re-roll.

> +       if (req_len < 0)
> +               return read_request_eof(fd, out);
> +       else
> +               return read_request_fixed_len(fd, req_len, out);
> +}

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Eric Sunshine]

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:
> Add tests for cases:
>
> * CONTENT_LENGTH is set, script's stdin has more data.
>   (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
>   and fail. It does not seem to cause any performance issues with the default
>   value of GIT_HTTP_MAX_REQUEST_BUFFER.)
> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
> @@ -0,0 +1,10 @@
> +int cmd_main(int argc, const char **argv)
> +{
> +       if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
> +               printf("%zu", (ssize_t)(-20));
> +
> +       return 0;

Perhaps this should return 0 only if it gets the expected argument
"(size_t)(-20)", and return an error otherwise.

> +}
> diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
> @@ -71,4 +71,34 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
> +test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
> +       NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" "(size_t)(-20)"` &&

Rather than introducing a new 'test' program, would it be possible to
get by with just using 'printf' from the shell?

    % printf "%zu\n" -20
    18446744073709551596

> +       env \
> +               CONTENT_TYPE=application/x-git-upload-pack-request \
> +               QUERY_STRING=/repo.git/git-upload-pack \
> +               PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
> +               GIT_HTTP_EXPORT_ALL=TRUE \
> +               REQUEST_METHOD=POST \
> +               CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
> +               git http-backend </dev/zero >/dev/null 2>err &&
> +       grep -q "fatal:.*CONTENT_LENGTH" err
> +'

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

On Sun, Nov 26, 2017 at 05:18:55PM -0500, Eric Sunshine wrote:
> On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:
>> +int cmd_main(int argc, const char **argv)
>> +{
>> +       if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
>> +               printf("%zu", (ssize_t)(-20));
>> +
>> +       return 0;
> 
> Perhaps this should return 0 only if it gets the expected argument
> "(size_t)(-20)", and return an error otherwise.

Yes, makes sense.

> Rather than introducing a new 'test' program, would it be possible to
> get by with just using 'printf' from the shell?
> 
>     % printf "%zu\n" -20
>     18446744073709551596

I thought about it, of course. But, I am not sure I can
exclude cases when the shell's printf uses 64-bit size_t and
git 32-bit one, or vise-versa. Same way, I cannot say it for
sure for any other software which I might use here instead
of the shell's printf. The only somewhat sure way would be
to use the same compiler, with same settings, which is used
for the production code.

I do not exclude possibility that my reasoning above is
wrong, either in general of specifically for git case. If
there are some examples where it is already used and the
risk of type size mismatch is prevented I could do it
similarly.

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> Add tests for cases:
>
> * CONTENT_LENGTH is set, script's stdin has more data.
>   (Failure would make it read GIT_HTTP_MAX_REQUEST_BUFFER bytes from /dev/zero
>   and fail. It does not seem to cause any performance issues with the default
>   value of GIT_HTTP_MAX_REQUEST_BUFFER.)
> * CONTENT_LENGTH is specified to a value which does not fix into ssize_t.

s/fix/fit/ you meant?

> diff --git a/t/helper/test-print-values.c b/t/helper/test-print-values.c
> new file mode 100644
> index 0000000000..8f7e5af319
> --- /dev/null
> +++ b/t/helper/test-print-values.c
> @@ -0,0 +1,10 @@
> +#include <stdio.h>
> +#include <string.h>
> +
> +int cmd_main(int argc, const char **argv)
> +{
> +	if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
> +		printf("%zu", (ssize_t)(-20));
> +
> +	return 0;
> +}

As far as I know, we avoid %zu (C99), as it may not be safe yet to
do so on all platforms.

e.g. c.f. https://public-inbox.org/git/64C7D52F-9030-460C-8F61-4076F5C1DDF6@gmail.com/

You may want to double check the 1/2 of this topic, too.

Forcing a test command line to spell out "(size_t)(-20)" feels a bit
atrocious, especially given that this program is capable of ever
showing that string and nothing else, and it does not even diagnose
typos as errors.

I wonder if we would want to have "test-print-larger-than-ssize" and
do something like

    #include "cache.h"

    int cmd_main(int ac, const char **av)
    {
            uintmax_t large = ((uintmax_t) SSIZE_MAX) + 1;

            printf("%" PRIuMAX "\n", large);
            return 0;
    }

perhaps?

Note that wrapper.c seems to assume that not everybody has
SSIZE_MAX, so we might have to do something silly like

	size_t large = ~0;
	large = ~(large & ~(large >> 1)) + 1;

        printf("%" PRIuMAX "\n", (uintmax_t) large);

just to be careful (even though we now assume 2's complement),
though.

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

To recap (other than the typofix in the proposed log message), here
is what I would have as SQUASH??? on top of (or interspersed with)
v6.

Thanks.

diff --git a/http-backend.c b/http-backend.c
index 69570d16e7..2268d65731 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -324,10 +324,9 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o
 	ssize_t cnt = 0;
 
 	if (max_request_buffer < req_len) {
-		die("request was larger than our maximum size (%lu): %lu;"
-			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
-			    max_request_buffer,
-			    req_len);
+		die("request was larger than our maximum size (%lu): "
+		    "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+		    max_request_buffer, (uintmax_t)req_len);
 	}
 
 	if (req_len <= 0) {

diff --git a/Makefile b/Makefile
index e61f8319b3..3380f68040 100644
--- a/Makefile
+++ b/Makefile
@@ -661,7 +661,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
-TEST_PROGRAMS_NEED_X += test-print-values
+TEST_PROGRAMS_NEED_X += test-print-larger-than-ssize
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-write-cache

diff --git a/t/helper/test-print-values.c b/t/helper/test-print-larger-than-ssize.c
similarity index 31%
rename from t/helper/test-print-values.c
rename to t/helper/test-print-larger-than-ssize.c
index 8f7e5af319..b9852c493d 100644
--- a/t/helper/test-print-values.c
+++ b/t/helper/test-print-larger-than-ssize.c
@@ -1,10 +1,10 @@
-#include <stdio.h>
-#include <string.h>
+#include "cache.h"
 
 int cmd_main(int argc, const char **argv)
 {
-	if (argc == 2 && strcmp(argv[1], "(size_t)(-20)") == 0)
-		printf("%zu", (ssize_t)(-20));
+	size_t large = ~0;
 
+	large = ~(large & ~(large >> 1)) + 1;
+	printf("%" PRIuMAX "\n", (uintmax_t) large);
 	return 0;
 }

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index f452090216..112b5d6eb2 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -72,7 +72,7 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '
 '
 
 # overrides existing definition for further cases
-run_backend() {
+run_backend () {
 	CONTENT_LENGTH="${#2}" && export CONTENT_LENGTH &&
 	( echo "$2" && cat /dev/zero ) |
 	QUERY_STRING="${1#*[?]}" \
@@ -89,7 +89,7 @@ test_expect_success 'CONTENT_LENGTH set and infinite input' '
 '
 
 test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
-	NOT_FIT_IN_SSIZE=`"$GIT_BUILD_DIR/t/helper/test-print-values" "(size_t)(-20)"` &&
+	NOT_FIT_IN_SSIZE=$("$GIT_BUILD_DIR/t/helper/test-print-larger-than-ssize") &&
 	env \
 		CONTENT_TYPE=application/x-git-upload-pack-request \
 		QUERY_STRING=/repo.git/git-upload-pack \

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Jeff King]

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

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

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

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

The concerns I had there were:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-Peff

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Jeff King]

On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:

> > Rather than introducing a new 'test' program, would it be possible to
> > get by with just using 'printf' from the shell?
> > 
> >     % printf "%zu\n" -20
> >     18446744073709551596
> 
> I thought about it, of course. But, I am not sure I can
> exclude cases when the shell's printf uses 64-bit size_t and
> git 32-bit one, or vise-versa. Same way, I cannot say it for
> sure for any other software which I might use here instead
> of the shell's printf. The only somewhat sure way would be
> to use the same compiler, with same settings, which is used
> for the production code.
> 
> I do not exclude possibility that my reasoning above is
> wrong, either in general of specifically for git case. If
> there are some examples where it is already used and the
> risk of type size mismatch is prevented I could do it
> similarly.

That's definitely something to worry about, and I have a vague
recollection that build differences between the shell environment and
git have bitten us in the past.

That said, we already have some precedent in "git version
--build-options" to report sizes there. Can we do something like the
patch below instead of adding a new test helper?

diff --git a/help.c b/help.c
index 88a3aeaeb9..9590eaba28 100644
--- a/help.c
+++ b/help.c
@@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 
 	if (build_options) {
 		printf("sizeof-long: %d\n", (int)sizeof(long));
+		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
 		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
 	}
 	return 0;

That does still require you to compute size_t based on the byte-size in
the test script, that should be do-able.

-Peff

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Mon, Nov 27, 2017 at 01:02:10PM +0900, Junio C Hamano wrote:
> To recap (other than the typofix in the proposed log message), here
> is what I would have as SQUASH??? on top of (or interspersed with)
> v6.

Thank you. I'll update it a bit later. May/should I add
"Signed-off-by:" from you?

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

On Tue, Nov 28, 2017 at 10:26:33PM -0500, Jeff King wrote:
> On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:
> That said, we already have some precedent in "git version
> --build-options" to report sizes there. Can we do something like the
> patch below instead of adding a new test helper?
> 
> diff --git a/help.c b/help.c
> index 88a3aeaeb9..9590eaba28 100644
> --- a/help.c
> +++ b/help.c
> @@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
>  
>  	if (build_options) {
>  		printf("sizeof-long: %d\n", (int)sizeof(long));
> +		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
>  		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
>  	}
>  	return 0;

Thank you! I knew there should have been something.

If nobody objects changing the user-visible behavior, I'll
consider using this.

PS: I'll respond to your other reply a bit later.

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> If nobody objects changing the user-visible behavior, I'll
> consider using this.

What user-visible behaviour?  A user tries to use a new test helper
introduced by the previous round and does not find it?  That's OK.

> PS: I'll respond to your other reply a bit later.

Thanks.  

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> On Mon, Nov 27, 2017 at 01:02:10PM +0900, Junio C Hamano wrote:
>> To recap (other than the typofix in the proposed log message), here
>> is what I would have as SQUASH??? on top of (or interspersed with)
>> v6.
>
> Thank you. I'll update it a bit later. May/should I add
> "Signed-off-by:" from you?

As you'd be dropping the new helper, I think you will not use a
major part of that message.  But even if you would, a "Helped-by:"
before your sign-off should be enough for a small "help" like the
one in the message you are responding.

Thanks.

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   3. For large inputs (like incoming packfiles), we connect the
>      descriptor directly to index-pack or unpack-objects, and they try
>      to read to EOF.
>
>      For a well-formed pack, I _think_ this would work OK. We'd see the
>      end of the pack and quit (there's a check for garbage at the end of
>      the pack, but it triggers only for the non-pipe case).
>
>      For a truncated input, we'd hang forever rather than report an
>      error.

Hmm.  index-pack and/or unpack-objects would be fed directly from
the incoming pipe, they are not told how many bytes to expect (by
design), so they try to read to EOF, which may come after the end of
the pack-stream data, and they write the remaining junk to their
standard output IIRC.

For a well-formed pack, the above is what should heppen.

I am having trouble trying to come up with a case where the input
stream is mangled and we cannot detect where the end of the
pack-stream is without reading more than we will be fed through the
pipe, and yet we do not trigger an "we tried to read because the data
we received so far is incomplete, and got an EOF" error.

Wouldn't "early EOF" trigger in the fill() helper that these two
programs have (but not share X-<)?

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Jeff King]

On Sat, Dec 02, 2017 at 05:02:37PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >   3. For large inputs (like incoming packfiles), we connect the
> >      descriptor directly to index-pack or unpack-objects, and they try
> >      to read to EOF.
> >
> >      For a well-formed pack, I _think_ this would work OK. We'd see the
> >      end of the pack and quit (there's a check for garbage at the end of
> >      the pack, but it triggers only for the non-pipe case).
> >
> >      For a truncated input, we'd hang forever rather than report an
> >      error.
> 
> Hmm.  index-pack and/or unpack-objects would be fed directly from
> the incoming pipe, they are not told how many bytes to expect (by
> design), so they try to read to EOF, which may come after the end of
> the pack-stream data, and they write the remaining junk to their
> standard output IIRC.
> 
> For a well-formed pack, the above is what should heppen.

Yeah, there should be zero bytes of "remaining junk" in the normal
well-formed case. And as long as the webserver does not mind us asking
to read() a few extra bytes, we are fine (it tells us there are no more
bytes available right now). The original problem report with IIS was
that it would hang trying to read() that any final EOF, and I don't
think that would happen here.

I wouldn't be surprised if there are webservers or situations where that
extra read() behaves badly (e.g., because it is connected directly to
the client socket and the client is trying to pipeline requests or
something).

> I am having trouble trying to come up with a case where the input
> stream is mangled and we cannot detect where the end of the
> pack-stream is without reading more than we will be fed through the
> pipe, and yet we do not trigger an "we tried to read because the data
> we received so far is incomplete, and got an EOF" error.
> 
> Wouldn't "early EOF" trigger in the fill() helper that these two
> programs have (but not share X-<)?

I think the original problem was the opposite of "early EOF": the other
side of the pipe never gives us EOF at all. So imagine the pack is
mangled so that the zlib stream of an object never ends, and just keeps
asking for more data. Eventually our fill() will block trying to get
data that is not there. On an Apache server, the webserver would know
there is nothing left to send us and close() the pipe, and we'd get EOF.
But on IIS, I think the pipe remains open and we'd just block
indefinitely trying to read().

I don't have such a setup to test on, and it's possible I'm
mis-interpreting or mis-remembering the discussion around the original
patch.

-Peff

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Eventually our fill() will block trying to get data that is
> not there. On an Apache server, the webserver would know there is
> nothing left to send us and close() the pipe, and we'd get EOF.
> But on IIS, I think the pipe remains open and we'd just block
> indefinitely trying to read().

Ah, yeah, under that scenario, trusting content-length and trying to
read, waiting for input that would never come, will be a problem,
and it would probably want to get documented.

AW: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Florian Manschwetus]

Hi All,
I 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.

Mit freundlichen Grüßen / With kind regards
Florian Manschwetus



> -----Ursprüngliche Nachricht-----
> Von: Junio C Hamano [mailto:gitster@pobox.com]
> Gesendet: Sonntag, 3. Dezember 2017 07:07
> An: Jeff King
> Cc: Max Kirillov; Eric Sunshine; Florian Manschwetus; Chris Packham;
> Konstantin Khomoutov; git@vger.kernel.org
> Betreff: Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as
> specified by rfc3875
> 
> Jeff King <peff@peff.net> writes:
> 
> > ... Eventually our fill() will block trying to get data that is not
> > there. On an Apache server, the webserver would know there is nothing
> > left to send us and close() the pipe, and we'd get EOF.
> > But on IIS, I think the pipe remains open and we'd just block
> > indefinitely trying to read().
> 
> Ah, yeah, under that scenario, trusting content-length and trying to read,
> waiting for input that would never come, will be a problem, and it would
> probably want to get documented.

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Jeff King]

On Mon, Dec 04, 2017 at 07:18:55AM +0000, Florian Manschwetus wrote:

> I 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.

I think it would be pretty easy to handle all cases by inserting another
process in front of http-backend that just reads $CONTENT_LENGTH bytes
and then gives an EOF to http-backend. I assume that's what your bash
script does. It's just that this passes the data through an extra layer
of pipes.

If that's an acceptable tradeoff, it seems like an ideal solution from a
maintainability standpoint, since it skips all the tricky situations
inside the http-backend code.

-Peff

Need to add test artifacts to .gitignore

[Dan Jacques]

FYI, I've noticed when building from "pu" that neither the
"t/helper/test-print-values" or "t/helper/test-print-larger-than-ssize"
testing artifacts added to Makefile in this patch series are not added to
"t/helper/.gitignore" like other helpers, resulting in the testing artifact
being recognized as an untracked file.

[RFC PATCH] t/helper: Move sources to t/helper-src; gitignore any files in t/helper

[Stefan Beller]

Compiled test helpers in t/helper are out of sync with the .gitignore
files quite frequently. This can happen when new test helpers are added,
but the explicit .gitignore file is not updated in the same commit, or
when you forget to 'make clean' before checking out a different version
of git, as the different version may have a different explicit list of
test helpers to ignore.

This can be fixed by using overly broad ignore patterns, such as ignoring
the whole directory via '//t/helper/*' in .gitignore.

However we do not want to have an overlap of checked source files and
ignored files, hence we'll move the the source files currently residing
in t/helper to t/helper-src. To accommodate that we'll need to update
the Makefile as well to look at a different place for the source files.
(This patch takes the hacky approach in symlinking the sources back into
the t/helper, which we'd want to avoid long term)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Makefile                                           |  4 +++
 t/{helper => helper-src}/.gitignore                |  0
 t/{helper => helper-src}/test-chmtime.c            |  0
 t/{helper => helper-src}/test-config.c             |  0
 t/{helper => helper-src}/test-ctype.c              |  0
 t/{helper => helper-src}/test-date.c               |  0
 t/{helper => helper-src}/test-delta.c              |  0
 t/{helper => helper-src}/test-dump-cache-tree.c    |  0
 t/{helper => helper-src}/test-dump-split-index.c   |  0
 .../test-dump-untracked-cache.c                    |  0
 t/{helper => helper-src}/test-fake-ssh.c           |  0
 t/{helper => helper-src}/test-genrandom.c          |  0
 t/{helper => helper-src}/test-hashmap.c            |  0
 t/{helper => helper-src}/test-index-version.c      |  0
 .../test-lazy-init-name-hash.c                     |  0
 t/{helper => helper-src}/test-line-buffer.c        |  0
 t/{helper => helper-src}/test-match-trees.c        |  0
 t/{helper => helper-src}/test-mergesort.c          |  0
 t/{helper => helper-src}/test-mktemp.c             |  0
 t/{helper => helper-src}/test-online-cpus.c        |  0
 t/{helper => helper-src}/test-parse-options.c      |  0
 t/{helper => helper-src}/test-path-utils.c         |  0
 t/{helper => helper-src}/test-prio-queue.c         |  0
 t/{helper => helper-src}/test-read-cache.c         |  0
 t/{helper => helper-src}/test-ref-store.c          |  0
 t/{helper => helper-src}/test-regex.c              |  0
 t/{helper => helper-src}/test-revision-walking.c   |  0
 t/{helper => helper-src}/test-run-command.c        |  0
 t/{helper => helper-src}/test-scrap-cache-tree.c   |  0
 t/{helper => helper-src}/test-sha1-array.c         |  0
 t/{helper => helper-src}/test-sha1.c               |  0
 t/{helper => helper-src}/test-sha1.sh              |  0
 t/{helper => helper-src}/test-sigchain.c           |  0
 t/{helper => helper-src}/test-strcmp-offset.c      |  0
 t/{helper => helper-src}/test-string-list.c        |  0
 t/{helper => helper-src}/test-submodule-config.c   |  0
 t/{helper => helper-src}/test-subprocess.c         |  0
 t/{helper => helper-src}/test-svn-fe.c             |  0
 .../test-urlmatch-normalization.c                  |  0
 t/{helper => helper-src}/test-wildmatch.c          |  0
 t/{helper => helper-src}/test-write-cache.c        |  0
 t/helper/.gitignore                                | 39 +---------------------
 42 files changed, 5 insertions(+), 38 deletions(-)
 copy t/{helper => helper-src}/.gitignore (100%)
 rename t/{helper => helper-src}/test-chmtime.c (100%)
 rename t/{helper => helper-src}/test-config.c (100%)
 rename t/{helper => helper-src}/test-ctype.c (100%)
 rename t/{helper => helper-src}/test-date.c (100%)
 rename t/{helper => helper-src}/test-delta.c (100%)
 rename t/{helper => helper-src}/test-dump-cache-tree.c (100%)
 rename t/{helper => helper-src}/test-dump-split-index.c (100%)
 rename t/{helper => helper-src}/test-dump-untracked-cache.c (100%)
 rename t/{helper => helper-src}/test-fake-ssh.c (100%)
 rename t/{helper => helper-src}/test-genrandom.c (100%)
 rename t/{helper => helper-src}/test-hashmap.c (100%)
 rename t/{helper => helper-src}/test-index-version.c (100%)
 rename t/{helper => helper-src}/test-lazy-init-name-hash.c (100%)
 rename t/{helper => helper-src}/test-line-buffer.c (100%)
 rename t/{helper => helper-src}/test-match-trees.c (100%)
 rename t/{helper => helper-src}/test-mergesort.c (100%)
 rename t/{helper => helper-src}/test-mktemp.c (100%)
 rename t/{helper => helper-src}/test-online-cpus.c (100%)
 rename t/{helper => helper-src}/test-parse-options.c (100%)
 rename t/{helper => helper-src}/test-path-utils.c (100%)
 rename t/{helper => helper-src}/test-prio-queue.c (100%)
 rename t/{helper => helper-src}/test-read-cache.c (100%)
 rename t/{helper => helper-src}/test-ref-store.c (100%)
 rename t/{helper => helper-src}/test-regex.c (100%)
 rename t/{helper => helper-src}/test-revision-walking.c (100%)
 rename t/{helper => helper-src}/test-run-command.c (100%)
 rename t/{helper => helper-src}/test-scrap-cache-tree.c (100%)
 rename t/{helper => helper-src}/test-sha1-array.c (100%)
 rename t/{helper => helper-src}/test-sha1.c (100%)
 rename t/{helper => helper-src}/test-sha1.sh (100%)
 rename t/{helper => helper-src}/test-sigchain.c (100%)
 rename t/{helper => helper-src}/test-strcmp-offset.c (100%)
 rename t/{helper => helper-src}/test-string-list.c (100%)
 rename t/{helper => helper-src}/test-submodule-config.c (100%)
 rename t/{helper => helper-src}/test-subprocess.c (100%)
 rename t/{helper => helper-src}/test-svn-fe.c (100%)
 rename t/{helper => helper-src}/test-urlmatch-normalization.c (100%)
 rename t/{helper => helper-src}/test-wildmatch.c (100%)
 rename t/{helper => helper-src}/test-write-cache.c (100%)
 rewrite t/helper/.gitignore (100%)

diff --git a/Makefile b/Makefile
index c26596c30a..477ddef820 100644
--- a/Makefile
+++ b/Makefile
@@ -2454,6 +2454,10 @@ t/helper/test-svn-fe$X: $(VCSSVN_LIB)
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
+t/helper/test-%.c: t/helper-src/test-%.c
+	@echo $@ $<
+	ln -s ../../$< $@
+
 check-sha1:: t/helper/test-sha1$X
 	t/helper/test-sha1.sh
 
diff --git a/t/helper/.gitignore b/t/helper-src/.gitignore
similarity index 100%
copy from t/helper/.gitignore
copy to t/helper-src/.gitignore
diff --git a/t/helper/test-chmtime.c b/t/helper-src/test-chmtime.c
similarity index 100%
rename from t/helper/test-chmtime.c
rename to t/helper-src/test-chmtime.c
diff --git a/t/helper/test-config.c b/t/helper-src/test-config.c
similarity index 100%
rename from t/helper/test-config.c
rename to t/helper-src/test-config.c
diff --git a/t/helper/test-ctype.c b/t/helper-src/test-ctype.c
similarity index 100%
rename from t/helper/test-ctype.c
rename to t/helper-src/test-ctype.c
diff --git a/t/helper/test-date.c b/t/helper-src/test-date.c
similarity index 100%
rename from t/helper/test-date.c
rename to t/helper-src/test-date.c
diff --git a/t/helper/test-delta.c b/t/helper-src/test-delta.c
similarity index 100%
rename from t/helper/test-delta.c
rename to t/helper-src/test-delta.c
diff --git a/t/helper/test-dump-cache-tree.c b/t/helper-src/test-dump-cache-tree.c
similarity index 100%
rename from t/helper/test-dump-cache-tree.c
rename to t/helper-src/test-dump-cache-tree.c
diff --git a/t/helper/test-dump-split-index.c b/t/helper-src/test-dump-split-index.c
similarity index 100%
rename from t/helper/test-dump-split-index.c
rename to t/helper-src/test-dump-split-index.c
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper-src/test-dump-untracked-cache.c
similarity index 100%
rename from t/helper/test-dump-untracked-cache.c
rename to t/helper-src/test-dump-untracked-cache.c
diff --git a/t/helper/test-fake-ssh.c b/t/helper-src/test-fake-ssh.c
similarity index 100%
rename from t/helper/test-fake-ssh.c
rename to t/helper-src/test-fake-ssh.c
diff --git a/t/helper/test-genrandom.c b/t/helper-src/test-genrandom.c
similarity index 100%
rename from t/helper/test-genrandom.c
rename to t/helper-src/test-genrandom.c
diff --git a/t/helper/test-hashmap.c b/t/helper-src/test-hashmap.c
similarity index 100%
rename from t/helper/test-hashmap.c
rename to t/helper-src/test-hashmap.c
diff --git a/t/helper/test-index-version.c b/t/helper-src/test-index-version.c
similarity index 100%
rename from t/helper/test-index-version.c
rename to t/helper-src/test-index-version.c
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper-src/test-lazy-init-name-hash.c
similarity index 100%
rename from t/helper/test-lazy-init-name-hash.c
rename to t/helper-src/test-lazy-init-name-hash.c
diff --git a/t/helper/test-line-buffer.c b/t/helper-src/test-line-buffer.c
similarity index 100%
rename from t/helper/test-line-buffer.c
rename to t/helper-src/test-line-buffer.c
diff --git a/t/helper/test-match-trees.c b/t/helper-src/test-match-trees.c
similarity index 100%
rename from t/helper/test-match-trees.c
rename to t/helper-src/test-match-trees.c
diff --git a/t/helper/test-mergesort.c b/t/helper-src/test-mergesort.c
similarity index 100%
rename from t/helper/test-mergesort.c
rename to t/helper-src/test-mergesort.c
diff --git a/t/helper/test-mktemp.c b/t/helper-src/test-mktemp.c
similarity index 100%
rename from t/helper/test-mktemp.c
rename to t/helper-src/test-mktemp.c
diff --git a/t/helper/test-online-cpus.c b/t/helper-src/test-online-cpus.c
similarity index 100%
rename from t/helper/test-online-cpus.c
rename to t/helper-src/test-online-cpus.c
diff --git a/t/helper/test-parse-options.c b/t/helper-src/test-parse-options.c
similarity index 100%
rename from t/helper/test-parse-options.c
rename to t/helper-src/test-parse-options.c
diff --git a/t/helper/test-path-utils.c b/t/helper-src/test-path-utils.c
similarity index 100%
rename from t/helper/test-path-utils.c
rename to t/helper-src/test-path-utils.c
diff --git a/t/helper/test-prio-queue.c b/t/helper-src/test-prio-queue.c
similarity index 100%
rename from t/helper/test-prio-queue.c
rename to t/helper-src/test-prio-queue.c
diff --git a/t/helper/test-read-cache.c b/t/helper-src/test-read-cache.c
similarity index 100%
rename from t/helper/test-read-cache.c
rename to t/helper-src/test-read-cache.c
diff --git a/t/helper/test-ref-store.c b/t/helper-src/test-ref-store.c
similarity index 100%
rename from t/helper/test-ref-store.c
rename to t/helper-src/test-ref-store.c
diff --git a/t/helper/test-regex.c b/t/helper-src/test-regex.c
similarity index 100%
rename from t/helper/test-regex.c
rename to t/helper-src/test-regex.c
diff --git a/t/helper/test-revision-walking.c b/t/helper-src/test-revision-walking.c
similarity index 100%
rename from t/helper/test-revision-walking.c
rename to t/helper-src/test-revision-walking.c
diff --git a/t/helper/test-run-command.c b/t/helper-src/test-run-command.c
similarity index 100%
rename from t/helper/test-run-command.c
rename to t/helper-src/test-run-command.c
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper-src/test-scrap-cache-tree.c
similarity index 100%
rename from t/helper/test-scrap-cache-tree.c
rename to t/helper-src/test-scrap-cache-tree.c
diff --git a/t/helper/test-sha1-array.c b/t/helper-src/test-sha1-array.c
similarity index 100%
rename from t/helper/test-sha1-array.c
rename to t/helper-src/test-sha1-array.c
diff --git a/t/helper/test-sha1.c b/t/helper-src/test-sha1.c
similarity index 100%
rename from t/helper/test-sha1.c
rename to t/helper-src/test-sha1.c
diff --git a/t/helper/test-sha1.sh b/t/helper-src/test-sha1.sh
similarity index 100%
rename from t/helper/test-sha1.sh
rename to t/helper-src/test-sha1.sh
diff --git a/t/helper/test-sigchain.c b/t/helper-src/test-sigchain.c
similarity index 100%
rename from t/helper/test-sigchain.c
rename to t/helper-src/test-sigchain.c
diff --git a/t/helper/test-strcmp-offset.c b/t/helper-src/test-strcmp-offset.c
similarity index 100%
rename from t/helper/test-strcmp-offset.c
rename to t/helper-src/test-strcmp-offset.c
diff --git a/t/helper/test-string-list.c b/t/helper-src/test-string-list.c
similarity index 100%
rename from t/helper/test-string-list.c
rename to t/helper-src/test-string-list.c
diff --git a/t/helper/test-submodule-config.c b/t/helper-src/test-submodule-config.c
similarity index 100%
rename from t/helper/test-submodule-config.c
rename to t/helper-src/test-submodule-config.c
diff --git a/t/helper/test-subprocess.c b/t/helper-src/test-subprocess.c
similarity index 100%
rename from t/helper/test-subprocess.c
rename to t/helper-src/test-subprocess.c
diff --git a/t/helper/test-svn-fe.c b/t/helper-src/test-svn-fe.c
similarity index 100%
rename from t/helper/test-svn-fe.c
rename to t/helper-src/test-svn-fe.c
diff --git a/t/helper/test-urlmatch-normalization.c b/t/helper-src/test-urlmatch-normalization.c
similarity index 100%
rename from t/helper/test-urlmatch-normalization.c
rename to t/helper-src/test-urlmatch-normalization.c
diff --git a/t/helper/test-wildmatch.c b/t/helper-src/test-wildmatch.c
similarity index 100%
rename from t/helper/test-wildmatch.c
rename to t/helper-src/test-wildmatch.c
diff --git a/t/helper/test-write-cache.c b/t/helper-src/test-write-cache.c
similarity index 100%
rename from t/helper/test-write-cache.c
rename to t/helper-src/test-write-cache.c
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
dissimilarity index 100%
index 7c9d28a834..72e8ffc0db 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,38 +1 @@
-/test-chmtime
-/test-ctype
-/test-config
-/test-date
-/test-delta
-/test-dump-cache-tree
-/test-dump-split-index
-/test-dump-untracked-cache
-/test-fake-ssh
-/test-scrap-cache-tree
-/test-genrandom
-/test-hashmap
-/test-index-version
-/test-lazy-init-name-hash
-/test-line-buffer
-/test-match-trees
-/test-mergesort
-/test-mktemp
-/test-online-cpus
-/test-parse-options
-/test-path-utils
-/test-prio-queue
-/test-read-cache
-/test-ref-store
-/test-regex
-/test-revision-walking
-/test-run-command
-/test-sha1
-/test-sha1-array
-/test-sigchain
-/test-strcmp-offset
-/test-string-list
-/test-submodule-config
-/test-subprocess
-/test-svn-fe
-/test-urlmatch-normalization
-/test-wildmatch
-/test-write-cache
+*
-- 
2.15.1.504.g5279b80103-goog

Re: [RFC PATCH] t/helper: Move sources to t/helper-src; gitignore any files in t/helper

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Compiled test helpers in t/helper are out of sync with the .gitignore
> files quite frequently. This can happen when new test helpers are added,
> but the explicit .gitignore file is not updated in the same commit, or
> when you forget to 'make clean' before checking out a different version
> of git, as the different version may have a different explicit list of
> test helpers to ignore.
>
> This can be fixed by using overly broad ignore patterns, such as ignoring
> the whole directory via '//t/helper/*' in .gitignore.

If we ignore everything but resurrect *.[ch] with negative exclude
rules, can we do the same without moving things around?

[PATCH] t/helper: ignore everything but sources

[Stefan Beller]

Compiled test helpers in t/helper are out of sync with the .gitignore
files quite frequently. This can happen when new test helpers are added,
but the explicit .gitignore file is not updated in the same commit, or
when you forget to 'make clean' before checking out a different version
of git, as the different version may have a different explicit list of
test helpers to ignore.

Fix this by having an overly broad ignore pattern in that directory:
Anything, except C and shell source, will be ignored.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

> If we ignore everything but resurrect *.[ch] with negative exclude
> rules, can we do the same without moving things around?

Yes, there is also one lonely shell script in there, which also needs
exclusion.

 I guess we want to test this overly broad pattern in a sub directory,
 as I could imagine that at the top level directory we'd have more cases
 to think through (I used to put untracked files into the top level dir,
 but I do not do it anymore).
 
Thanks,
Stefan

 t/helper/.gitignore | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index d02f9b39ac..ee1e39fd08 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,40 +1,3 @@
-/test-chmtime
-/test-ctype
-/test-config
-/test-date
-/test-delta
-/test-drop-caches
-/test-dump-cache-tree
-/test-dump-fsmonitor
-/test-dump-split-index
-/test-dump-untracked-cache
-/test-fake-ssh
-/test-scrap-cache-tree
-/test-genrandom
-/test-hashmap
-/test-index-version
-/test-lazy-init-name-hash
-/test-line-buffer
-/test-match-trees
-/test-mergesort
-/test-mktemp
-/test-online-cpus
-/test-parse-options
-/test-path-utils
-/test-prio-queue
-/test-read-cache
-/test-ref-store
-/test-regex
-/test-revision-walking
-/test-run-command
-/test-sha1
-/test-sha1-array
-/test-sigchain
-/test-strcmp-offset
-/test-string-list
-/test-submodule-config
-/test-subprocess
-/test-svn-fe
-/test-urlmatch-normalization
-/test-wildmatch
-/test-write-cache
+*
+!.sh
+!.[ch]
-- 
2.15.1.504.g5279b80103-goog

Re: [PATCH] t/helper: ignore everything but sources

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Yes, there is also one lonely shell script in there, which also needs
> exclusion.

Thanks for catching them.

> +*
> +!.sh
> +!.[ch]

I'd use this instead, though.

-- >8 --
*
!*.sh
!*.[ch]
!*.gitignore
-- 8< --

In a dirty repository full of crufts but without any local
modifications, if you do

    $ git rm --cached -r t/helper
    $ git add t/helper

you should be able to make your index identical to HEAD.  The
version that was posted did not resurrect .gitignore and none
of the source files, but the replaced one should.

Re: [PATCH] t/helper: ignore everything but sources

[Todd Zullinger]

Hi Stefan,

Stefan Beller wrote:
>> If we ignore everything but resurrect *.[ch] with negative exclude
>> rules, can we do the same without moving things around?
> 
> Yes, there is also one lonely shell script in there, which also needs
> exclusion.

There aren't currently any .h files, but I suppose it doesn't hurt to
include that pattern to be safer for the future.

> +*
> +!.sh
> +!.[ch]

The ! patterns are missing a '*'.  I think it should be:

*
!*.[ch]
!*.sh

Does it make sense to also include !.gitignore as well?
It's already committed, so it's not ignored.  But perhaps
having it listed will save someone from getting their repo
into a state where local changes to .gitignore aren't picked
up (I know that's a bit of a stretch).

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
How much does it cost to entice a dope-smoking UNIX system guru to
Dayton?
    -- Brian Boyle, UNIX/WORLD's First Annual Salary Survey

[PATCH] t/helper: ignore everything but sources

[Stefan Beller]

Compiled test helpers in t/helper are out of sync with the .gitignore
files quite frequently. This can happen when new test helpers are added,
but the explicit .gitignore file is not updated in the same commit, or
when you forget to 'make clean' before checking out a different version
of git, as the different version may have a different explicit list of
test helpers to ignore.

Fix this by having an overly broad ignore pattern in that directory:
Anything, except C and shell source, will be ignored.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Thanks Todd and Junio for mentioning the .gitignore file.


 t/helper/.gitignore | 44 ++++----------------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index d02f9b39ac..5b540625af 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,40 +1,4 @@
-/test-chmtime
-/test-ctype
-/test-config
-/test-date
-/test-delta
-/test-drop-caches
-/test-dump-cache-tree
-/test-dump-fsmonitor
-/test-dump-split-index
-/test-dump-untracked-cache
-/test-fake-ssh
-/test-scrap-cache-tree
-/test-genrandom
-/test-hashmap
-/test-index-version
-/test-lazy-init-name-hash
-/test-line-buffer
-/test-match-trees
-/test-mergesort
-/test-mktemp
-/test-online-cpus
-/test-parse-options
-/test-path-utils
-/test-prio-queue
-/test-read-cache
-/test-ref-store
-/test-regex
-/test-revision-walking
-/test-run-command
-/test-sha1
-/test-sha1-array
-/test-sigchain
-/test-strcmp-offset
-/test-string-list
-/test-submodule-config
-/test-subprocess
-/test-svn-fe
-/test-urlmatch-normalization
-/test-wildmatch
-/test-write-cache
+*
+!*.[ch]
+!*.sh
+!.gitignore
-- 
2.15.1.504.g5279b80103-goog

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> v6:
>
> Do not implement generic git_env_ssize_t(), instead export git_parse_ssize_t() from config.c
> and hardcode the rest.
>
> Florian Manschwetus (1):
>   http-backend: respect CONTENT_LENGTH as specified by rfc3875
>
> Max Kirillov (1):
>   t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases
>
>  Makefile                         |  1 +
>  config.c                         |  2 +-
>  config.h                         |  1 +
>  http-backend.c                   | 50 +++++++++++++++++++++++++++++++++++++++-
>  t/helper/test-print-values.c     | 10 ++++++++
>  t/t5560-http-backend-noserver.sh | 30 ++++++++++++++++++++++++
>  6 files changed, 92 insertions(+), 2 deletions(-)
>  create mode 100644 t/helper/test-print-values.c

So... is there going to be an update (or has there been one and I
missed it)?

Thanks.

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Tue, Dec 19, 2017 at 02:13:33PM -0800, Junio C Hamano wrote:
> So... is there going to be an update (or has there been one and I
> missed it)?

Yes there it! I wanted to add tests for the cases Jeff
mentioned. It is almost done, I just need to check I did not
miss some note.