From 6b1fae1dfbbdb6dc352567c0fc45a9e87474192d Mon Sep 17 00:00:00 2001 From: Max Kirillov Date: Sun, 10 Jun 2018 18:05:19 +0300 Subject: [PATCH 1/4] http-backend: cleanup writing to child process As explained in [1], we should not assume the reason why the writing has failed, and even if the reason is that child has existed not the reason why it have done so. So instead just say that writing has failed. [1] https://public-inbox.org/git/20180604044408.GD14451@sigill.intra.peff.net/ Signed-off-by: Max Kirillov Signed-off-by: Junio C Hamano --- http-backend.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/http-backend.c b/http-backend.c index adaef16fad..cefdfd6fc6 100644 --- a/http-backend.c +++ b/http-backend.c @@ -279,6 +279,12 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name) return svc; } +static void write_to_child(int out, const unsigned char *buf, ssize_t len, const char *prog_name) +{ + if (write_in_full(out, buf, len) < 0) + die("unable to write to '%s'", prog_name); +} + /* * This is basically strbuf_read(), except that if we * hit max_request_buffer we die (we'd rather reject a @@ -361,9 +367,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input) die("zlib error inflating request, result %d", ret); n = stream.total_out - cnt; - if (write_in_full(out, out_buf, n) < 0) - die("%s aborted reading request", prog_name); - cnt += n; + write_to_child(out, out_buf, stream.total_out - cnt, prog_name); + cnt = stream.total_out; if (ret == Z_STREAM_END) goto done; @@ -382,8 +387,7 @@ static void copy_request(const char *prog_name, int out) ssize_t n = read_request(0, &buf); if (n < 0) die_errno("error reading request body"); - if (write_in_full(out, buf, n) < 0) - die("%s aborted reading request", prog_name); + write_to_child(out, buf, n, prog_name); close(out); free(buf); } From c79edf73f4b018310428632887f9ce2ce32d839a Mon Sep 17 00:00:00 2001 From: Max Kirillov Date: Sun, 10 Jun 2018 18:05:20 +0300 Subject: [PATCH 2/4] 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. This commit only fixes buffered input, whcih reads whole body before processign it. Non-buffered input is going to be fixed in subsequent commit. Signed-off-by: Florian Manschwetus [mk: fixed trivial build failures and polished style issues] Helped-by: Junio C Hamano Signed-off-by: Max Kirillov Signed-off-by: Junio C Hamano --- config.c | 2 +- config.h | 1 + http-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/config.c b/config.c index fbbf0f8e9f..158afa858b 100644 --- a/config.c +++ b/config.c @@ -921,7 +921,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 cdac2fc73e..7808413bd0 100644 --- a/config.h +++ b/config.h @@ -73,6 +73,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 cefdfd6fc6..d0b6cb1b09 100644 --- a/http-backend.c +++ b/http-backend.c @@ -290,7 +290,7 @@ static void write_to_child(int out, const unsigned char *buf, ssize_t len, const * 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); @@ -327,7 +327,46 @@ static ssize_t read_request(int fd, unsigned char **out) } } -static void inflate_request(const char *prog_name, int out, int buffer_input) +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): " + "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER", + max_request_buffer, (uintmax_t)req_len); + } + + buf = xmalloc(req_len); + cnt = read_in_full(fd, buf, req_len); + if (cnt < 0) { + free(buf); + return -1; + } + *out = buf; + return cnt; +} + +static ssize_t get_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) +{ + 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, ssize_t req_len) { git_zstream stream; unsigned char *full_request = NULL; @@ -345,7 +384,7 @@ 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)); @@ -381,10 +420,10 @@ done: free(full_request); } -static void copy_request(const char *prog_name, int out) +static void copy_request(const char *prog_name, int out, ssize_t req_len) { unsigned char *buf; - ssize_t n = read_request(0, &buf); + ssize_t n = read_request(0, &buf, req_len); if (n < 0) die_errno("error reading request body"); write_to_child(out, buf, n, prog_name); @@ -399,6 +438,7 @@ static void run_service(const char **argv, int buffer_input) const char *host = getenv("REMOTE_ADDR"); int gzipped_request = 0; struct child_process cld = CHILD_PROCESS_INIT; + ssize_t req_len = get_content_length(); if (encoding && !strcmp(encoding, "gzip")) gzipped_request = 1; @@ -425,9 +465,9 @@ static void run_service(const char **argv, int buffer_input) close(1); if (gzipped_request) - inflate_request(argv[0], cld.in, buffer_input); + inflate_request(argv[0], cld.in, buffer_input, req_len); else if (buffer_input) - copy_request(argv[0], cld.in); + copy_request(argv[0], cld.in, req_len); else close(0); From 6c213e863aeb0af078bf82deefb22da20427c2ab Mon Sep 17 00:00:00 2001 From: Max Kirillov Date: Fri, 27 Jul 2018 06:48:59 +0300 Subject: [PATCH 3/4] http-backend: respect CONTENT_LENGTH for receive-pack 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 Signed-off-by: Max Kirillov Signed-off-by: Junio C Hamano --- help.c | 1 + http-backend.c | 32 ++++- t/t5562-http-backend-content-length.sh | 155 +++++++++++++++++++++++++ t/t5562/invoke-with-content-length.pl | 37 ++++++ 4 files changed, 223 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 dd35fcc133..e469f5731c 100644 --- a/help.c +++ b/help.c @@ -609,6 +609,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 d0b6cb1b09..e88d29f62b 100644 --- a/http-backend.c +++ b/http-backend.c @@ -373,6 +373,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); @@ -387,8 +389,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) @@ -431,6 +440,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"); @@ -457,7 +483,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)) @@ -468,6 +494,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..057dcb85d6 --- /dev/null +++ b/t/t5562-http-backend-content-length.sh @@ -0,0 +1,155 @@ +#!/bin/sh + +test_description='test git-http-backend respects CONTENT_LENGTH' +. ./test-lib.sh + +test_lazy_prereq GZIP 'gzip --version' + +verify_http_result() { + # some fatal errors still produce status 200 + # so check if there is the error message + 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" + request_body="$2" + 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 \ + "$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl \ + "$request_body" git http-backend >act.out 2>act.err +} + +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' ' + export HTTP_CONTENT_ENCODING="identity" && + 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.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.trunc && + : >empty_body +' + +test_expect_success GZIP 'setup, compression related' ' + gzip -c fetch_body >fetch_body.gz && + test_copy_bytes 10 fetch_body.gz.trunc && + gzip -c push_body >push_body.gz && + test_copy_bytes 10 push_body.gz.trunc +' + +test_expect_success 'fetch plain' ' + test_http_env upload fetch_body && + verify_http_result "200 OK" +' + +test_expect_success 'fetch plain truncated' ' + test_http_env upload fetch_body.trunc && + ! verify_http_result "200 OK" +' + +test_expect_success 'fetch plain empty' ' + test_http_env upload empty_body && + ! verify_http_result "200 OK" +' + +test_expect_success GZIP 'fetch gzipped' ' + test_env HTTP_CONTENT_ENCODING="gzip" test_http_env upload fetch_body.gz && + verify_http_result "200 OK" +' + +test_expect_success GZIP 'fetch gzipped truncated' ' + test_env HTTP_CONTENT_ENCODING="gzip" test_http_env upload fetch_body.gz.trunc && + ! verify_http_result "200 OK" +' + +test_expect_success GZIP 'fetch gzipped empty' ' + test_env HTTP_CONTENT_ENCODING="gzip" test_http_env upload empty_body && + ! verify_http_result "200 OK" +' + +test_expect_success GZIP 'push plain' ' + test_when_finished "git branch -D newbranch" && + test_http_env receive push_body && + 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 push_body.trunc && + ! verify_http_result "200 OK" +' + +test_expect_success 'push plain empty' ' + test_http_env receive empty_body && + ! verify_http_result "200 OK" +' + +test_expect_success GZIP 'push gzipped' ' + test_when_finished "git branch -D newbranch" && + test_env HTTP_CONTENT_ENCODING="gzip" test_http_env receive push_body.gz && + 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_env HTTP_CONTENT_ENCODING="gzip" test_http_env receive push_body.gz.trunc && + ! verify_http_result "200 OK" +' + +test_expect_success GZIP 'push gzipped empty' ' + test_env HTTP_CONTENT_ENCODING="gzip" test_http_env receive empty_body && + ! 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/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"; +} From eebfe409628e337e283d57a870f52ae0d0e0de34 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Sat, 28 Jul 2018 23:51:28 +0100 Subject: [PATCH 4/4] t5562: avoid non-portable "export FOO=bar" construct Commit 6c213e863a ("http-backend: respect CONTENT_LENGTH for receive-pack", 2018-07-27) adds a test which uses the non-portable export construct. Replace it with "FOO=bar && export FOO" instead. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- t/t5562-http-backend-content-length.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh index 057dcb85d6..43570ce120 100755 --- a/t/t5562-http-backend-content-length.sh +++ b/t/t5562-http-backend-content-length.sh @@ -45,7 +45,8 @@ ssize_b100dots() { } test_expect_success 'setup' ' - export HTTP_CONTENT_ENCODING="identity" && + HTTP_CONTENT_ENCODING="identity" && + export HTTP_CONTENT_ENCODING && git config http.receivepack true && test_commit c0 && test_commit c1 &&