threads / patch / 41856

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

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

## tl;dr

50 messages between Mar 29, 2016 and Dec 20, 2017. Diffs are folded; open one to read it.

replies: 49people: 8as markdown or json

Florian Manschwetus· Mar 29, 2016, 10:38 UTC · lore
-----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:
Show 61 quoted lines
> 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(-)
Show changes to http-backend.c +31 −17
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
Jeff King· Mar 29, 2016, 20:13 UTC · re: Florian Manschwetus · lore

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:
Show 17 quoted lines
> > | 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).

Show 27 quoted lines
> 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
Florian Manschwetus· Mar 30, 2016, 09:08 UTC · re: Jeff King · lore

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

Show 77 quoted lines
> -----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(-)
Show changes to http-backend.c +30 −17
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
Jeff King· Apr 1, 2016, 23:55 UTC · re: Florian Manschwetus · lore

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

On Wed, Mar 30, 2016 at 09:08:56AM +0000, Florian Manschwetus wrote:
Show 6 quoted lines
> 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).

Show 5 quoted lines
> 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.

Show 7 quoted lines
> 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.

Show 19 quoted lines
> +/**
> + * 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
Max Kirillov· Nov 23, 2017, 23:45 UTC · re: Jeff King · lore

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

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(-)
Show changes to 3 files +80 −1

config.c, config.h, http-backend.c

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
Eric Sunshine· Nov 24, 2017, 01:30 UTC · re: Max Kirillov · lore

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

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/
Show 14 quoted lines
> 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/
Show 5 quoted lines
> +{
> +       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.

Show 8 quoted lines
> +       }
> +
> +       if (req_len <= 0) {
> +               *out = NULL;
> +               return 0;
> +       }
> +
> +       /* allocate buffer */
Drop valueless comment.
> +       buf = xmalloc(req_len);
> +
> +
Style: Too many blank lines.
Show 15 quoted 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/
Show 9 quoted lines
> +                       *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.
Show 6 quoted lines
> + * 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.
Show 6 quoted lines
> +       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);
> +}
Max Kirillov· Nov 25, 2017, 21:47 UTC · re: Eric Sunshine · lore

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

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
Eric Sunshine· Nov 26, 2017, 00:38 UTC · re: Max Kirillov · lore

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

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.

Show 10 quoted lines
> 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?

Max Kirillov· Nov 26, 2017, 00:43 UTC · re: Eric Sunshine · lore

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

On Sat, Nov 25, 2017 at 07:38:33PM -0500, Eric Sunshine wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Nov 24, 2017, 05:54 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
Show 15 quoted lines
> 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>
    ---
