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

Hi,
I put together a first patch for the issue.

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

E-Mail: [email protected]
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:[email protected]]
Gesendet: Donnerstag, 10. März 2016 13:55
An: Florian Manschwetus
Cc: [email protected]
Betreff: Re: Problem with git-http-backend.exe as iis cgi

Quoted 7 lines · On Thu, 10 Mar 2016 07:28:50 +0000 Florian Manschwetus…

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

Quoted 8 lines

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

Quoted 11 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:

Quoted 6 lines

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

http-backend.c +7 -2

index 8870a26..94976df 100644
@@ -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 */

-----Ursprüngliche Nachricht-----
Von: Jeff King [mailto:[email protected]]
Gesendet: Dienstag, 29. März 2016 22:14
An: Florian Manschwetus
Cc: Chris Packham; Konstantin Khomoutov; [email protected]
Betreff: Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http- backend.exe as iis cgi

Quoted 11 lines · Florian Manschwetus

Quoted 11 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:

Quoted 6 lines

Quoted 6 lines

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

http-backend.c static struct rpc_service *select_service(const

Quoted 5 lines

http-backend.c index 8870a26..94976df renamed from http-backend.c +0 -0

100644

http-backend.c +0 -0

@@ -277,16 +277,32 @@ static struct rpc_service *select_service(const

char *name)

http-backend.c static struct rpc_service *select_service(const · line 279

Quoted 6 lines

http-backend.c +1 -2

  */
 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 <[email protected]>
Date: Tue, 29 Mar 2016 12:16:21 +0200
Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
CONTENT_LENGTH, violating rfc3875

From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
From: Florian Manschwetus <[email protected]>
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

http-backend.c +9 -0

+/**
+ * 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)

[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 <[email protected]>

Authored-by Florian Manschwetus <[email protected]>

Fixed-by Max Kirillov <[email protected]>

Signed-off-by Max Kirillov <[email protected]>

---

http-backend.c +5 -0

+       while (1) {
+               ssize_t cnt;
+
+               cnt = read_in_full(fd, buf + len, req_len - len);
+               if (cnt < 0) {

http-backend.c +6 -0

+       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);
+}

Quoted 1 line

+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/

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?

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.

Quoted 3 lines

Quoted 1 line

+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)

Quoted 10 lines · Max Kirillov

Quoted 3 lines · Eric Sunshine

Quoted 1 line

+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?

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 <[email protected]>

Authored-by Florian Manschwetus <[email protected]>

Fixed-by Max Kirillov <[email protected]>

Signed-off-by Max Kirillov <[email protected]>

---
...
I hope I marked it correctly in the trailers.

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

-----Ursprüngliche Nachricht-----
Von: Junio C Hamano [mailto:[email protected]]
Gesendet: Freitag, 24. November 2017 06:55
An: Max Kirillov
Cc: Jeff King; Florian Manschwetus; Chris Packham; Konstantin Khomoutov; [email protected]
Betreff: Re: [PATCH] http-backend: respect CONTENT_LENGTH as specified by rfc3875

Quoted 3 lines · Max Kirillov

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

IIS/Windows, for example.

Quoted 4 lines

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.

Quoted 2 lines

Signed-off-by Florian Manschwetus <manschwetus@cs-software-

gmbh.de>

Quoted 1 line

Authored-by Florian Manschwetus <manschwetus@cs-software-

gmbh.de>

Quoted 5 lines

Fixed-by Max Kirillov <[email protected]>

Signed-off-by Max Kirillov <[email protected]>

---
...
I hope I marked it correctly in the trailers.

It is probably more conventional to do it like so:

From: Florian Manschwetus <[email protected]>
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 <[email protected]>

---

Review

Quoted 6 lines

(quoted diff) +5 -0

+/*
+ * 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.

Quoted 1 line · Max Kirillov

I hope I marked it correctly in the trailers.

It is probably more conventional to do it like so:

From: Florian Manschwetus <[email protected]>
Date: <original date of Florian's patch series>

Signed-off-by Florian Manschwetus <[email protected]>

[mk: fixed trivial build failures and stuff]

Signed-off-by Max Kirillov <[email protected]>

Author: Florian Manschwetus <[email protected]>

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 <[email protected]>

[mk: fixed trivial build failures and polished style issues]

Signed-off-by Max Kirillov <[email protected]>

Review

Quoted 8 lines · Max Kirillov

(quoted diff) +8 -0

+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?

http-backend.c +9 -1

+{
+	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

Quoted 1 line · Max Kirillov

+static ssize_t env_content_length()

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

[...]
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. [...]

http-backend.c +6 -0

+       }
+
+       if (req_len <= 0) {
+               *out = NULL;
+               return 0;
+       }

http-backend.c +5 -0

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

From: Florian Manschwetus <[email protected]>
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.

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

http-backend.c +4 -0

+	unsigned char *buf = NULL;
+	ssize_t cnt = 0;
+
+	if (max_request_buffer < req_len) {

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.

Quoted 10 lines · Jeff King

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-<)?

... 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().

-----Ursprüngliche Nachricht-----
Von: Junio C Hamano [mailto:[email protected]]
Gesendet: Sonntag, 3. Dezember 2017 07:07
An: Jeff King
Cc: Max Kirillov; Eric Sunshine; Florian Manschwetus; Chris Packham; Konstantin Khomoutov; [email protected]
Betreff: Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

Quoted 5 lines · Jeff King

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

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.

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 <[email protected]>

---

t/t5560-http-backend-noserver.sh +10 -0

+       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
+'

t/helper/test-print-values.c line 9

Quoted 6 lines · Max Kirillov

t/helper/test-print-values.c new file +6 -0

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

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

Quoted 5 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.

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?

help.c +0 -0

index 88a3aeaeb9..9590eaba28 100644

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

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

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.

t/helper/test-print-values.c new file +0 -0

new file mode 100644
index 0000000000..8f7e5af319

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.

Quoted 3 lines · Junio C Hamano

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

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

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?

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

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

Quoted 2 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.

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

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)?

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 <[email protected]>

[mk: fixed trivial build failures and polished style issues]

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Max Kirillov <[email protected]>

---
 config.c       |  2 +-
 config.h       |  1 +
 http-backend.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 2 deletions(-)

http-backend.c static void copy_request(const char *prog_name, int out) · line 446

Quoted 3 lines · Max Kirillov

http-backend.c +3 -0

+		size_t n = xread(0, buf, chunk_length);
+		if (n < 0)
+			die_errno("Reading request failed");

n that is of type size_t is unsigned and cannot be negative here.

Quoted 6 lines · Junio C Hamano

http-backend.c static void copy_request(const char *prog_name, int out) · line 446

Quoted 3 lines · Max Kirillov

http-backend.c +3 -0

+		size_t n = xread(0, buf, chunk_length);
+		if (n < 0)
+			die_errno("Reading request failed");

n that is of type size_t is unsigned and cannot be negative here.

Thanks, fixing it
Do you think sanity check for n <= chunk_length (the code will go mad in this case) is needed? As far as I can see n returns straight from system's read()

Push passes to another commands, as described in
https://public-inbox.org/git/[email protected]/

As it gets complicated to correctly track the data length, instead transfer the data through parent process and cut the pipe as the specified length is reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input directly to the forked commands.

http-backend.c +0 -0

index 3066697a24..78a588c551 100644
@@ -351,23 +351,22 @@ static ssize_t get_content_length(void)
 	return val;
 }
 

http-backend.c +1 -2

@@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)
 			if (full_request)
 				n = 0; /* nothing left to read */
 			else
-				n = read_request(0, &full_request);
+				n = read_request(0, &full_request, req_len);
 			stream.next_in = full_request;
 		} else {
-			n = xread(0, in_buf, sizeof(in_buf));

t/helper/test-print-larger-than-ssize.c new file +1 -0

new file mode 100644
index 0000000000..83472a32f1
@@ -0,0 +1,11 @@
+#include "test-tool.h"

t/t5562-http-backend-content-length.sh new file +4 -0

+verify_http_result() {
+	# sometimes there is fatal error buit the result is still 200
+	if grep 'fatal:' act.err
+	then

Since this is slightly less efficient, and because it only matters if the web server does not already close the pipe, should this have a run-time configuration knob, even if it defaults to safe-but-slightly-slower?

http-backend.c static void copy_request(const char *prog_name, int out) · line 448

Quoted 2 lines

http-backend.c +2 -0

+		if (write_in_full(out, buf, n) < 0)
+			die_errno("%s aborted reading request", prog_name);

We don't necessarily know why the write failed. If it's EPIPE, then yes, the program probably did abort. But all we know is that write() failed. We should probably say something more generic like:

  die_errno("unable to write to '%s'");

or similar.

t/t5562-http-backend-content-length.sh line 39

Quoted 7 lines

t/t5562-http-backend-content-length.sh new file +7 -0

+test_expect_success 'setup repository' '
+	test_commit c0 &&
+	test_commit c1
+'
+
+hash_head=$(git rev-parse HEAD)
+hash_prev=$(git rev-parse HEAD~1)

We generally prefer to have all commands, even ones we don't expect to fail, inside test_expect blocks (e.g., with a "setup" description).

t/t5562-http-backend-content-length.sh line 45

Quoted 5 lines

t/t5562-http-backend-content-length.sh new file +5 -0

+cat >fetch_body <<EOF
+0032want $hash_head
+00000032have $hash_prev
+0009done
+EOF

This depends on the size of the hash. That's always 40 for now, but is something that may change soon.

We already have a packetize() helper; could we use it here?

t/t5562-http-backend-content-length.sh line 75

Quoted 5 lines

t/t5562-http-backend-content-length.sh new file +5 -0

+test_expect_success 'fetch plain truncated' '
+	test_http_env upload \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&
+	test_must_fail verify_http_result "200 OK"
+'

Usually test_must_fail on a checking function like this is a sign that the check is not as robust as we'd like. If the function checks two things "A && B", then checking test_must_fail will only let us know "!A || !B", but you probably want to check both.

(We'd also generally not use test_must_fail with a non-git command, and just use a simple "! verify_http_result"; that would apply equally if gets split into two commands).

Quoted 5 lines

t/t5562/invoke-with-content-length.pl new file +5 -0

+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+        close($out);
+        die "Command did not exit after reading whole body";
+}

Also, do we need to protect ourselves against other signals being delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is it going to erroneously end the sleep and say "nope, no exited signal"?

Quoted 4 lines · Jeff King

Since this is slightly less efficient, and because it only matters if the web server does not already close the pipe, should this have a run-time configuration knob, even if it defaults to safe-but-slightly-slower?

Personally, I of course don't want this. Also, I don't think the difference is much noticeable. But you can never be sure without trying. I'll try to measure some numbers.

Quoted 5 lines

We should probably say something more generic like:

  die_errno("unable to write to '%s'");

or similar.

Actually, it is already 3rd same error in this file. Maybe deserve some refactoring. I will change the message also.

Quoted 9 lines

t/t5562-http-backend-content-length.sh line 45

Quoted 5 lines

t/t5562-http-backend-content-length.sh new file +5 -0

+cat >fetch_body <<EOF
+0032want $hash_head
+00000032have $hash_prev
+0009done
+EOF

This depends on the size of the hash. That's always 40 for now, but is something that may change soon.

Quoted 1 line

We already have a packetize() helper; could we use it here?

Could you point me to it? I cannot find it.

Quoted 3 lines

Also, do we need to protect ourselves against other signals being delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is it going to erroneously end the sleep and say "nope, no exited signal"?

I'll check, but what could I do? Should I add blocking other signals there?

Quoted 5 lines · Max Kirillov

Since this is slightly less efficient, and because it only matters if the web server does not already close the pipe, should this have a run-time configuration knob, even if it defaults to safe-but-slightly-slower?

Personally, I of course don't want this. Also, I don't think the difference is much noticeable. But you can never be sure without trying. I'll try to measure some numbers.

Actually, it is already 3rd same error in this file. Maybe deserve some refactoring. I will change the message also.

Quoted 2 lines

We generally prefer to have all commands, even ones we don't expect to fail, inside test_expect blocks (e.g., with a "setup" description).

Will the defined variables get to the next test? I'll try to do as you describe.

Quoted 10 lines

t/t5562-http-backend-content-length.sh line 75

Quoted 5 lines

t/t5562-http-backend-content-length.sh new file +5 -0

+test_expect_success 'fetch plain truncated' '
+	test_http_env upload \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&
+	test_must_fail verify_http_result "200 OK"
+'

Usually test_must_fail on a checking function like this is a sign that the check is not as robust as we'd like. If the function checks two things "A && B", then checking test_must_fail will only let us know "!A || !B", but you probably want to check both.

Well here I just want to know that the request has failed, and we already know that it can fail in different ways, but the test is not going to differentiate those ways.

Quoted 9 lines

t/t5562/invoke-with-content-length.pl line 30

Quoted 5 lines

t/t5562/invoke-with-content-length.pl new file +5 -0

+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+        close($out);
+        die "Command did not exit after reading whole body";
+}

Also, do we need to protect ourselves against other signals being delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is it going to erroneously end the sleep and say "nope, no exited signal"?

I'll check, but what could I do? Should I add blocking other signals there?

Quoted 10 lines

Quoted 9 lines

t/t5562/invoke-with-content-length.pl line 30

Quoted 5 lines

t/t5562/invoke-with-content-length.pl new file +5 -0

+sleep 1; # is interrupted by SIGCHLD
+if (!$exited) {
+        close($out);
+        die "Command did not exit after reading whole body";
+}

Also, do we need to protect ourselves against other signals being delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is it going to erroneously end the sleep and say "nope, no exited signal"?

Quoted 2 lines

I'll check, but what could I do? Should I add blocking other signals there?

I think a more robust check may be to waitpid() on the child for up to N seconds. Something like this:

  $SIG{ALRM} = sub {
	  kill(9, $pid);
	  die "command did not exit after reading whole body"

};
alarm(60);
waitpid($pid, 0);
alarm(0);

That should exit immediately if $pid does, and otherwise die after exactly 60 seconds. Perl's waitpid implementation will restart automatically if it gets another signal.

t/t5560-http-backend-noserver.sh test_expect_success 'http-backend blocks bad PATH_INFO' ' · line 84

Quoted 9 lines · Max Kirillov

t/t5560-http-backend-noserver.sh +9 -0

+	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

I'm not sure if these messages should be marked for translation. If so, you'd want test_i18ngrep here.

Quoted 13 lines · Jeff King

t/t5560-http-backend-noserver.sh test_expect_success 'http-backend blocks bad PATH_INFO' ' · line 84

Quoted 9 lines · Max Kirillov

t/t5560-http-backend-noserver.sh +9 -0

+	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

I'm not sure if these messages should be marked for translation. If so, you'd want test_i18ngrep here.

Message localization does not seem to be used in http-backend at all. It makes sense - server-side software probably does not know who is the user on the other side, if the message gets to the user at all. So, I think the message should not be translated.

Push passes to another commands, as described in
https://public-inbox.org/git/[email protected]/

As it gets complicated to correctly track the data length, instead transfer the data through parent process and cut the pipe as the specified length is reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input directly to the forked commands.

Add tests for cases:

* CONTENT_LENGTH is set, script's stdin has more data, with all combinations of variations: fetch or push, plain or compressed body, correct or truncated input.

* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Max Kirillov <[email protected]>

---
 help.c                                 |   1 +
 http-backend.c                         |  32 ++++-
 t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++
 t/t5562/invoke-with-content-length.pl  |  37 ++++++
 4 files changed, 237 insertions(+), 2 deletions(-)
 create mode 100755 t/t5562-http-backend-content-length.sh
 create mode 100755 t/t5562/invoke-with-content-length.pl

t/t5562-http-backend-content-length.sh new file +0 -0

new file mode 100755
index 0000000000..8040d80e04

t/t5562-http-backend-content-length.sh new file +49 -0

+
+	if ! grep "Status" act.out >act
+	then
+		printf "Status: 200 OK\r\n" >act
+	fi
+	printf "Status: $1\r\n" >exp &&
+	test_cmp exp act
+}
+
+test_http_env() {
+	handler_type="$1"
+	shift
+	env \
+		CONTENT_TYPE="application/x-git-$handler_type-pack-request" \
+		QUERY_STRING="/repo.git/git-$handler_type-pack" \
+		PATH_TRANSLATED="$PWD/.git/git-$handler_type-pack" \
+		GIT_HTTP_EXPORT_ALL=TRUE \
+		REQUEST_METHOD=POST \
+		"$@"
+}
+
+ssize_b100dots() {
+	# hardcoded ((size_t) SSIZE_MAX) + 1
+	case "$(build_option sizeof-size_t)" in
+	8) echo 9223372036854775808;;
+	4) echo 2147483648;;
+	*) die "Unexpected ssize_t size: $(build_option sizeof-size_t)";;
+	esac
+}
+
+test_expect_success 'setup' '
+	git config http.receivepack true &&
+	test_commit c0 &&
+	test_commit c1 &&
+	hash_head=$(git rev-parse HEAD) &&
+	hash_prev=$(git rev-parse HEAD~1) &&
+	printf "want %s" "$hash_head" | packetize >fetch_body &&
+	printf 0000 >>fetch_body &&
+	printf "have %s" "$hash_prev" | packetize >>fetch_body &&
+	printf done | packetize >>fetch_body &&
+	test_copy_bytes 10 <fetch_body >fetch_body.trunc &&
+	hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&
+	printf "%s %s refs/heads/newbranch\\0report-status\\n" "$_z40" "$hash_next" | packetize >push_body &&
+	printf 0000 >>push_body &&
+	echo "$hash_next" | git pack-objects --stdout >>push_body &&
+	test_copy_bytes 10 <push_body >push_body.trunc &&
+	: >empty_body
+'
+

