[PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[Max Kirillov]

Push passes to another commands, as described in
https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/

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

Add tests for cases:

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

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Max Kirillov <max@max630.net>
---
 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

diff --git a/help.c b/help.c
index 60071a9bea..42600ca026 100644
--- a/help.c
+++ b/help.c
@@ -419,6 +419,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 		else
 			printf("no commit associated with this build\n");
 		printf("sizeof-long: %d\n", (int)sizeof(long));
+		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
 		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
 	}
 	return 0;
diff --git a/http-backend.c b/http-backend.c
index 0c9e9be2b7..28c07e7c2a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -372,6 +372,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss
 	unsigned char in_buf[8192];
 	unsigned char out_buf[8192];
 	unsigned long cnt = 0;
+	int req_len_defined = req_len >= 0;
+	size_t req_remaining_len = req_len;
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init_gzip_only(&stream);
@@ -386,8 +388,15 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss
 				n = read_request(0, &full_request, req_len);
 			stream.next_in = full_request;
 		} else {
-			n = xread(0, in_buf, sizeof(in_buf));
+			ssize_t buffer_len;
+			if (req_len_defined && req_remaining_len <= sizeof(in_buf))
+				buffer_len = req_remaining_len;
+			else
+				buffer_len = sizeof(in_buf);
+			n = xread(0, in_buf, buffer_len);
 			stream.next_in = in_buf;
+			if (req_len_defined && n > 0)
+				req_remaining_len -= n;
 		}
 
 		if (n <= 0)
@@ -430,6 +439,23 @@ static void copy_request(const char *prog_name, int out, ssize_t req_len)
 	free(buf);
 }
 
+static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)
+{
+	unsigned char buf[8192];
+	size_t remaining_len = req_len;
+
+	while (remaining_len > 0) {
+		size_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;
+		ssize_t n = xread(0, buf, chunk_length);
+		if (n < 0)
+			die_errno("Reading request failed");
+		write_to_child(out, buf, n, prog_name);
+		remaining_len -= n;
+	}
+
+	close(out);
+}
+
 static void run_service(const char **argv, int buffer_input)
 {
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
@@ -456,7 +482,7 @@ static void run_service(const char **argv, int buffer_input)
 				 "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
 
 	cld.argv = argv;
-	if (buffer_input || gzipped_request)
+	if (buffer_input || gzipped_request || req_len >= 0)
 		cld.in = -1;
 	cld.git_cmd = 1;
 	if (start_command(&cld))
@@ -467,6 +493,8 @@ static void run_service(const char **argv, int buffer_input)
 		inflate_request(argv[0], cld.in, buffer_input, req_len);
 	else if (buffer_input)
 		copy_request(argv[0], cld.in, req_len);
+	else if (req_len >= 0)
+		pipe_fixed_length(argv[0], cld.in, req_len);
 	else
 		close(0);
 
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
new file mode 100755
index 0000000000..8040d80e04
--- /dev/null
+++ b/t/t5562-http-backend-content-length.sh
@@ -0,0 +1,169 @@
+#!/bin/sh
+
+test_description='test git-http-backend respects CONTENT_LENGTH'
+. ./test-lib.sh
+
+test_lazy_prereq GZIP 'gzip --version'
+
+verify_http_result() {
+	# sometimes there is fatal error buit the result is still 200
+	if grep 'fatal:' act.err
+	then
+		return 1
+	fi
+
+	if ! grep "Status" act.out >act
+	then
+		printf "Status: 200 OK\r\n" >act
+	fi
+	printf "Status: $1\r\n" >exp &&
+	test_cmp exp act
+}
+
+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
+'
+
+test_expect_success GZIP 'setup, compression related' '
+	gzip -k fetch_body &&
+	test_copy_bytes 10 <fetch_body.gz >fetch_body.gz.trunc &&
+	gzip -k push_body &&
+	test_copy_bytes 10 <push_body.gz >push_body.gz.trunc
+'
+
+test_expect_success 'fetch plain' '
+	test_http_env upload \
+		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&
+	verify_http_result "200 OK"
+'
+
+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 &&
+	! verify_http_result "200 OK"
+'
+
+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
diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl
new file mode 100755
index 0000000000..6c2aae7692
--- /dev/null
+++ b/t/t5562/invoke-with-content-length.pl
@@ -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";
+}
-- 
2.17.0.1185.g782057d875

Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[SZEDER Gábor]

> Push passes to another commands, as described in
> https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
> 
> As it gets complicated to correctly track the data length, instead transfer
> the data through parent process and cut the pipe as the specified length is
> reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
> directly to the forked commands.
> 
> Add tests for cases:
> 
> * CONTENT_LENGTH is set, script's stdin has more data, with all combinations
>   of variations: fetch or push, plain or compressed body, correct or truncated
>   input.
> 
> * CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  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


> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> new file mode 100755
> index 0000000000..8040d80e04
> --- /dev/null
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -0,0 +1,169 @@
> +#!/bin/sh
> +
> +test_description='test git-http-backend respects CONTENT_LENGTH'
> +. ./test-lib.sh
> +
> +test_lazy_prereq GZIP 'gzip --version'
> +
> +verify_http_result() {
> +	# sometimes there is fatal error buit the result is still 200

s/buit/but/

> +	if grep 'fatal:' act.err
> +	then
> +		return 1
> +	fi

I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.

What does that "sometimes" in the above comment mean, and how often
does such a failure happen?  I see these patches are in 'pu' for over
a month now, so based on the number of reflog entries since then it
happened once from about 30-35 builds on Travis CI so far.

I don't really like the idea of adding a bunch of flaky test cases...
we have enough of them already, unfortunately.

> +
> +	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
> +'
> +
> +test_expect_success GZIP 'setup, compression related' '
> +	gzip -k fetch_body &&
> +	test_copy_bytes 10 <fetch_body.gz >fetch_body.gz.trunc &&
> +	gzip -k push_body &&
> +	test_copy_bytes 10 <push_body.gz >push_body.gz.trunc
> +'
> +
> +test_expect_success 'fetch plain' '
> +	test_http_env upload \
> +		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&

Don't save the standard error of the whole shell function.
When running the test with /bin/sh and '-x' tracing, then the trace of
commands executed in the function will be included in the standard
error as well, which may interfere with later verification (though in
this case it doesn't seem like it would cause any issues).

Please limit the redirections to the relevant command's output.  AFAICT
all invocations of 'test_http_env' in these tests have their stdout and
stderr redirected to the same pair of files, so perhaps you could
simply move all these redirections inside the function.

> +	verify_http_result "200 OK"
> +'
> +
> +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 &&

If this command were to print a "fatal: ..." message to its standard
error, then ...

> +	! verify_http_result "200 OK"

... this function would return error (because of that 'if grep fatal:
...' statement) without even looking at the status, but the test would
still succeed.  Is that really the desired behavior here?

> +'
> +
> +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
> diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl
> new file mode 100755
> index 0000000000..6c2aae7692
> --- /dev/null
> +++ b/t/t5562/invoke-with-content-length.pl
> @@ -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";
> +}
> -- 
> 2.17.0.1185.g782057d875
> 
> 

Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[SZEDER Gábor]

[Hrm, this time with hopefully proper In-Reply-To: header.
 Sorry for the double post.]


> Push passes to another commands, as described in
> https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/
> 
> As it gets complicated to correctly track the data length, instead transfer
> the data through parent process and cut the pipe as the specified length is
> reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input
> directly to the forked commands.
> 
> Add tests for cases:
> 
> * CONTENT_LENGTH is set, script's stdin has more data, with all combinations
>   of variations: fetch or push, plain or compressed body, correct or truncated
>   input.
> 
> * CONTENT_LENGTH is specified to a value which does not fit into ssize_t.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  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


> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> new file mode 100755
> index 0000000000..8040d80e04
> --- /dev/null
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -0,0 +1,169 @@
> +#!/bin/sh
> +
> +test_description='test git-http-backend respects CONTENT_LENGTH'
> +. ./test-lib.sh
> +
> +test_lazy_prereq GZIP 'gzip --version'
> +
> +verify_http_result() {
> +	# sometimes there is fatal error buit the result is still 200

s/buit/but/

> +	if grep 'fatal:' act.err
> +	then
> +		return 1
> +	fi

I just happened to stumble upon a failure because of 'fatal: the
remote end hung up unexpectedly' in the test 'push plain'.

What does that "sometimes" in the above comment mean, and how often
does such a failure happen?  I see these patches are in 'pu' for over
a month now, so based on the number of reflog entries since then it
happened once from about 30-35 builds on Travis CI so far.

I don't really like the idea of adding a bunch of flaky test cases...
we have enough of them already, unfortunately.

> +
> +	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
> +'
> +
> +test_expect_success GZIP 'setup, compression related' '
> +	gzip -k fetch_body &&
> +	test_copy_bytes 10 <fetch_body.gz >fetch_body.gz.trunc &&
> +	gzip -k push_body &&
> +	test_copy_bytes 10 <push_body.gz >push_body.gz.trunc
> +'
> +
> +test_expect_success 'fetch plain' '
> +	test_http_env upload \
> +		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&

Don't save the standard error of the whole shell function.
When running the test with /bin/sh and '-x' tracing, then the trace of
commands executed in the function will be included in the standard
error as well, which may interfere with later verification (though in
this case it doesn't seem like it would cause any issues).

Please limit the redirections to the relevant command's output.  AFAICT
all invocations of 'test_http_env' in these tests have their stdout and
stderr redirected to the same pair of files, so perhaps you could
simply move all these redirections inside the function.

> +	verify_http_result "200 OK"
> +'
> +
> +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 &&

If this command were to print a "fatal: ..." message to its standard
error, then ...

> +	! verify_http_result "200 OK"

... this function would return error (because of that 'if grep fatal:
...' statement) without even looking at the status, but the test would
still succeed.  Is that really the desired behavior here?

> +'
> +
> +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
> diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl
> new file mode 100755
> index 0000000000..6c2aae7692
> --- /dev/null
> +++ b/t/t5562/invoke-with-content-length.pl
> @@ -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";
> +}
> -- 
> 2.17.0.1185.g782057d875
> 
> 

Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[Max Kirillov]

On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:
>> +	# sometimes there is fatal error buit the result is still 200
> 
> s/buit/but/

Thanks, will fix

>> +	if grep 'fatal:' act.err
>> +	then
>> +		return 1
>> +	fi
> 
> I just happened to stumble upon a failure because of 'fatal: the
> remote end hung up unexpectedly' in the test 'push plain'.

Did it happen once or repeated? It is rather strange, that
one shoud not fail. Which OS it was?

There have been doubds that a random incoming signal can
trigger such a failure.

> What does that "sometimes" in the above comment mean, and how often
> does such a failure happen?  I see these patches are in 'pu' for over
> a month now, so based on the number of reflog entries since then it
> happened once from about 30-35 builds on Travis CI so far.

"sometimes" here means "for some kinds of fatal error
failure", there is nothing random in it.

> > +		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&
> 
> Don't save the standard error of the whole shell function.
> When running the test with /bin/sh and '-x' tracing, then the trace of
> commands executed in the function will be included in the standard
> error as well, which may interfere with later verification (though in
> this case it doesn't seem like it would cause any issues).
> 
> Please limit the redirections to the relevant command's output.  AFAICT
> all invocations of 'test_http_env' in these tests have their stdout and
> stderr redirected to the same pair of files, so perhaps you could
> simply move all these redirections inside the function.

Thanks, I'll try to fix it 

>> +	! verify_http_result "200 OK"
> 
> ... this function would return error (because of that 'if grep fatal:
> ...' statement) without even looking at the status, but the test would
> still succeed.  Is that really the desired behavior here?

Yes, it is a desired behavior. A failure is expected here,
and the failure does not show up as non-200 status, as
described above.

Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[SZEDER Gábor]

On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov <max@max630.net> wrote:
>
> On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:
> >> +    # sometimes there is fatal error buit the result is still 200

> >> +    if grep 'fatal:' act.err
> >> +    then
> >> +            return 1
> >> +    fi
> >
> > I just happened to stumble upon a failure because of 'fatal: the
> > remote end hung up unexpectedly' in the test 'push plain'.
>
> Did it happen once or repeated? It is rather strange, that
> one shoud not fail. Which OS it was?

Only once, so far.  It was one of my OSX build jobs on Travis CI, but
I don't know what OSX version is used.

'act.err' contained this (which will get line-wrapped, I'm afraid):

++handler_type=receive
++shift
++env CONTENT_TYPE=application/x-git-receive-pack-request
QUERY_STRING=/repo.git/git-receive-pack
'PATH_TRANSLATED=/Users/travis/t/trash
dir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE
REQUEST_METHOD=POST
/Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl
push_body git http-backend
<...128 zero bytes...>fatal: the remote end hung up unexpectedly

I couldn't reproduce it on my Linux box.

> There have been doubds that a random incoming signal can
> trigger such a failure.
>
> > What does that "sometimes" in the above comment mean, and how often
> > does such a failure happen?  I see these patches are in 'pu' for over
> > a month now, so based on the number of reflog entries since then it
> > happened once from about 30-35 builds on Travis CI so far.
>
> "sometimes" here means "for some kinds of fatal error
> failure", there is nothing random in it.

> >> +    ! verify_http_result "200 OK"
> >
> > ... this function would return error (because of that 'if grep fatal:
> > ...' statement) without even looking at the status, but the test would
> > still succeed.  Is that really the desired behavior here?
>
> Yes, it is a desired behavior. A failure is expected here,
> and the failure does not show up as non-200 status, as
> described above.

OK, then I misunderstood that comment.

Perhaps a different wording could make it slightly better?  E.g. "In
some of these tests ..." instead of that "sometimes".  Dunno.

Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

[Max Kirillov]

On Wed, Jul 25, 2018 at 08:41:31PM +0200, SZEDER Gábor wrote:
> On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov <max@max630.net> wrote:
>>> I just happened to stumble upon a failure because of 'fatal: the
>>> remote end hung up unexpectedly' in the test 'push plain'.
>>
>> Did it happen once or repeated? It is rather strange, that
>> one shoud not fail. Which OS it was?
> 
> Only once, so far.  It was one of my OSX build jobs on Travis CI, but
> I don't know what OSX version is used.
> 
> 'act.err' contained this (which will get line-wrapped, I'm afraid):
> 
> ++handler_type=receive
> ++shift
> ++env CONTENT_TYPE=application/x-git-receive-pack-request
> QUERY_STRING=/repo.git/git-receive-pack
> 'PATH_TRANSLATED=/Users/travis/t/trash
> dir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE
> REQUEST_METHOD=POST
> /Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl
> push_body git http-backend
> <...128 zero bytes...>fatal: the remote end hung up unexpectedly
> 
> I couldn't reproduce it on my Linux box.

The only reason for this I could imagine is some perl
utility failure to feed the body to git http-backend.
I could not reproduce it either, but if such things happen
often again maybe should concider C helper instead. Though
I'm afraid I easily can make more mistakes in it than perl
interpreter authors.

I'll make the other changes, and sofar just hope it would
not happen again.