Show 6 quoted lines
>  
> +/*
> + * 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.

Florian Manschwetus· Nov 24, 2017, 08:30 UTC · re: Junio C Hamano · lore

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

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
Show 56 quoted lines
> -----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.
Max Kirillov· Nov 26, 2017, 01:50 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 24, 2017 at 02:54:50PM +0900, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Max Kirillov· Nov 26, 2017, 01:47 UTC · re: Max Kirillov · lore

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

* 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
Max Kirillov· Nov 26, 2017, 01:47 UTC · re: Max Kirillov · lore

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

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(-)
Show changes to 3 files +47 −1

config.c, config.h, http-backend.c

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
Max Kirillov· Nov 26, 2017, 01:47 UTC · re: Max Kirillov · lore

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

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
Show changes to 3 files +41 −0

Makefile, t/helper/test-print-values.c, t/t5560-http-backend-noserver.sh

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
Max Kirillov· Nov 26, 2017, 01:54 UTC · re: Max Kirillov · lore

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

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
Max Kirillov· Nov 26, 2017, 01:54 UTC · re: Max Kirillov · lore

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

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
Show changes to 3 files +41 −0

Makefile, t/helper/test-print-values.c, t/t5560-http-backend-noserver.sh

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
Max Kirillov· Nov 26, 2017, 01:54 UTC · re: Max Kirillov · lore

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

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(-)
Show changes to 3 files +47 −1

config.c, config.h, http-backend.c

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
Junio C Hamano· Nov 26, 2017, 03:46 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
> Author: Florian Manschwetus <manschwetus@cs-software-gmbh.de>
This should read "From: ...";
Show 17 quoted lines
> 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.

Show 8 quoted lines
> +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.

Max Kirillov· Nov 26, 2017, 08:13 UTC · re: Junio C Hamano · lore

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

On Sun, Nov 26, 2017 at 12:46:12PM +0900, Junio C Hamano wrote:
Show 20 quoted lines
> 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?

Show changes to http-backend.c +10 −1
--- 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
Junio C Hamano· Nov 26, 2017, 09:38 UTC · re: Max Kirillov · lore

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

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.

Show 7 quoted lines
> --- 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.
Show 15 quoted lines
> +{
> +	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
Max Kirillov· Nov 26, 2017, 19:39 UTC · re: Junio C Hamano · lore

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

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
Max Kirillov· Nov 26, 2017, 19:38 UTC · re: Max Kirillov · lore

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

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
Max Kirillov· Nov 26, 2017, 19:38 UTC · re: Max Kirillov · lore

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

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(-)
Show changes to 3 files +51 −2

config.c, config.h, http-backend.c

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
Eric Sunshine· Nov 26, 2017, 22:08 UTC · re: Max Kirillov · lore

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

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:
Show 5 quoted lines
> [...]
> 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...

Show 12 quoted lines
> 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.
Show 16 quoted lines
> +       }
> +
> +       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.
Show 5 quoted lines
> +{
> +       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).

Show 7 quoted lines
> +               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.
Show 5 quoted lines
> +       if (req_len < 0)
> +               return read_request_eof(fd, out);
> +       else
> +               return read_request_fixed_len(fd, req_len, out);
> +}
Jeff King· Nov 29, 2017, 03:22 UTC · re: Max Kirillov · lore

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

On Sun, Nov 26, 2017 at 09:38:12PM +0200, Max Kirillov wrote:
Show 13 quoted lines
> 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.

Show 5 quoted lines
> ---
>  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.

Show 14 quoted lines
> +	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
Junio C Hamano· Dec 3, 2017, 01:02 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
>   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-<)?

Jeff King· Dec 3, 2017, 02:49 UTC · re: Junio C Hamano · lore

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

On Sat, Dec 02, 2017 at 05:02:37PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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).

Show 8 quoted lines
> 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
Junio C Hamano· Dec 3, 2017, 06:07 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> ... 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.

Florian Manschwetus· Dec 4, 2017, 07:18 UTC · re: Junio C Hamano · lore

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

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

Show 20 quoted lines
> -----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.
Jeff King· Dec 4, 2017, 17:13 UTC · re: Florian Manschwetus · lore

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

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
Max Kirillov· Nov 26, 2017, 19:38 UTC · re: Max Kirillov · lore

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

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
Show changes to 3 files +41 −0

Makefile, t/helper/test-print-values.c, t/t5560-http-backend-noserver.sh

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
Eric Sunshine· Nov 26, 2017, 22:18 UTC · re: Max Kirillov · lore

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

On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <max@max630.net> wrote:
Show 18 quoted lines
> 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.

Show 5 quoted lines
> +}
> 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
Show 10 quoted lines
> +       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
> +'
Max Kirillov· Nov 26, 2017, 22:40 UTC · re: Eric Sunshine · lore

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

On Sun, Nov 26, 2017 at 05:18:55PM -0500, Eric Sunshine wrote:
Show 10 quoted lines
> 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.
Show 5 quoted lines
> 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.

Jeff King· Nov 29, 2017, 03:26 UTC · re: Max Kirillov · lore

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

On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:
Show 19 quoted lines
> > 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?

Show changes to help.c +1 −1
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
Max Kirillov· Nov 29, 2017, 05:19 UTC · re: Jeff King · lore

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

On Tue, Nov 28, 2017 at 10:26:33PM -0500, Jeff King wrote:
Show 17 quoted lines
> 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.
Junio C Hamano· Dec 3, 2017, 00:46 UTC · re: Max Kirillov · lore

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

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.  
Junio C Hamano· Nov 27, 2017, 00:29 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
Show 7 quoted lines
> 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?
Show 16 quoted lines
> 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.

Junio C Hamano· Nov 27, 2017, 04:02 UTC · re: Max Kirillov · lore

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

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.
Show changes to 5 files +10 −11

http-backend.c, Makefile, t/helper/test-print-values.c, t/helper/test-print-larger-than-ssize.c, t/t5560-http-backend-noserver.sh

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 \
Max Kirillov· Nov 29, 2017, 05:07 UTC · re: Junio C Hamano · lore

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

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?

Junio C Hamano· Dec 3, 2017, 00:48 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
Show 7 quoted lines
> 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.
Dan Jacques· Dec 12, 2017, 16:17 UTC · re: Junio C Hamano · lore

Need to add test artifacts to .gitignore

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.

Stefan Beller· Dec 12, 2017, 19:00 UTC · re: Dan Jacques · lore

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

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%)
Show changes to 41 files +5 −38

Makefile, t/helper/.gitignore, t/helper/test-chmtime.c, t/helper/test-config.c, t/helper/test-ctype.c, t/helper/test-date.c, t/helper/test-delta.c, t/helper/test-dump-cache-tree.c, t/helper/test-dump-split-index.c, t/helper/test-dump-untracked-cache.c, t/helper/test-fake-ssh.c, t/helper/test-genrandom.c, t/helper/test-hashmap.c, t/helper/test-index-version.c, t/helper/test-lazy-init-name-hash.c, t/helper/test-line-buffer.c, t/helper/test-match-trees.c, t/helper/test-mergesort.c, t/helper/test-mktemp.c, t/helper/test-online-cpus.c, t/helper/test-parse-options.c, t/helper/test-path-utils.c, t/helper/test-prio-queue.c, t/helper/test-read-cache.c, t/helper/test-ref-store.c, t/helper/test-regex.c, t/helper/test-revision-walking.c, t/helper/test-run-command.c, t/helper/test-scrap-cache-tree.c, t/helper/test-sha1-array.c, t/helper/test-sha1.c, t/helper/test-sha1.sh, t/helper/test-sigchain.c, t/helper/test-strcmp-offset.c, t/helper/test-string-list.c, t/helper/test-submodule-config.c, t/helper/test-subprocess.c, t/helper/test-svn-fe.c, t/helper/test-urlmatch-normalization.c, t/helper/test-wildmatch.c, t/helper/test-write-cache.c

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
Junio C Hamano· Dec 12, 2017, 19:59 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 9 quoted lines
> 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?

Stefan Beller· Dec 12, 2017, 20:56 UTC · re: Junio C Hamano · lore

[PATCH] t/helper: ignore everything but sources

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(-)
Show changes to t/helper/.gitignore +3 −40
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
Junio C Hamano· Dec 12, 2017, 21:06 UTC · re: Stefan Beller · lore

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

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.

Stefan Beller· Dec 13, 2017, 20:12 UTC · re: Junio C Hamano · lore

[PATCH] t/helper: ignore everything but sources

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(-)
Show changes to t/helper/.gitignore +4 −40
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
Todd Zullinger· Dec 12, 2017, 21:06 UTC · re: Stefan Beller · lore

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

Hi Stefan,
Stefan Beller wrote:
Show 5 quoted lines
>> 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
Junio C Hamano· Dec 19, 2017, 22:13 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
Show 19 quoted lines
> 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.
Max Kirillov· Dec 20, 2017, 04:30 UTC · re: Junio C Hamano · lore

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

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.

← back to recent threads