t/t5562-http-backend-content-length.sh new file +90 -0

+'
+
+test_expect_success 'fetch plain empty' '
+	test_http_env upload \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'fetch gzipped' '
+	test_http_env upload \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body.gz git http-backend >act.out 2>act.err &&
+	verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'fetch gzipped truncated' '
+	test_http_env upload \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body.gz.trunc git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'fetch gzipped empty' '
+	test_http_env upload \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'push plain' '
+	test_when_finished "git branch -D newbranch" &&
+	test_http_env receive \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl push_body git http-backend >act.out 2>act.err &&
+	verify_http_result "200 OK" &&
+	git rev-parse newbranch >act.head &&
+	echo "$hash_next" >exp.head &&
+	test_cmp act.head exp.head
+'
+
+test_expect_success 'push plain truncated' '
+	test_http_env receive \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl push_body.trunc git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success 'push plain empty' '
+	test_http_env receive \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'push gzipped' '
+	test_when_finished "git branch -D newbranch" &&
+	test_http_env receive \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl push_body.gz git http-backend >act.out 2>act.err &&
+	verify_http_result "200 OK" &&
+	git rev-parse newbranch >act.head &&
+	echo "$hash_next" >exp.head &&
+	test_cmp act.head exp.head
+'
+
+test_expect_success GZIP 'push gzipped truncated' '
+	test_http_env receive \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl push_body.gz.trunc git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success GZIP 'push gzipped empty' '
+	test_http_env receive \
+		HTTP_CONTENT_ENCODING="gzip" \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&
+	! verify_http_result "200 OK"
+'
+
+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
+	NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
+	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 "fatal:.*CONTENT_LENGTH" err
+'
+
+test_done

t/t5562/invoke-with-content-length.pl new file +37 -0

new file mode 100755
index 0000000000..6c2aae7692
@@ -0,0 +1,37 @@
+#!/usr/bin/perl
+use 5.008;
+use strict;
+use warnings;
+
+my $body_filename = $ARGV[0];
+my @command = @ARGV[1 .. $#ARGV];
+
+# read data
+my $body_size = -s $body_filename;
+$ENV{"CONTENT_LENGTH"} = $body_size;
+open(my $body_fh, "<", $body_filename) or die "Cannot open $body_filename: $!";
+my $body_data;
+defined read($body_fh, $body_data, $body_size) or die "Cannot read $body_filename: $!";
+close($body_fh);
+
+my $exited = 0;
+$SIG{"CHLD"} = sub {
+        $exited = 1;
+};
+
+# write data
+my $pid = open(my $out, "|-", @command);
+{
+        # disable buffering at $out
+        my $old_selected = select;
+        select $out;
+        $| = 1;
+        select $old_selected;
+}
+print $out $body_data or die "Cannot write data: $!";
+
+sleep 60; # is interrupted by SIGCHLD
+if (!$exited) {
+        close($out);
+        die "Command did not exit after reading whole body";
+}

Signature · 1 line

2.17.0.1185.g782057d875