{"thread":{"id":"48630","subject":"[PATCH v7 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","startedAt":"2018-06-02T21:39:37Z","lastAt":"2018-08-04T17:20:32Z","messageCount":31,"participants":["Max Kirillov","Jeff King","Junio C Hamano","Ramsay Jones","SZEDER Gábor","Duy Nguyen"],"isPatch":true,"patchVersion":7,"patchTotal":2},"messages":[{"id":"349101","messageId":"20180602212749.21324-1-max@max630.net","threadId":"48630","inReplyTo":null,"subject":"[PATCH v7 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-02T21:27:47Z","receivedAt":"2018-06-02T21:39:37Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"It's been time. Thank you for parience.\n\nChanges:\n\n* did most of the changes proposed\n* rebase to newer master (latest conflicting change is addition of combined test helper)\n* make tests which cover, hopefully, all cases.\n* handle incorectly truncated input also in receive-pack. Considering the complications\n pointed out by Jeff, it just filters the input in the frontend process. I hope it\n is acceptable thing to do.\n\nMax Kirillov (2):\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n http-backend: respect CONTENT_LENGTH for receive-pack\n\n Makefile | 1 +\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 86 +++++++++++--\n t/helper/test-print-larger-than-ssize.c | 11 ++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5560-http-backend-noserver.sh | 13 ++\n t/t5562-http-backend-content-length.sh | 155 ++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 30 +++++\n 10 files changed, 291 insertions(+), 10 deletions(-)\n create mode 100644 t/helper/test-print-larger-than-ssize.c\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349102","messageId":"20180602212749.21324-2-max@max630.net","threadId":"48630","inReplyTo":"20180602212749.21324-1-max@max630.net","subject":"[PATCH v7 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-02T21:27:48Z","receivedAt":"2018-06-02T21:39:37Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"http-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \n---\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 43 ++++++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c698988f5e..4148a3529d 100644\n--- a/config.c\n+++ b/config.c\n@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n-static int git_parse_ssize_t(const char *value, ssize_t *ret)\n+int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\ndiff --git a/config.h b/config.h\nindex ef70a9cac1..c143a1b634 100644\n--- a/config.h\n+++ b/config.h\n@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);\n extern int config_with_options(config_fn_t fn, void *,\n \t\t\t struct git_config_source *config_source,\n \t\t\t const struct config_options *opts);\n+extern int git_parse_ssize_t(const char *, ssize_t *);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/http-backend.c b/http-backend.c\nindex 88d2a9bc40..3066697a24 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -283,7 +283,7 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -320,6 +320,47 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): \"\n+\t\t \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t max_request_buffer, (uintmax_t)req_len);\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\t*out = buf;\n+\treturn cnt;\n+}\n+\n+static ssize_t get_content_length(void)\n+{\n+\tssize_t val = -1;\n+\tconst char *str = getenv(\"CONTENT_LENGTH\");\n+\n+\tif (str && !git_parse_ssize_t(str, &val))\n+\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\treturn val;\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tssize_t req_len = get_content_length();\n+\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349103","messageId":"20180602212749.21324-3-max@max630.net","threadId":"48630","inReplyTo":"20180602212749.21324-1-max@max630.net","subject":"[PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-02T21:27:49Z","receivedAt":"2018-06-02T21:39:38Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Push passes to another commands, as described in\nhttps://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/\n\nAs it gets complicated to correctly track the data length, instead transfer\nthe data through parent process and cut the pipe as the specified length is\nreached. Do it only when CONTENT_LENGTH is set, otherwise pass the input\ndirectly to the forked commands.\n\nAdd tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data, with all combinations\n of variations: fetch or push, plain or compressed body, correct or truncated\n input.\n\n* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \n---\n Makefile | 1 +\n http-backend.c | 49 ++++++--\n t/helper/test-print-larger-than-ssize.c | 11 ++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5560-http-backend-noserver.sh | 13 ++\n t/t5562-http-backend-content-length.sh | 155 ++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 30 +++++\n 8 files changed, 250 insertions(+), 11 deletions(-)\n create mode 100644 t/helper/test-print-larger-than-ssize.c\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\ndiff --git a/Makefile b/Makefile\nindex f181687250..93dc4bc23b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -678,6 +678,7 @@ TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-path-utils.o\n+TEST_BUILTINS_OBJS += test-print-larger-than-ssize.o\n TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-ref-store.o\ndiff --git a/http-backend.c b/http-backend.c\nindex 3066697a24..78a588c551 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -351,23 +351,22 @@ static ssize_t get_content_length(void)\n \treturn val;\n }\n \n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request(int fd, unsigned char **out, ssize_t req_len)\n {\n-\tssize_t req_len = get_content_length();\n-\n \tif (req_len < 0)\n \t\treturn read_request_eof(fd, out);\n \telse\n \t\treturn read_request_fixed_len(fd, req_len, out);\n }\n \n-static void inflate_request(const char *prog_name, int out, int buffer_input)\n+static void inflate_request(const char *prog_name, int out, int buffer_input, ssize_t req_len)\n {\n \tgit_zstream stream;\n \tunsigned char *full_request = NULL;\n \tunsigned char in_buf[8192];\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n+\tssize_t req_remaining_len = req_len;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init_gzip_only(&stream);\n@@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\tif (full_request)\n \t\t\t\tn = 0; /* nothing left to read */\n \t\t\telse\n-\t\t\t\tn = read_request(0, &full_request);\n+\t\t\t\tn = read_request(0, &full_request, req_len);\n \t\t\tstream.next_in = full_request;\n \t\t} else {\n-\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n+\t\t\tssize_t buffer_len;\n+\t\t\tif (req_remaining_len < 0 || req_remaining_len > sizeof(in_buf))\n+\t\t\t buffer_len = sizeof(in_buf);\n+\t\t\telse\n+\t\t\t buffer_len = req_remaining_len;\n+\t\t\tn = xread(0, in_buf, buffer_len);\n \t\t\tstream.next_in = in_buf;\n+\t\t\tif (req_remaining_len >= 0)\n+\t\t\t\treq_remaining_len -= n;\n \t\t}\n \n \t\tif (n <= 0)\n@@ -416,10 +422,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \tfree(full_request);\n }\n \n-static void copy_request(const char *prog_name, int out)\n+static void copy_request(const char *prog_name, int out, ssize_t req_len)\n {\n \tunsigned char *buf;\n-\tssize_t n = read_request(0, &buf);\n+\tssize_t n = read_request(0, &buf, req_len);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n \tif (write_in_full(out, buf, n) < 0)\n@@ -428,6 +434,24 @@ static void copy_request(const char *prog_name, int out)\n \tfree(buf);\n }\n \n+static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)\n+{\n+\tunsigned char buf[8192];\n+\tsize_t remaining_len = req_len;\n+\n+\twhile (remaining_len > 0) {\n+\t\tsize_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;\n+\t\tsize_t n = xread(0, buf, chunk_length);\n+\t\tif (n < 0)\n+\t\t\tdie_errno(\"Reading request failed\");\n+\t\tif (write_in_full(out, buf, n) < 0)\n+\t\t\tdie_errno(\"%s aborted reading request\", prog_name);\n+\t\tremaining_len -= n;\n+\t}\n+\n+\tclose(out);\n+}\n+\n static void run_service(const char **argv, int buffer_input)\n {\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n@@ -435,6 +459,7 @@ static void run_service(const char **argv, int buffer_input)\n \tconst char *host = getenv(\"REMOTE_ADDR\");\n \tint gzipped_request = 0;\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n+\tssize_t req_len = get_content_length();\n \n \tif (encoding && !strcmp(encoding, \"gzip\"))\n \t\tgzipped_request = 1;\n@@ -453,7 +478,7 @@ static void run_service(const char **argv, int buffer_input)\n \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tif (buffer_input || gzipped_request)\n+\tif (buffer_input || gzipped_request || req_len >= 0)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n \tif (start_command(&cld))\n@@ -461,9 +486,11 @@ static void run_service(const char **argv, int buffer_input)\n \n \tclose(1);\n \tif (gzipped_request)\n-\t\tinflate_request(argv[0], cld.in, buffer_input);\n+\t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n-\t\tcopy_request(argv[0], cld.in);\n+\t\tcopy_request(argv[0], cld.in, req_len);\n+\telse if (req_len >= 0)\n+\t\tpipe_fixed_length(argv[0], cld.in, req_len);\n \telse\n \t\tclose(0);\n \ndiff --git a/t/helper/test-print-larger-than-ssize.c b/t/helper/test-print-larger-than-ssize.c\nnew file mode 100644\nindex 0000000000..83472a32f1\n--- /dev/null\n+++ b/t/helper/test-print-larger-than-ssize.c\n@@ -0,0 +1,11 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+\n+int cmd__print_larger_than_ssize(int argc, const char **argv)\n+{\n+\tsize_t large = ~0;\n+\n+\tlarge = ~(large & ~(large >> 1)) + 1;\n+\tprintf(\"%\" PRIuMAX \"\\n\", (uintmax_t) large);\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 87066ced62..edcfe5df63 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -25,6 +25,7 @@ static struct test_cmd cmds[] = {\n \t{ \"mktemp\", cmd__mktemp },\n \t{ \"online-cpus\", cmd__online_cpus },\n \t{ \"path-utils\", cmd__path_utils },\n+ { \"print-larger-than-ssize\", cmd__print_larger_than_ssize },\n \t{ \"prio-queue\", cmd__prio_queue },\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"ref-store\", cmd__ref_store },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb94..c2aa0803d2 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -19,6 +19,7 @@ int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__path_utils(int argc, const char **argv);\n+int cmd__print_larger_than_ssize(int argc, const char **argv);\n int cmd__prio_queue(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9fafcf1945..8c212393b4 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -71,4 +71,17 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n \texpect_aliased 1 //domain/data.txt\n '\n \n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=$(\"$GIT_BUILD_DIR/t/helper/test-tool\" print-larger-than-ssize) &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n test_done\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nnew file mode 100755\nindex 0000000000..6b0c005db0\n--- /dev/null\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -0,0 +1,155 @@\n+#!/bin/sh\n+\n+test_description='test git-http-backend respects CONTENT_LENGTH'\n+. ./test-lib.sh\n+\n+verify_http_result() {\n+\t# sometimes there is fatal error buit the result is still 200\n+\tif grep 'fatal:' act.err\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tif ! grep \"Status\" act.out >act\n+\tthen\n+\t\tprintf \"Status: 200 OK\\r\\n\" >act\n+\tfi\n+\tprintf \"Status: $1\\r\\n\" >exp &&\n+\ttest_cmp exp act\n+}\n+\n+test_http_env() {\n+\thandler_type=\"$1\"\n+\tshift\n+\tenv \\\n+\t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n+\t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\t\"$@\"\n+}\n+\n+test_expect_success 'setup repository' '\n+\ttest_commit c0 &&\n+\ttest_commit c1\n+'\n+\n+hash_head=$(git rev-parse HEAD)\n+hash_prev=$(git rev-parse HEAD~1)\n+\n+cat >fetch_body <fetch_body.gz.trunc\n+\n+head -c -10 fetch_body.trunc\n+\n+hash_next=$(git commit-tree -p HEAD -m next HEAD^{tree})\n+printf '00790000000000000000000000000000000000000000 %s refs/heads/newbranch\\0report-status\\n0000' \"$hash_next\" >push_body\n+echo \"$hash_next\" | git pack-objects --stdout >>push_body\n+\n+gzip -k push_body\n+\n+head -c -10 push_body.gz.trunc\n+\n+head -c -10 push_body.trunc\n+\n+touch empty_body\n+\n+test_expect_success 'fetch plain' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain truncated' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain empty' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch gzipped' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch gzipped truncated' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz.trunc git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch gzipped empty' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push plain' '\n+\tgit config http.receivepack true &&\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head &&\n+\tgit branch -D newbranch\n+'\n+\n+\n+test_expect_success 'push plain truncated' '\n+\tgit config http.receivepack true &&\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.trunc git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push plain empty' '\n+\tgit config http.receivepack true &&\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push gzipped' '\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head &&\n+\tgit branch -D newbranch\n+'\n+\n+test_expect_success 'push gzipped truncated' '\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz.trunc git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push gzipped empty' '\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\ttest_must_fail verify_http_result \"200 OK\"\n+'\n+\n+test_done\ndiff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl\nnew file mode 100755\nindex 0000000000..7f84242e77\n--- /dev/null\n+++ b/t/t5562/invoke-with-content-length.pl\n@@ -0,0 +1,30 @@\n+#!/usr/bin/perl\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+my $body_filename = $ARGV[0];\n+my @command = @ARGV[1 .. $#ARGV];\n+\n+# read data\n+my $body_size = -s $body_filename;\n+$ENV{\"CONTENT_LENGTH\"} = $body_size;\n+open(my $body_fh, \"<\", $body_filename) or die \"Cannot open $body_filename: $!\";\n+my $body_data;\n+defined read($body_fh, $body_data, $body_size) or die \"Cannot read $body_filename: $!\";\n+close($body_fh);\n+\n+my $exited = 0;\n+$SIG{\"CHLD\"} = sub {\n+ $exited = 1;\n+};\n+\n+# write data\n+my $pid = open(my $out, \"|-\", @command);\n+defined syswrite($out, $body_data) or die \"Cannot write data: $!\";\n+\n+sleep 1; # is interrupted by SIGCHLD\n+if (!$exited) {\n+ close($out);\n+ die \"Command did not exit after reading whole body\";\n+}\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349209","messageId":"20180604034402.GC14451@sigill.intra.peff.net","threadId":"48630","inReplyTo":"20180602212749.21324-2-max@max630.net","subject":"Re: [PATCH v7 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T03:44:03Z","receivedAt":"2018-06-04T03:44:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 03, 2018 at 12:27:48AM +0300, Max Kirillov wrote:\n\n> http-backend reads whole input until EOF. However, the RFC 3875 specifies\n> that a script must read only as many bytes as specified by CONTENT_LENGTH\n> environment variable. Web server may exercise the specification by not closing\n> the script's standard input after writing content. In that case http-backend\n> would hang waiting for the input. The issue is known to happen with\n> IIS/Windows, for example.\n> \n> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\n> the whole input until EOF. If the variable is not defined, keep older behavior\n> of reading until EOF because it is used to support chunked transfer-encoding.\n> \n> Signed-off-by: Florian Manschwetus \n> [mk: fixed trivial build failures and polished style issues]\n> Helped-by: Junio C Hamano \n> Signed-off-by: Max Kirillov \n> ---\n> config.c | 2 +-\n> config.h | 1 +\n> http-backend.c | 43 ++++++++++++++++++++++++++++++++++++++++++-\n> 3 files changed, 44 insertions(+), 2 deletions(-)\n\nThis first patch looks good to me, though it may be worth mentioning in\nthe commit message that we're only handling the buffered-input side here\n(that is obvious to anybody reading this whole series now, but it may\nhelp out people digging in the history later).\n\n-Peff\n"},{"id":"349213","messageId":"xmqqefhn5g01.fsf@gitster-ct.c.googlers.com","threadId":"48630","inReplyTo":"20180602212749.21324-3-max@max630.net","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T04:31:58Z","receivedAt":"2018-06-04T04:32:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> +static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)\n> +{\n> +\tunsigned char buf[8192];\n> +\tsize_t remaining_len = req_len;\n> +\n> +\twhile (remaining_len > 0) {\n> +\t\tsize_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;\n> +\t\tsize_t n = xread(0, buf, chunk_length);\n> +\t\tif (n < 0)\n> +\t\t\tdie_errno(\"Reading request failed\");\n\nn that is of type size_t is unsigned and cannot be negative here.\n"},{"id":"349214","messageId":"20180604044408.GD14451@sigill.intra.peff.net","threadId":"48630","inReplyTo":"20180602212749.21324-3-max@max630.net","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T04:44:09Z","receivedAt":"2018-06-04T04:44:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:\n\n> Push passes to another commands, as described in\n> https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/\n> \n> As it gets complicated to correctly track the data length, instead transfer\n> the data through parent process and cut the pipe as the specified length is\n> reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input\n> directly to the forked commands.\n\nI think this approach is reasonable. It's basically converting the\nknown-length case to a read-to-eof case for the sub-program, which\nshould paper over any problems of this type. And it's what we really\n_want_ the web server to be doing in the first place.\n\nSince this is slightly less efficient, and because it only matters if\nthe web server does not already close the pipe, should this have a\nrun-time configuration knob, even if it defaults to\nsafe-but-slightly-slower?\n\nI admit I don't overly care that much myself (the only large-scale Git\nserver deployment I am personally familiar with does not use\ngit-http-backend at all), but it might be nice to leave an escape hatch.\n\nThere are a few things in the patch worth fixing, but overall I think it\nlooks like a pretty good direction. Comments inline.\n\n> diff --git a/http-backend.c b/http-backend.c\n> index 3066697a24..78a588c551 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -351,23 +351,22 @@ static ssize_t get_content_length(void)\n> \treturn val;\n> }\n> \n> -static ssize_t read_request(int fd, unsigned char **out)\n> +static ssize_t read_request(int fd, unsigned char **out, ssize_t req_len)\n> {\n> -\tssize_t req_len = get_content_length();\n> -\n> \tif (req_len < 0)\n> \t\treturn read_request_eof(fd, out);\n> \telse\n> \t\treturn read_request_fixed_len(fd, req_len, out);\n> }\n\nMinor nit, but it might have been nice to build in this infrastructure\nin the first patch, rather than refactoring it here. It would also make\nit much more obvious that the first one is not handling some cases,\nsince we'd have \"req_len\" but not pass it to all of the code paths. ;)\n\n> @@ -379,11 +378,18 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n> \t\t\tif (full_request)\n> \t\t\t\tn = 0; /* nothing left to read */\n> \t\t\telse\n> -\t\t\t\tn = read_request(0, &full_request);\n> +\t\t\t\tn = read_request(0, &full_request, req_len);\n> \t\t\tstream.next_in = full_request;\n> \t\t} else {\n> -\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n> +\t\t\tssize_t buffer_len;\n> +\t\t\tif (req_remaining_len < 0 || req_remaining_len > sizeof(in_buf))\n> +\t\t\t buffer_len = sizeof(in_buf);\n> +\t\t\telse\n> +\t\t\t buffer_len = req_remaining_len;\n> +\t\t\tn = xread(0, in_buf, buffer_len);\n> \t\t\tstream.next_in = in_buf;\n> +\t\t\tif (req_remaining_len >= 0)\n> +\t\t\t\treq_remaining_len -= n;\n> \t\t}\n\nWhat happens here if xread() returns an error? We probably don't want to\nmodify req_remaining_len (it probably doesn't matter since we'd report\nthe errot after this, but it feels funny not to check here).\n\n> +static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)\n> +{\n> +\tunsigned char buf[8192];\n> +\tsize_t remaining_len = req_len;\n> +\n> +\twhile (remaining_len > 0) {\n> +\t\tsize_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;\n> +\t\tsize_t n = xread(0, buf, chunk_length);\n> +\t\tif (n < 0)\n> +\t\t\tdie_errno(\"Reading request failed\");\n\nI was going to complain that we usually start our error messages with a\nlowercase, but this program seems to be an exception. So here you've\nfollowed the local custom, which is OK.\n\n> +\t\tif (write_in_full(out, buf, n) < 0)\n> +\t\t\tdie_errno(\"%s aborted reading request\", prog_name);\n\nWe don't necessarily know why the write failed. If it's EPIPE, then yes,\nthe program probably did abort. But all we know is that write() failed.\nWe should probably say something more generic like:\n\n die_errno(\"unable to write to '%s'\");\n\nor similar.\n\n> diff --git a/t/helper/test-print-larger-than-ssize.c b/t/helper/test-print-larger-than-ssize.c\n> new file mode 100644\n> index 0000000000..83472a32f1\n> --- /dev/null\n> +++ b/t/helper/test-print-larger-than-ssize.c\n> @@ -0,0 +1,11 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +\n> +int cmd__print_larger_than_ssize(int argc, const char **argv)\n> +{\n> +\tsize_t large = ~0;\n> +\n> +\tlarge = ~(large & ~(large >> 1)) + 1;\n> +\tprintf(\"%\" PRIuMAX \"\\n\", (uintmax_t) large);\n> +\treturn 0;\n> +}\n\nI think this might be nicer as part of \"git version --build-options\".\nEither as a byte-size as I showed in [1], or directly showing this\nvalue.\n\n[1] https://public-inbox.org/git/20171129032632.GC32345@sigill.intra.peff.net/\n\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 87066ced62..edcfe5df63 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -25,6 +25,7 @@ static struct test_cmd cmds[] = {\n> \t{ \"mktemp\", cmd__mktemp },\n> \t{ \"online-cpus\", cmd__online_cpus },\n> \t{ \"path-utils\", cmd__path_utils },\n> + { \"print-larger-than-ssize\", cmd__print_larger_than_ssize },\n\nIndent with spaces?\n\n> diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\n> index 9fafcf1945..8c212393b4 100755\n> --- a/t/t5560-http-backend-noserver.sh\n> +++ b/t/t5560-http-backend-noserver.sh\n> @@ -71,4 +71,17 @@ test_expect_success 'http-backend blocks bad PATH_INFO' '\n> \texpect_aliased 1 //domain/data.txt\n> '\n> \n> +test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n> +\tNOT_FIT_IN_SSIZE=$(\"$GIT_BUILD_DIR/t/helper/test-tool\" print-larger-than-ssize) &&\n\nWe put helpers in the PATH, so this could just be \"test-tool\nprint-larger-than-ssize\" (though I still prefer the --build-options\nvariant).\n\n> +\tenv \\\n> +\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n> +\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n> +\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> +\t\tREQUEST_METHOD=POST \\\n> +\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n> +\t\tgit http-backend /dev/null 2>err &&\n> +\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n\nI'm not sure if these messages should be marked for translation. If so,\nyou'd want test_i18ngrep here.\n\nWe also generally avoid \"-q\" to grep. If the script is in non-verbose\nmode it will go to /dev/null anyway, and in verbose mode it's useful to\nsee (possibly ditto for the /dev/null redirection of stdout above, but I\nthink that might actually spew a binary packfile if the test fails,\nwhich we'd rather avoid).\n\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> new file mode 100755\n> index 0000000000..6b0c005db0\n> --- /dev/null\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -0,0 +1,155 @@\n> +#!/bin/sh\n> +\n> +test_description='test git-http-backend respects CONTENT_LENGTH'\n> +. ./test-lib.sh\n\nWhy is the too-large CONTENT_LENGTH test in another file? I'd have\nthought it would go well here, based on the description.\n\n> +verify_http_result() {\n> +\t# sometimes there is fatal error buit the result is still 200\n> +\tif grep 'fatal:' act.err\n> +\tthen\n> +\t\treturn 1\n> +\tfi\n> +\n> +\tif ! grep \"Status\" act.out >act\n> +\tthen\n> +\t\tprintf \"Status: 200 OK\\r\\n\" >act\n> +\tfi\n> +\tprintf \"Status: $1\\r\\n\" >exp &&\n> +\ttest_cmp exp act\n> +}\n\n200 with a fatal error sounds non-ideal. But I think it's unavoidable in\nsome cases where we see write failures, etc.\n\n> +test_http_env() {\n> +\thandler_type=\"$1\"\n> +\tshift\n> +\tenv \\\n> +\t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n> +\t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n> +\t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> +\t\tREQUEST_METHOD=POST \\\n> +\t\t\"$@\"\n> +}\n\nI think this env (and the earlier one) are not strictly necessary, as\nyou could just use shell one-shot variables. But I'm OK with them as an\nabundance of caution, since in theory a caller could use a shell\nfunction rather than a real command here (in which case one-shot\nvariables do the wrong thing).\n\n> +test_expect_success 'setup repository' '\n> +\ttest_commit c0 &&\n> +\ttest_commit c1\n> +'\n> +\n> +hash_head=$(git rev-parse HEAD)\n> +hash_prev=$(git rev-parse HEAD~1)\n\nWe generally prefer to have all commands, even ones we don't expect to\nfail, inside test_expect blocks (e.g., with a \"setup\" description).\n\n> +cat >fetch_body < +0032want $hash_head\n> +00000032have $hash_prev\n> +0009done\n> +EOF\n\nThis depends on the size of the hash. That's always 40 for now, but is\nsomething that may change soon.\n\nWe already have a packetize() helper; could we use it here?\n\n(Looking at the definition of that helper, it's actually kind of\nexpensive in terms of number of processes. We could perhaps convert it\nto perl and do it all in a single process, but that's orthogonal to your\nseries).\n\n> +gzip -k fetch_body\n\nWe don't unconditionally rely on gzip elsewhere. The test blocks using\nit (and the ones that depend on them) should be marked with the GZIP\nprerequisite.\n\n> +head -c -10 fetch_body.gz.trunc\n> +\n> +head -c -10 fetch_body.trunc\n\nWe can into portability problems with \"head -c\", but I think they were\nmostly with different buffering behavior (i.e., reading more than 10\nbytes). And that would be OK in this setting, since nobody is going to\nread the rest of the input after us.\n\nSo it's probably OK, but we could use \"test_copy_bytes 10\" here if it\nisn't.\n\n> +touch empty_body\n\nWe usually prefer \">empty_body\" to using \"touch\".\n\n> +test_expect_success 'fetch plain truncated' '\n> +\ttest_http_env upload \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n> +\ttest_must_fail verify_http_result \"200 OK\"\n> +'\n\nUsually test_must_fail on a checking function like this is a sign that\nthe check is not as robust as we'd like. If the function checks two\nthings \"A && B\", then checking test_must_fail will only let us know\n\"!A || !B\", but you probably want to check both.\n\nThe usual solution is for verify_http_result to take an optional \"!\" in\nthe first parameter and invert its sense. Or to just split it into two\nseparate functions.\n\n(We'd also generally not use test_must_fail with a non-git command, and\njust use a simple \"! verify_http_result\"; that would apply equally if\ngets split into two commands).\n\n> +test_expect_success 'push plain' '\n> +\tgit config http.receivepack true &&\n\nThis will persist after the test finishes. Try:\n\n test_config http.receivepack true\n\nwhich will clean up after the test finishes. Alternatively, since I\nthink you'd want this whole script to run with http.receivepack set,\nthis could be part of the repository setup in the earlier steps (and\nthen _don't_ use test_config, because the whole point is for it to\npersist).\n\n> +\ttest_http_env receive \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body git http-backend >act.out 2>act.err &&\n> +\tverify_http_result \"200 OK\" &&\n> +\tgit rev-parse newbranch >act.head &&\n> +\techo \"$hash_next\" >exp.head &&\n> +\ttest_cmp act.head exp.head &&\n> +\tgit branch -D newbranch\n> +'\n\nShould this \"git branch -D\" go into a \"test_when_finished\" block closer\nto when it is created?\n\n> +# write data\n> +my $pid = open(my $out, \"|-\", @command);\n> +defined syswrite($out, $body_data) or die \"Cannot write data: $!\";\n\nI assume perl's syswrite() has the usual write() pitfalls, like\nsometimes returning without writing all of the bytes. Could this just\nbe:\n\n print $out $body_date;\n\n?\n\n> +sleep 1; # is interrupted by SIGCHLD\n> +if (!$exited) {\n> + close($out);\n> + die \"Command did not exit after reading whole body\";\n> +}\n\nA sleep like this is a recipe for having the test fail when the system\nis under heavy load and it takes the sub-process more than a second to\nreturn (and the SIGCHLD to get delivered).\n\nNormally I'd suggest wait() or pause(), but I think the intent is to\nsleep because in the failure case we'd never see the signal, and just\nhang? If so, then perhaps we should give a much higher sleep, like 60\nseconds. That will mean the test eventually does report failure, but\nshould be much less likely to cause a false negative. And if we do get\nthe signal (which we'd usually expect), then we exit immediately.\n\nAlso, do we need to protect ourselves against other signals being\ndelivered? E.g., if I resize my xterm and this process gets SIGWINCH, is\nit going to erroneously end the sleep and say \"nope, no exited signal\"?\n\n\nMy read through the tests was mostly looking for mechanical problems. I\ndidn't give much though to whether we were getting full coverage, and\nnow it's my bed-time here. So I'll leave that for later (or somebody\nelse).\n\n-Peff\n"},{"id":"349280","messageId":"20180604170640.GB27650@jessie.local","threadId":"48630","inReplyTo":"xmqqefhn5g01.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-04T17:06:40Z","receivedAt":"2018-06-04T17:06:47Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Jun 04, 2018 at 01:31:58PM +0900, Junio C Hamano wrote:\n> Max Kirillov writes:\n>> +\t\tsize_t n = xread(0, buf, chunk_length);\n>> +\t\tif (n < 0)\n>> +\t\t\tdie_errno(\"Reading request failed\");\n> \n> n that is of type size_t is unsigned and cannot be negative here.\n\nThanks, fixing it\nDo you think sanity check for n <= chunk_length (the code\nwill go mad in this case) is needed? As far as I can see n\nreturns straight from system's read()\n"},{"id":"349301","messageId":"20180604221807.GC27650@jessie.local","threadId":"48630","inReplyTo":"20180604044408.GD14451@sigill.intra.peff.net","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-04T22:18:08Z","receivedAt":"2018-06-04T22:25:35Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:\n\nThanks for the comments, I will do the things you proposed,\nor try to and get back later if there are any issues. Some\nnotes below.\n\n> On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:\n> Since this is slightly less efficient, and because it only matters if\n> the web server does not already close the pipe, should this have a\n> run-time configuration knob, even if it defaults to\n> safe-but-slightly-slower?\n\nPersonally, I of course don't want this. Also, I don't think\nthe difference is much noticeable. But you can never be sure\nwithout trying. I'll try to measure some numbers.\n\n>> +\t\tif (write_in_full(out, buf, n) < 0)\n>> +\t\t\tdie_errno(\"%s aborted reading request\", prog_name);\n> \n> We don't necessarily know why the write failed. If it's EPIPE, then yes,\n> the program probably did abort. But all we know is that write() failed.\n> We should probably say something more generic like:\n> \n> die_errno(\"unable to write to '%s'\");\n> \n> or similar.\n\nActually, it is already 3rd same error in this file. Maybe\ndeserve some refactoring. I will change the message also.\n\n>> +test_expect_success 'setup repository' '\n>> +\ttest_commit c0 &&\n>> +\ttest_commit c1\n>> +'\n>> +\n>> +hash_head=$(git rev-parse HEAD)\n>> +hash_prev=$(git rev-parse HEAD~1)\n> \n> We generally prefer to have all commands, even ones we don't expect to\n> fail, inside test_expect blocks (e.g., with a \"setup\" description).\n\nWill the defined variables get to the next test? I'll try to\ndo as you describe.\n\n>> +cat >fetch_body <> +0032want $hash_head\n>> +00000032have $hash_prev\n>> +0009done\n>> +EOF\n> \n> This depends on the size of the hash. That's always 40 for now, but is\n> something that may change soon.\n> \n> We already have a packetize() helper; could we use it here?\n\nCould you point me to it? I cannot find it.\n\nMy understanfing is that the current protocol assumes\n40 symbols hash, so another hash length would be another\nprotocol, and since it's manually forged here it would\nanyway has to be changeda.\n\n>> +test_expect_success 'fetch plain truncated' '\n>> +\ttest_http_env upload \\\n>> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n>> +\ttest_must_fail verify_http_result \"200 OK\"\n>> +'\n> \n> Usually test_must_fail on a checking function like this is a sign that\n> the check is not as robust as we'd like. If the function checks two\n> things \"A && B\", then checking test_must_fail will only let us know\n> \"!A || !B\", but you probably want to check both.\n\nWell here I just want to know that the request has failed,\nand we already know that it can fail in different ways,\nbut the test is not going to differentiate those ways.\n\n> (We'd also generally not use test_must_fail with a non-git command, and\n> just use a simple \"! verify_http_result\"; that would apply equally if\n> gets split into two commands).\n\nWill use ! there.\n\n>> +sleep 1; # is interrupted by SIGCHLD\n>> +if (!$exited) {\n>> + close($out);\n>> + die \"Command did not exit after reading whole body\";\n>> +}\n\n...\n\n> Also, do we need to protect ourselves against other signals being\n> delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is\n> it going to erroneously end the sleep and say \"nope, no exited signal\"?\n\nI'll check, but what could I do? Should I add blocking other\nsignals there?\n"},{"id":"349315","messageId":"ff9215cb-6f38-90df-3073-c0592751249d@ramsayjones.plus.com","threadId":"48630","inReplyTo":"20180604170640.GB27650@jessie.local","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-06-05T02:30:24Z","receivedAt":"2018-06-05T02:30:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/06/18 18:06, Max Kirillov wrote:\n> On Mon, Jun 04, 2018 at 01:31:58PM +0900, Junio C Hamano wrote:\n>> Max Kirillov writes:\n>>> +\t\tsize_t n = xread(0, buf, chunk_length);\n>>> +\t\tif (n < 0)\n>>> +\t\t\tdie_errno(\"Reading request failed\");\n>>\n>> n that is of type size_t is unsigned and cannot be negative here.\n> \n\nHmm, xread() returns an ssize_t, which is a signed type ...\n\n> Thanks, fixing it\n> Do you think sanity check for n <= chunk_length (the code\n> will go mad in this case) is needed? As far as I can see n\n> returns straight from system's read()\n\nATB,\nRamsay Jones\n\n"},{"id":"349858","messageId":"20180610150521.9714-1-max@max630.net","threadId":"48630","inReplyTo":"20180602212749.21324-1-max@max630.net","subject":"[PATCH v8 0/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:05:18Z","receivedAt":"2018-06-10T15:13:14Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Did the fixes proposed for v7\n\nMax Kirillov (3):\n http-backend: cleanup writing to child process\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n http-backend: respect CONTENT_LENGTH for receive-pack\n\n config.c | 2 +-\n config.h | 1 +\n help.c | 1 +\n http-backend.c | 100 +++++++++++++--\n t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 37 ++++++\n 6 files changed, 295 insertions(+), 15 deletions(-)\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349859","messageId":"20180610150521.9714-2-max@max630.net","threadId":"48630","inReplyTo":"20180610150521.9714-1-max@max630.net","subject":"[PATCH v8 1/3] http-backend: cleanup writing to child process","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:05:19Z","receivedAt":"2018-06-10T15:13:18Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"As explained in [1], we should not assume the reason why the writing has\nfailed, and even if the reason is that child has existed not the reason\nwhy it have done so. So instead just say that writing has failed.\n\n[1] https://public-inbox.org/git/20180604044408.GD14451@sigill.intra.peff.net/\n\nSigned-off-by: Max Kirillov \n---\n http-backend.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 88d2a9bc40..206dc28e07 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -278,6 +278,12 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n \treturn svc;\n }\n \n+static void write_to_child(int out, const unsigned char *buf, ssize_t len, const char *prog_name)\n+{\n+\tif (write_in_full(out, buf, len) < 0)\n+\t\tdie(\"unable to write to '%s'\", prog_name);\n+}\n+\n /*\n * This is basically strbuf_read(), except that if we\n * hit max_request_buffer we die (we'd rather reject a\n@@ -360,9 +366,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\t\tdie(\"zlib error inflating request, result %d\", ret);\n \n \t\t\tn = stream.total_out - cnt;\n-\t\t\tif (write_in_full(out, out_buf, n) < 0)\n-\t\t\t\tdie(\"%s aborted reading request\", prog_name);\n-\t\t\tcnt += n;\n+\t\t\twrite_to_child(out, out_buf, stream.total_out - cnt, prog_name);\n+\t\t\tcnt = stream.total_out;\n \n \t\t\tif (ret == Z_STREAM_END)\n \t\t\t\tgoto done;\n@@ -381,8 +386,7 @@ static void copy_request(const char *prog_name, int out)\n \tssize_t n = read_request(0, &buf);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n-\tif (write_in_full(out, buf, n) < 0)\n-\t\tdie(\"%s aborted reading request\", prog_name);\n+\twrite_to_child(out, buf, n, prog_name);\n \tclose(out);\n \tfree(buf);\n }\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349860","messageId":"20180610150521.9714-3-max@max630.net","threadId":"48630","inReplyTo":"20180610150521.9714-1-max@max630.net","subject":"[PATCH v8 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:05:20Z","receivedAt":"2018-06-10T15:13:19Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"http-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nThis commit only fixes buffered input, whcih reads whole body before\nprocessign it. Non-buffered input is going to be fixed in subsequent commit.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \n---\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++-------\n 3 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c698988f5e..4148a3529d 100644\n--- a/config.c\n+++ b/config.c\n@@ -853,7 +853,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n-static int git_parse_ssize_t(const char *value, ssize_t *ret)\n+int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\ndiff --git a/config.h b/config.h\nindex ef70a9cac1..c143a1b634 100644\n--- a/config.h\n+++ b/config.h\n@@ -48,6 +48,7 @@ extern void git_config(config_fn_t fn, void *);\n extern int config_with_options(config_fn_t fn, void *,\n \t\t\t struct git_config_source *config_source,\n \t\t\t const struct config_options *opts);\n+extern int git_parse_ssize_t(const char *, ssize_t *);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/http-backend.c b/http-backend.c\nindex 206dc28e07..0c9e9be2b7 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -289,7 +289,7 @@ static void write_to_child(int out, const unsigned char *buf, ssize_t len, const\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -326,7 +326,46 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n-static void inflate_request(const char *prog_name, int out, int buffer_input)\n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): \"\n+\t\t \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t max_request_buffer, (uintmax_t)req_len);\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\t*out = buf;\n+\treturn cnt;\n+}\n+\n+static ssize_t get_content_length(void)\n+{\n+\tssize_t val = -1;\n+\tconst char *str = getenv(\"CONTENT_LENGTH\");\n+\n+\tif (str && !git_parse_ssize_t(str, &val))\n+\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\treturn val;\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out, ssize_t req_len)\n+{\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n+static void inflate_request(const char *prog_name, int out, int buffer_input, ssize_t req_len)\n {\n \tgit_zstream stream;\n \tunsigned char *full_request = NULL;\n@@ -344,7 +383,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\tif (full_request)\n \t\t\t\tn = 0; /* nothing left to read */\n \t\t\telse\n-\t\t\t\tn = read_request(0, &full_request);\n+\t\t\t\tn = read_request(0, &full_request, req_len);\n \t\t\tstream.next_in = full_request;\n \t\t} else {\n \t\t\tn = xread(0, in_buf, sizeof(in_buf));\n@@ -380,10 +419,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \tfree(full_request);\n }\n \n-static void copy_request(const char *prog_name, int out)\n+static void copy_request(const char *prog_name, int out, ssize_t req_len)\n {\n \tunsigned char *buf;\n-\tssize_t n = read_request(0, &buf);\n+\tssize_t n = read_request(0, &buf, req_len);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n \twrite_to_child(out, buf, n, prog_name);\n@@ -398,6 +437,7 @@ static void run_service(const char **argv, int buffer_input)\n \tconst char *host = getenv(\"REMOTE_ADDR\");\n \tint gzipped_request = 0;\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n+\tssize_t req_len = get_content_length();\n \n \tif (encoding && !strcmp(encoding, \"gzip\"))\n \t\tgzipped_request = 1;\n@@ -424,9 +464,9 @@ static void run_service(const char **argv, int buffer_input)\n \n \tclose(1);\n \tif (gzipped_request)\n-\t\tinflate_request(argv[0], cld.in, buffer_input);\n+\t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n-\t\tcopy_request(argv[0], cld.in);\n+\t\tcopy_request(argv[0], cld.in, req_len);\n \telse\n \t\tclose(0);\n \n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349861","messageId":"20180610150521.9714-4-max@max630.net","threadId":"48630","inReplyTo":"20180610150521.9714-1-max@max630.net","subject":"[PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:05:21Z","receivedAt":"2018-06-10T15:13:24Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Push passes to another commands, as described in\nhttps://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/\n\nAs it gets complicated to correctly track the data length, instead transfer\nthe data through parent process and cut the pipe as the specified length is\nreached. Do it only when CONTENT_LENGTH is set, otherwise pass the input\ndirectly to the forked commands.\n\nAdd tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data, with all combinations\n of variations: fetch or push, plain or compressed body, correct or truncated\n input.\n\n* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \n---\n help.c | 1 +\n http-backend.c | 32 ++++-\n t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 37 ++++++\n 4 files changed, 237 insertions(+), 2 deletions(-)\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\ndiff --git a/help.c b/help.c\nindex 60071a9bea..42600ca026 100644\n--- a/help.c\n+++ b/help.c\n@@ -419,6 +419,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tprintf(\"no commit associated with this build\\n\");\n \t\tprintf(\"sizeof-long: %d\\n\", (int)sizeof(long));\n+\t\tprintf(\"sizeof-size_t: %d\\n\", (int)sizeof(size_t));\n \t\t/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */\n \t}\n \treturn 0;\ndiff --git a/http-backend.c b/http-backend.c\nindex 0c9e9be2b7..28c07e7c2a 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -372,6 +372,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss\n \tunsigned char in_buf[8192];\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n+\tint req_len_defined = req_len >= 0;\n+\tsize_t req_remaining_len = req_len;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init_gzip_only(&stream);\n@@ -386,8 +388,15 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss\n \t\t\t\tn = read_request(0, &full_request, req_len);\n \t\t\tstream.next_in = full_request;\n \t\t} else {\n-\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n+\t\t\tssize_t buffer_len;\n+\t\t\tif (req_len_defined && req_remaining_len <= sizeof(in_buf))\n+\t\t\t\tbuffer_len = req_remaining_len;\n+\t\t\telse\n+\t\t\t\tbuffer_len = sizeof(in_buf);\n+\t\t\tn = xread(0, in_buf, buffer_len);\n \t\t\tstream.next_in = in_buf;\n+\t\t\tif (req_len_defined && n > 0)\n+\t\t\t\treq_remaining_len -= n;\n \t\t}\n \n \t\tif (n <= 0)\n@@ -430,6 +439,23 @@ static void copy_request(const char *prog_name, int out, ssize_t req_len)\n \tfree(buf);\n }\n \n+static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)\n+{\n+\tunsigned char buf[8192];\n+\tsize_t remaining_len = req_len;\n+\n+\twhile (remaining_len > 0) {\n+\t\tsize_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;\n+\t\tssize_t n = xread(0, buf, chunk_length);\n+\t\tif (n < 0)\n+\t\t\tdie_errno(\"Reading request failed\");\n+\t\twrite_to_child(out, buf, n, prog_name);\n+\t\tremaining_len -= n;\n+\t}\n+\n+\tclose(out);\n+}\n+\n static void run_service(const char **argv, int buffer_input)\n {\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n@@ -456,7 +482,7 @@ static void run_service(const char **argv, int buffer_input)\n \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tif (buffer_input || gzipped_request)\n+\tif (buffer_input || gzipped_request || req_len >= 0)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n \tif (start_command(&cld))\n@@ -467,6 +493,8 @@ static void run_service(const char **argv, int buffer_input)\n \t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n \t\tcopy_request(argv[0], cld.in, req_len);\n+\telse if (req_len >= 0)\n+\t\tpipe_fixed_length(argv[0], cld.in, req_len);\n \telse\n \t\tclose(0);\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nnew file mode 100755\nindex 0000000000..8040d80e04\n--- /dev/null\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -0,0 +1,169 @@\n+#!/bin/sh\n+\n+test_description='test git-http-backend respects CONTENT_LENGTH'\n+. ./test-lib.sh\n+\n+test_lazy_prereq GZIP 'gzip --version'\n+\n+verify_http_result() {\n+\t# sometimes there is fatal error buit the result is still 200\n+\tif grep 'fatal:' act.err\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tif ! grep \"Status\" act.out >act\n+\tthen\n+\t\tprintf \"Status: 200 OK\\r\\n\" >act\n+\tfi\n+\tprintf \"Status: $1\\r\\n\" >exp &&\n+\ttest_cmp exp act\n+}\n+\n+test_http_env() {\n+\thandler_type=\"$1\"\n+\tshift\n+\tenv \\\n+\t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n+\t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\t\"$@\"\n+}\n+\n+ssize_b100dots() {\n+\t# hardcoded ((size_t) SSIZE_MAX) + 1\n+\tcase \"$(build_option sizeof-size_t)\" in\n+\t8) echo 9223372036854775808;;\n+\t4) echo 2147483648;;\n+\t*) die \"Unexpected ssize_t size: $(build_option sizeof-size_t)\";;\n+\tesac\n+}\n+\n+test_expect_success 'setup' '\n+\tgit config http.receivepack true &&\n+\ttest_commit c0 &&\n+\ttest_commit c1 &&\n+\thash_head=$(git rev-parse HEAD) &&\n+\thash_prev=$(git rev-parse HEAD~1) &&\n+\tprintf \"want %s\" \"$hash_head\" | packetize >fetch_body &&\n+\tprintf 0000 >>fetch_body &&\n+\tprintf \"have %s\" \"$hash_prev\" | packetize >>fetch_body &&\n+\tprintf done | packetize >>fetch_body &&\n+\ttest_copy_bytes 10 fetch_body.trunc &&\n+\thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n+\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \"$_z40\" \"$hash_next\" | packetize >push_body &&\n+\tprintf 0000 >>push_body &&\n+\techo \"$hash_next\" | git pack-objects --stdout >>push_body &&\n+\ttest_copy_bytes 10 push_body.trunc &&\n+\t: >empty_body\n+'\n+\n+test_expect_success GZIP 'setup, compression related' '\n+\tgzip -k fetch_body &&\n+\ttest_copy_bytes 10 fetch_body.gz.trunc &&\n+\tgzip -k push_body &&\n+\ttest_copy_bytes 10 push_body.gz.trunc\n+'\n+\n+test_expect_success 'fetch plain' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain truncated' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain empty' '\n+\ttest_http_env upload \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped truncated' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz.trunc git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped empty' '\n+\ttest_http_env upload \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push plain' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head\n+'\n+\n+test_expect_success 'push plain truncated' '\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.trunc git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push plain empty' '\n+\ttest_http_env receive \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push gzipped' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz git http-backend >act.out 2>act.err &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head\n+'\n+\n+test_expect_success GZIP 'push gzipped truncated' '\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz.trunc git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push gzipped empty' '\n+\ttest_http_env receive \\\n+\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=$(ssize_b100dots) &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n+test_done\ndiff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl\nnew file mode 100755\nindex 0000000000..6c2aae7692\n--- /dev/null\n+++ b/t/t5562/invoke-with-content-length.pl\n@@ -0,0 +1,37 @@\n+#!/usr/bin/perl\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+my $body_filename = $ARGV[0];\n+my @command = @ARGV[1 .. $#ARGV];\n+\n+# read data\n+my $body_size = -s $body_filename;\n+$ENV{\"CONTENT_LENGTH\"} = $body_size;\n+open(my $body_fh, \"<\", $body_filename) or die \"Cannot open $body_filename: $!\";\n+my $body_data;\n+defined read($body_fh, $body_data, $body_size) or die \"Cannot read $body_filename: $!\";\n+close($body_fh);\n+\n+my $exited = 0;\n+$SIG{\"CHLD\"} = sub {\n+ $exited = 1;\n+};\n+\n+# write data\n+my $pid = open(my $out, \"|-\", @command);\n+{\n+ # disable buffering at $out\n+ my $old_selected = select;\n+ select $out;\n+ $| = 1;\n+ select $old_selected;\n+}\n+print $out $body_data or die \"Cannot write data: $!\";\n+\n+sleep 60; # is interrupted by SIGCHLD\n+if (!$exited) {\n+ close($out);\n+ die \"Command did not exit after reading whole body\";\n+}\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"349862","messageId":"20180610150619.GD27650@jessie.local","threadId":"48630","inReplyTo":"20180604221807.GC27650@jessie.local","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:06:19Z","receivedAt":"2018-06-10T15:13:44Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Jun 05, 2018 at 01:18:08AM +0300, Max Kirillov wrote:\n> On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:\n>> Since this is slightly less efficient, and because it only matters if\n>> the web server does not already close the pipe, should this have a\n>> run-time configuration knob, even if it defaults to\n>> safe-but-slightly-slower?\n> \n> Personally, I of course don't want this. Also, I don't think\n> the difference is much noticeable. But you can never be sure\n> without trying. I'll try to measure some numbers.\n\nIt seems to be challenging to see any effect at my system.\nAt least not with any real operation because changing\nreferences needs IO and index-pack needs CPU so. I'll try\nit some more.\n\n>> We should probably say something more generic like:\n>> \n>> die_errno(\"unable to write to '%s'\");\n>> \n>> or similar.\n> \n> Actually, it is already 3rd same error in this file. Maybe\n> deserve some refactoring. I will change the message also.\n\nExtracted the writing and refactoring to a single function,\nalso fixed the message.\n\n>>> +cat >fetch_body <>> +0032want $hash_head\n>>> +00000032have $hash_prev\n>>> +0009done\n>>> +EOF\n>> \n>> This depends on the size of the hash. That's always 40 for now, but is\n>> something that may change soon.\n>> \n>> We already have a packetize() helper; could we use it here?\n\n> Could you point me to it? I cannot find it.\n\nSorry, misread it as packetSize. Found and used.\n\n>> Also, do we need to protect ourselves against other signals being\n>> delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is\n>> it going to erroneously end the sleep and say \"nope, no exited signal\"?\n\n> I'll check, but what could I do? Should I add blocking other\n> signals there?\n\nIn my Linux I don't see the signal. Except that, there seem to\nbe not that many ignored signals. Anyway, I don't see what\ncould be done bout it.\n"},{"id":"349863","messageId":"20180610150727.GE27650@jessie.local","threadId":"48630","inReplyTo":"20180604044408.GD14451@sigill.intra.peff.net","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-06-10T15:07:27Z","receivedAt":"2018-06-10T15:14:56Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:\n> On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:\n>> +\tenv \\\n>> +\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n>> +\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n>> +\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n>> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n>> +\t\tREQUEST_METHOD=POST \\\n>> +\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n>> +\t\tgit http-backend /dev/null 2>err &&\n>> +\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n> \n> I'm not sure if these messages should be marked for translation. If so,\n> you'd want test_i18ngrep here.\n\nMessage localization does not seem to be used in\nhttp-backend at all. It makes sense - server-side software\nprobably does not know who is the user on the other side, if\nthe message gets to the user at all. So, I think the\nmessage should not be translated.\n"},{"id":"349884","messageId":"20180611085930.GA16414@sigill.intra.peff.net","threadId":"48630","inReplyTo":"20180610150727.GE27650@jessie.local","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T08:59:31Z","receivedAt":"2018-06-11T08:59:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 10, 2018 at 06:07:27PM +0300, Max Kirillov wrote:\n\n> On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:\n> > On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:\n> >> +\tenv \\\n> >> +\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n> >> +\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n> >> +\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n> >> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> >> +\t\tREQUEST_METHOD=POST \\\n> >> +\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n> >> +\t\tgit http-backend /dev/null 2>err &&\n> >> +\tgrep -q \"fatal:.*CONTENT_LENGTH\" err\n> > \n> > I'm not sure if these messages should be marked for translation. If so,\n> > you'd want test_i18ngrep here.\n> \n> Message localization does not seem to be used in\n> http-backend at all. It makes sense - server-side software\n> probably does not know who is the user on the other side, if\n> the message gets to the user at all. So, I think the\n> message should not be translated.\n\nOK. I think there's been talk of localizing \"fatal:\", but whoever does\nthat patch would have to deal with fallout all over the test-suite. I\ndon't think we need to worry about it yet.\n\n-Peff\n"},{"id":"349885","messageId":"20180611091813.GB16414@sigill.intra.peff.net","threadId":"48630","inReplyTo":"20180604221807.GC27650@jessie.local","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T09:18:13Z","receivedAt":"2018-06-11T09:18:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2018 at 01:18:08AM +0300, Max Kirillov wrote:\n\n> > On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:\n> > Since this is slightly less efficient, and because it only matters if\n> > the web server does not already close the pipe, should this have a\n> > run-time configuration knob, even if it defaults to\n> > safe-but-slightly-slower?\n> \n> Personally, I of course don't want this. Also, I don't think\n> the difference is much noticeable. But you can never be sure\n> without trying. I'll try to measure some numbers.\n\nI don't know if it will matter or not. I just wonder if we want to leave\nan escape hatch for people who might. I could take or leave it.\n\n> Actually, it is already 3rd same error in this file. Maybe\n> deserve some refactoring. I will change the message also.\n\nThanks, that kind of related cleanup is very welcome.\n\n> > We generally prefer to have all commands, even ones we don't expect to\n> > fail, inside test_expect blocks (e.g., with a \"setup\" description).\n> \n> Will the defined variables get to the next test? I'll try to\n> do as you describe.\n\nYes, the tests are all run as evals. So as long as you don't open a\nsubshell yourself, any changes you make to process state will persist.\n\n> >> +test_expect_success 'fetch plain truncated' '\n> >> +\ttest_http_env upload \\\n> >> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n> >> +\ttest_must_fail verify_http_result \"200 OK\"\n> >> +'\n> > \n> > Usually test_must_fail on a checking function like this is a sign that\n> > the check is not as robust as we'd like. If the function checks two\n> > things \"A && B\", then checking test_must_fail will only let us know\n> > \"!A || !B\", but you probably want to check both.\n> \n> Well here I just want to know that the request has failed,\n> and we already know that it can fail in different ways,\n> but the test is not going to differentiate those ways.\n\nOK, looking over your verify_http_result function, I _think_ we are OK\nhere, because the only && is against a printf, which we wouldn't really\nexpect to fail.\n\n> >> +sleep 1; # is interrupted by SIGCHLD\n> >> +if (!$exited) {\n> >> + close($out);\n> >> + die \"Command did not exit after reading whole body\";\n> >> +}\n> \n> > Also, do we need to protect ourselves against other signals being\n> > delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is\n> > it going to erroneously end the sleep and say \"nope, no exited signal\"?\n> \n> I'll check, but what could I do? Should I add blocking other\n> signals there?\n\nI think a more robust check may be to waitpid() on the child for up to N\nseconds. Something like this:\n\n $SIG{ALRM} = sub {\n\t kill(9, $pid);\n\t die \"command did not exit after reading whole body\"\n };\n alarm(60);\n waitpid($pid, 0);\n alarm(0);\n\nThat should exit immediately if $pid does, and otherwise die after\nexactly 60 seconds. Perl's waitpid implementation will restart\nautomatically if it gets another signal.\n\n-Peff\n"},{"id":"349886","messageId":"20180611092440.GC16414@sigill.intra.peff.net","threadId":"48630","inReplyTo":"20180611091813.GB16414@sigill.intra.peff.net","subject":"Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T09:24:40Z","receivedAt":"2018-06-11T09:24:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 05:18:13AM -0400, Jeff King wrote:\n\n> > >> +sleep 1; # is interrupted by SIGCHLD\n> > >> +if (!$exited) {\n> > >> + close($out);\n> > >> + die \"Command did not exit after reading whole body\";\n> > >> +}\n> > \n> > > Also, do we need to protect ourselves against other signals being\n> > > delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is\n> > > it going to erroneously end the sleep and say \"nope, no exited signal\"?\n> > \n> > I'll check, but what could I do? Should I add blocking other\n> > signals there?\n> \n> I think a more robust check may be to waitpid() on the child for up to N\n> seconds. Something like this:\n> \n> $SIG{ALRM} = sub {\n> \t kill(9, $pid);\n> \t die \"command did not exit after reading whole body\"\n> };\n> alarm(60);\n> waitpid($pid, 0);\n> alarm(0);\n> \n> That should exit immediately if $pid does, and otherwise die after\n> exactly 60 seconds. Perl's waitpid implementation will restart\n> automatically if it gets another signal.\n\nI tried your original, delivering some signals to it. I think it\nactually is OK, too, because perl's sleep() implementation will also\nrestart for something like SIGWINCH.\n\nE.g., stracing looks like this:\n\n nanosleep({tv_sec=60, tv_nsec=0}, {tv_sec=57, tv_nsec=791891377}) = ? ERESTART_RESTARTBLOCK (Interrupted by signal)\n --- SIGWINCH {si_signo=SIGWINCH, si_code=SI_KERNEL} ---\n restart_syscall(<... resuming interrupted nanosleep ...>\n\n-Peff\n"},{"id":"353561","messageId":"20180725121435.20519-1-szeder.dev@gmail.com","threadId":"48630","inReplyTo":"20180610150521.9714-4-max@max630.net","subject":"Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-25T12:14:35Z","receivedAt":"2018-07-25T12:14:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n[Hrm, this time with hopefully proper In-Reply-To: header.\n Sorry for the double post.]\n\n\n> Push passes to another commands, as described in\n> https://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/\n> \n> As it gets complicated to correctly track the data length, instead transfer\n> the data through parent process and cut the pipe as the specified length is\n> reached. Do it only when CONTENT_LENGTH is set, otherwise pass the input\n> directly to the forked commands.\n> \n> Add tests for cases:\n> \n> * CONTENT_LENGTH is set, script's stdin has more data, with all combinations\n> of variations: fetch or push, plain or compressed body, correct or truncated\n> input.\n> \n> * CONTENT_LENGTH is specified to a value which does not fit into ssize_t.\n> \n> Helped-by: Junio C Hamano \n> Signed-off-by: Max Kirillov \n> ---\n> help.c | 1 +\n> http-backend.c | 32 ++++-\n> t/t5562-http-backend-content-length.sh | 169 +++++++++++++++++++++++++\n> t/t5562/invoke-with-content-length.pl | 37 ++++++\n> 4 files changed, 237 insertions(+), 2 deletions(-)\n> create mode 100755 t/t5562-http-backend-content-length.sh\n> create mode 100755 t/t5562/invoke-with-content-length.pl\n\n\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> new file mode 100755\n> index 0000000000..8040d80e04\n> --- /dev/null\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -0,0 +1,169 @@\n> +#!/bin/sh\n> +\n> +test_description='test git-http-backend respects CONTENT_LENGTH'\n> +. ./test-lib.sh\n> +\n> +test_lazy_prereq GZIP 'gzip --version'\n> +\n> +verify_http_result() {\n> +\t# sometimes there is fatal error buit the result is still 200\n\ns/buit/but/\n\n> +\tif grep 'fatal:' act.err\n> +\tthen\n> +\t\treturn 1\n> +\tfi\n\nI just happened to stumble upon a failure because of 'fatal: the\nremote end hung up unexpectedly' in the test 'push plain'.\n\nWhat does that \"sometimes\" in the above comment mean, and how often\ndoes such a failure happen? I see these patches are in 'pu' for over\na month now, so based on the number of reflog entries since then it\nhappened once from about 30-35 builds on Travis CI so far.\n\nI don't really like the idea of adding a bunch of flaky test cases...\nwe have enough of them already, unfortunately.\n\n> +\n> +\tif ! grep \"Status\" act.out >act\n> +\tthen\n> +\t\tprintf \"Status: 200 OK\\r\\n\" >act\n> +\tfi\n> +\tprintf \"Status: $1\\r\\n\" >exp &&\n> +\ttest_cmp exp act\n> +}\n> +\n> +test_http_env() {\n> +\thandler_type=\"$1\"\n> +\tshift\n> +\tenv \\\n> +\t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n> +\t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n> +\t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> +\t\tREQUEST_METHOD=POST \\\n> +\t\t\"$@\"\n> +}\n> +\n> +ssize_b100dots() {\n> +\t# hardcoded ((size_t) SSIZE_MAX) + 1\n> +\tcase \"$(build_option sizeof-size_t)\" in\n> +\t8) echo 9223372036854775808;;\n> +\t4) echo 2147483648;;\n> +\t*) die \"Unexpected ssize_t size: $(build_option sizeof-size_t)\";;\n> +\tesac\n> +}\n> +\n> +test_expect_success 'setup' '\n> +\tgit config http.receivepack true &&\n> +\ttest_commit c0 &&\n> +\ttest_commit c1 &&\n> +\thash_head=$(git rev-parse HEAD) &&\n> +\thash_prev=$(git rev-parse HEAD~1) &&\n> +\tprintf \"want %s\" \"$hash_head\" | packetize >fetch_body &&\n> +\tprintf 0000 >>fetch_body &&\n> +\tprintf \"have %s\" \"$hash_prev\" | packetize >>fetch_body &&\n> +\tprintf done | packetize >>fetch_body &&\n> +\ttest_copy_bytes 10 fetch_body.trunc &&\n> +\thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n> +\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \"$_z40\" \"$hash_next\" | packetize >push_body &&\n> +\tprintf 0000 >>push_body &&\n> +\techo \"$hash_next\" | git pack-objects --stdout >>push_body &&\n> +\ttest_copy_bytes 10 push_body.trunc &&\n> +\t: >empty_body\n> +'\n> +\n> +test_expect_success GZIP 'setup, compression related' '\n> +\tgzip -k fetch_body &&\n> +\ttest_copy_bytes 10 fetch_body.gz.trunc &&\n> +\tgzip -k push_body &&\n> +\ttest_copy_bytes 10 push_body.gz.trunc\n> +'\n> +\n> +test_expect_success 'fetch plain' '\n> +\ttest_http_env upload \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&\n\nDon't save the standard error of the whole shell function.\nWhen running the test with /bin/sh and '-x' tracing, then the trace of\ncommands executed in the function will be included in the standard\nerror as well, which may interfere with later verification (though in\nthis case it doesn't seem like it would cause any issues).\n\nPlease limit the redirections to the relevant command's output. AFAICT\nall invocations of 'test_http_env' in these tests have their stdout and\nstderr redirected to the same pair of files, so perhaps you could\nsimply move all these redirections inside the function.\n\n> +\tverify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success 'fetch plain truncated' '\n> +\ttest_http_env upload \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.trunc git http-backend >act.out 2>act.err &&\n\nIf this command were to print a \"fatal: ...\" message to its standard\nerror, then ...\n\n> +\t! verify_http_result \"200 OK\"\n\n... this function would return error (because of that 'if grep fatal:\n...' statement) without even looking at the status, but the test would\nstill succeed. Is that really the desired behavior here?\n\n> +'\n> +\n> +test_expect_success 'fetch plain empty' '\n> +\ttest_http_env upload \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'fetch gzipped' '\n> +\ttest_http_env upload \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz git http-backend >act.out 2>act.err &&\n> +\tverify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'fetch gzipped truncated' '\n> +\ttest_http_env upload \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body.gz.trunc git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'fetch gzipped empty' '\n> +\ttest_http_env upload \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'push plain' '\n> +\ttest_when_finished \"git branch -D newbranch\" &&\n> +\ttest_http_env receive \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body git http-backend >act.out 2>act.err &&\n> +\tverify_http_result \"200 OK\" &&\n> +\tgit rev-parse newbranch >act.head &&\n> +\techo \"$hash_next\" >exp.head &&\n> +\ttest_cmp act.head exp.head\n> +'\n> +\n> +test_expect_success 'push plain truncated' '\n> +\ttest_http_env receive \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.trunc git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success 'push plain empty' '\n> +\ttest_http_env receive \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'push gzipped' '\n> +\ttest_when_finished \"git branch -D newbranch\" &&\n> +\ttest_http_env receive \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz git http-backend >act.out 2>act.err &&\n> +\tverify_http_result \"200 OK\" &&\n> +\tgit rev-parse newbranch >act.head &&\n> +\techo \"$hash_next\" >exp.head &&\n> +\ttest_cmp act.head exp.head\n> +'\n> +\n> +test_expect_success GZIP 'push gzipped truncated' '\n> +\ttest_http_env receive \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl push_body.gz.trunc git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success GZIP 'push gzipped empty' '\n> +\ttest_http_env receive \\\n> +\t\tHTTP_CONTENT_ENCODING=\"gzip\" \\\n> +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl empty_body git http-backend >act.out 2>act.err &&\n> +\t! verify_http_result \"200 OK\"\n> +'\n> +\n> +test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n> +\tNOT_FIT_IN_SSIZE=$(ssize_b100dots) &&\n> +\tenv \\\n> +\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n> +\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n> +\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> +\t\tREQUEST_METHOD=POST \\\n> +\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n> +\t\tgit http-backend /dev/null 2>err &&\n> +\tgrep \"fatal:.*CONTENT_LENGTH\" err\n> +'\n> +\n> +test_done\n> diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl\n> new file mode 100755\n> index 0000000000..6c2aae7692\n> --- /dev/null\n> +++ b/t/t5562/invoke-with-content-length.pl\n> @@ -0,0 +1,37 @@\n> +#!/usr/bin/perl\n> +use 5.008;\n> +use strict;\n> +use warnings;\n> +\n> +my $body_filename = $ARGV[0];\n> +my @command = @ARGV[1 .. $#ARGV];\n> +\n> +# read data\n> +my $body_size = -s $body_filename;\n> +$ENV{\"CONTENT_LENGTH\"} = $body_size;\n> +open(my $body_fh, \"<\", $body_filename) or die \"Cannot open $body_filename: $!\";\n> +my $body_data;\n> +defined read($body_fh, $body_data, $body_size) or die \"Cannot read $body_filename: $!\";\n> +close($body_fh);\n> +\n> +my $exited = 0;\n> +$SIG{\"CHLD\"} = sub {\n> + $exited = 1;\n> +};\n> +\n> +# write data\n> +my $pid = open(my $out, \"|-\", @command);\n> +{\n> + # disable buffering at $out\n> + my $old_selected = select;\n> + select $out;\n> + $| = 1;\n> + select $old_selected;\n> +}\n> +print $out $body_data or die \"Cannot write data: $!\";\n> +\n> +sleep 60; # is interrupted by SIGCHLD\n> +if (!$exited) {\n> + close($out);\n> + die \"Command did not exit after reading whole body\";\n> +}\n> -- \n> 2.17.0.1185.g782057d875\n> \n> \n"},{"id":"353568","messageId":"20180725145100.GA1959@jessie.local","threadId":"48630","inReplyTo":"20180725121435.20519-1-szeder.dev@gmail.com","subject":"Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-25T14:51:00Z","receivedAt":"2018-07-25T14:51:08Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:\n>> +\t# sometimes there is fatal error buit the result is still 200\n> \n> s/buit/but/\n\nThanks, will fix\n\n>> +\tif grep 'fatal:' act.err\n>> +\tthen\n>> +\t\treturn 1\n>> +\tfi\n> \n> I just happened to stumble upon a failure because of 'fatal: the\n> remote end hung up unexpectedly' in the test 'push plain'.\n\nDid it happen once or repeated? It is rather strange, that\none shoud not fail. Which OS it was?\n\nThere have been doubds that a random incoming signal can\ntrigger such a failure.\n\n> What does that \"sometimes\" in the above comment mean, and how often\n> does such a failure happen? I see these patches are in 'pu' for over\n> a month now, so based on the number of reflog entries since then it\n> happened once from about 30-35 builds on Travis CI so far.\n\n\"sometimes\" here means \"for some kinds of fatal error\nfailure\", there is nothing random in it.\n\n> > +\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&\n> \n> Don't save the standard error of the whole shell function.\n> When running the test with /bin/sh and '-x' tracing, then the trace of\n> commands executed in the function will be included in the standard\n> error as well, which may interfere with later verification (though in\n> this case it doesn't seem like it would cause any issues).\n> \n> Please limit the redirections to the relevant command's output. AFAICT\n> all invocations of 'test_http_env' in these tests have their stdout and\n> stderr redirected to the same pair of files, so perhaps you could\n> simply move all these redirections inside the function.\n\nThanks, I'll try to fix it \n\n>> +\t! verify_http_result \"200 OK\"\n> \n> ... this function would return error (because of that 'if grep fatal:\n> ...' statement) without even looking at the status, but the test would\n> still succeed. Is that really the desired behavior here?\n\nYes, it is a desired behavior. A failure is expected here,\nand the failure does not show up as non-200 status, as\ndescribed above.\n"},{"id":"353583","messageId":"CAM0VKjkSMqPy=N3_0HUNxpCFwusrD_XE5j7kMsE4L-79g2t_VA@mail.gmail.com","threadId":"48630","inReplyTo":"20180725145100.GA1959@jessie.local","subject":"Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-25T18:41:31Z","receivedAt":"2018-07-25T18:41:46Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov wrote:\n>\n> On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:\n> >> + # sometimes there is fatal error buit the result is still 200\n\n> >> + if grep 'fatal:' act.err\n> >> + then\n> >> + return 1\n> >> + fi\n> >\n> > I just happened to stumble upon a failure because of 'fatal: the\n> > remote end hung up unexpectedly' in the test 'push plain'.\n>\n> Did it happen once or repeated? It is rather strange, that\n> one shoud not fail. Which OS it was?\n\nOnly once, so far. It was one of my OSX build jobs on Travis CI, but\nI don't know what OSX version is used.\n\n'act.err' contained this (which will get line-wrapped, I'm afraid):\n\n++handler_type=receive\n++shift\n++env CONTENT_TYPE=application/x-git-receive-pack-request\nQUERY_STRING=/repo.git/git-receive-pack\n'PATH_TRANSLATED=/Users/travis/t/trash\ndir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE\nREQUEST_METHOD=POST\n/Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl\npush_body git http-backend\n<...128 zero bytes...>fatal: the remote end hung up unexpectedly\n\nI couldn't reproduce it on my Linux box.\n\n> There have been doubds that a random incoming signal can\n> trigger such a failure.\n>\n> > What does that \"sometimes\" in the above comment mean, and how often\n> > does such a failure happen? I see these patches are in 'pu' for over\n> > a month now, so based on the number of reflog entries since then it\n> > happened once from about 30-35 builds on Travis CI so far.\n>\n> \"sometimes\" here means \"for some kinds of fatal error\n> failure\", there is nothing random in it.\n\n> >> + ! verify_http_result \"200 OK\"\n> >\n> > ... this function would return error (because of that 'if grep fatal:\n> > ...' statement) without even looking at the status, but the test would\n> > still succeed. Is that really the desired behavior here?\n>\n> Yes, it is a desired behavior. A failure is expected here,\n> and the failure does not show up as non-200 status, as\n> described above.\n\nOK, then I misunderstood that comment.\n\nPerhaps a different wording could make it slightly better? E.g. \"In\nsome of these tests ...\" instead of that \"sometimes\". Dunno.\n"},{"id":"353610","messageId":"20180726043751.GB1959@jessie.local","threadId":"48630","inReplyTo":"CAM0VKjkSMqPy=N3_0HUNxpCFwusrD_XE5j7kMsE4L-79g2t_VA@mail.gmail.com","subject":"Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-26T04:37:51Z","receivedAt":"2018-07-26T04:40:13Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Jul 25, 2018 at 08:41:31PM +0200, SZEDER Gábor wrote:\n> On Wed, Jul 25, 2018 at 4:51 PM Max Kirillov wrote:\n>>> I just happened to stumble upon a failure because of 'fatal: the\n>>> remote end hung up unexpectedly' in the test 'push plain'.\n>>\n>> Did it happen once or repeated? It is rather strange, that\n>> one shoud not fail. Which OS it was?\n> \n> Only once, so far. It was one of my OSX build jobs on Travis CI, but\n> I don't know what OSX version is used.\n> \n> 'act.err' contained this (which will get line-wrapped, I'm afraid):\n> \n> ++handler_type=receive\n> ++shift\n> ++env CONTENT_TYPE=application/x-git-receive-pack-request\n> QUERY_STRING=/repo.git/git-receive-pack\n> 'PATH_TRANSLATED=/Users/travis/t/trash\n> dir.t5562/.git/git-receive-pack' GIT_HTTP_EXPORT_ALL=TRUE\n> REQUEST_METHOD=POST\n> /Users/travis/build/szeder/git-cooking-topics-for-travis-ci/t/t5562/invoke-with-content-length.pl\n> push_body git http-backend\n> <...128 zero bytes...>fatal: the remote end hung up unexpectedly\n> \n> I couldn't reproduce it on my Linux box.\n\nThe only reason for this I could imagine is some perl\nutility failure to feed the body to git http-backend.\nI could not reproduce it either, but if such things happen\noften again maybe should concider C helper instead. Though\nI'm afraid I easily can make more mistakes in it than perl\ninterpreter authors.\n\nI'll make the other changes, and sofar just hope it would\nnot happen again.\n"},{"id":"353685","messageId":"20180727034859.15769-1-max@max630.net","threadId":"48630","inReplyTo":"20180610150521.9714-1-max@max630.net","subject":"[PATCH v9 0/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-27T03:48:56Z","receivedAt":"2018-07-27T03:49:09Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"* fix the gzip usage as suggested in https://public-inbox.org/git/xmqqk1quvegh.fsf@gitster-ct.c.googlers.com/\n* better explanation of why status check is needed\n* redirect only the helper call, not the whole shell function, also move more into the shell function\n\nMax Kirillov (3):\n http-backend: cleanup writing to child process\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n http-backend: respect CONTENT_LENGTH for receive-pack\n\n config.c | 2 +-\n config.h | 1 +\n help.c | 1 +\n http-backend.c | 100 +++++++++++++---\n t/t5562-http-backend-content-length.sh | 155 +++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 37 ++++++\n 6 files changed, 281 insertions(+), 15 deletions(-)\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"353686","messageId":"20180727034859.15769-2-max@max630.net","threadId":"48630","inReplyTo":"20180727034859.15769-1-max@max630.net","subject":"[PATCH v9 1/3] http-backend: cleanup writing to child process","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-27T03:48:57Z","receivedAt":"2018-07-27T03:49:12Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"As explained in [1], we should not assume the reason why the writing has\nfailed, and even if the reason is that child has existed not the reason\nwhy it have done so. So instead just say that writing has failed.\n\n[1] https://public-inbox.org/git/20180604044408.GD14451@sigill.intra.peff.net/\n\nSigned-off-by: Max Kirillov \nSigned-off-by: Junio C Hamano \n---\n http-backend.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex adaef16fad..cefdfd6fc6 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -279,6 +279,12 @@ static struct rpc_service *select_service(struct strbuf *hdr, const char *name)\n \treturn svc;\n }\n \n+static void write_to_child(int out, const unsigned char *buf, ssize_t len, const char *prog_name)\n+{\n+\tif (write_in_full(out, buf, len) < 0)\n+\t\tdie(\"unable to write to '%s'\", prog_name);\n+}\n+\n /*\n * This is basically strbuf_read(), except that if we\n * hit max_request_buffer we die (we'd rather reject a\n@@ -361,9 +367,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\t\tdie(\"zlib error inflating request, result %d\", ret);\n \n \t\t\tn = stream.total_out - cnt;\n-\t\t\tif (write_in_full(out, out_buf, n) < 0)\n-\t\t\t\tdie(\"%s aborted reading request\", prog_name);\n-\t\t\tcnt += n;\n+\t\t\twrite_to_child(out, out_buf, stream.total_out - cnt, prog_name);\n+\t\t\tcnt = stream.total_out;\n \n \t\t\tif (ret == Z_STREAM_END)\n \t\t\t\tgoto done;\n@@ -382,8 +387,7 @@ static void copy_request(const char *prog_name, int out)\n \tssize_t n = read_request(0, &buf);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n-\tif (write_in_full(out, buf, n) < 0)\n-\t\tdie(\"%s aborted reading request\", prog_name);\n+\twrite_to_child(out, buf, n, prog_name);\n \tclose(out);\n \tfree(buf);\n }\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"353687","messageId":"20180727034859.15769-3-max@max630.net","threadId":"48630","inReplyTo":"20180727034859.15769-1-max@max630.net","subject":"[PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-27T03:48:58Z","receivedAt":"2018-07-27T03:49:15Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"http-backend reads whole input until EOF. However, the RFC 3875 specifies\nthat a script must read only as many bytes as specified by CONTENT_LENGTH\nenvironment variable. Web server may exercise the specification by not closing\nthe script's standard input after writing content. In that case http-backend\nwould hang waiting for the input. The issue is known to happen with\nIIS/Windows, for example.\n\nMake http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than\nthe whole input until EOF. If the variable is not defined, keep older behavior\nof reading until EOF because it is used to support chunked transfer-encoding.\n\nThis commit only fixes buffered input, whcih reads whole body before\nprocessign it. Non-buffered input is going to be fixed in subsequent commit.\n\nSigned-off-by: Florian Manschwetus \n[mk: fixed trivial build failures and polished style issues]\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \nSigned-off-by: Junio C Hamano \n---\n config.c | 2 +-\n config.h | 1 +\n http-backend.c | 54 +++++++++++++++++++++++++++++++++++++++++++-------\n 3 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex fbbf0f8e9f..158afa858b 100644\n--- a/config.c\n+++ b/config.c\n@@ -921,7 +921,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n-static int git_parse_ssize_t(const char *value, ssize_t *ret)\n+int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n \tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\ndiff --git a/config.h b/config.h\nindex cdac2fc73e..7808413bd0 100644\n--- a/config.h\n+++ b/config.h\n@@ -73,6 +73,7 @@ extern void git_config(config_fn_t fn, void *);\n extern int config_with_options(config_fn_t fn, void *,\n \t\t\t struct git_config_source *config_source,\n \t\t\t const struct config_options *opts);\n+extern int git_parse_ssize_t(const char *, ssize_t *);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\ndiff --git a/http-backend.c b/http-backend.c\nindex cefdfd6fc6..d0b6cb1b09 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -290,7 +290,7 @@ static void write_to_child(int out, const unsigned char *buf, ssize_t len, const\n * hit max_request_buffer we die (we'd rather reject a\n * maliciously large request than chew up infinite memory).\n */\n-static ssize_t read_request(int fd, unsigned char **out)\n+static ssize_t read_request_eof(int fd, unsigned char **out)\n {\n \tsize_t len = 0, alloc = 8192;\n \tunsigned char *buf = xmalloc(alloc);\n@@ -327,7 +327,46 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t}\n }\n \n-static void inflate_request(const char *prog_name, int out, int buffer_input)\n+static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n+{\n+\tunsigned char *buf = NULL;\n+\tssize_t cnt = 0;\n+\n+\tif (max_request_buffer < req_len) {\n+\t\tdie(\"request was larger than our maximum size (%lu): \"\n+\t\t \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t max_request_buffer, (uintmax_t)req_len);\n+\t}\n+\n+\tbuf = xmalloc(req_len);\n+\tcnt = read_in_full(fd, buf, req_len);\n+\tif (cnt < 0) {\n+\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\t*out = buf;\n+\treturn cnt;\n+}\n+\n+static ssize_t get_content_length(void)\n+{\n+\tssize_t val = -1;\n+\tconst char *str = getenv(\"CONTENT_LENGTH\");\n+\n+\tif (str && !git_parse_ssize_t(str, &val))\n+\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\treturn val;\n+}\n+\n+static ssize_t read_request(int fd, unsigned char **out, ssize_t req_len)\n+{\n+\tif (req_len < 0)\n+\t\treturn read_request_eof(fd, out);\n+\telse\n+\t\treturn read_request_fixed_len(fd, req_len, out);\n+}\n+\n+static void inflate_request(const char *prog_name, int out, int buffer_input, ssize_t req_len)\n {\n \tgit_zstream stream;\n \tunsigned char *full_request = NULL;\n@@ -345,7 +384,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\tif (full_request)\n \t\t\t\tn = 0; /* nothing left to read */\n \t\t\telse\n-\t\t\t\tn = read_request(0, &full_request);\n+\t\t\t\tn = read_request(0, &full_request, req_len);\n \t\t\tstream.next_in = full_request;\n \t\t} else {\n \t\t\tn = xread(0, in_buf, sizeof(in_buf));\n@@ -381,10 +420,10 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \tfree(full_request);\n }\n \n-static void copy_request(const char *prog_name, int out)\n+static void copy_request(const char *prog_name, int out, ssize_t req_len)\n {\n \tunsigned char *buf;\n-\tssize_t n = read_request(0, &buf);\n+\tssize_t n = read_request(0, &buf, req_len);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n \twrite_to_child(out, buf, n, prog_name);\n@@ -399,6 +438,7 @@ static void run_service(const char **argv, int buffer_input)\n \tconst char *host = getenv(\"REMOTE_ADDR\");\n \tint gzipped_request = 0;\n \tstruct child_process cld = CHILD_PROCESS_INIT;\n+\tssize_t req_len = get_content_length();\n \n \tif (encoding && !strcmp(encoding, \"gzip\"))\n \t\tgzipped_request = 1;\n@@ -425,9 +465,9 @@ static void run_service(const char **argv, int buffer_input)\n \n \tclose(1);\n \tif (gzipped_request)\n-\t\tinflate_request(argv[0], cld.in, buffer_input);\n+\t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n-\t\tcopy_request(argv[0], cld.in);\n+\t\tcopy_request(argv[0], cld.in, req_len);\n \telse\n \t\tclose(0);\n \n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"353688","messageId":"20180727034859.15769-4-max@max630.net","threadId":"48630","inReplyTo":"20180727034859.15769-1-max@max630.net","subject":"[PATCH v9 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-27T03:48:59Z","receivedAt":"2018-07-27T03:49:18Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Push passes to another commands, as described in\nhttps://public-inbox.org/git/20171129032214.GB32345@sigill.intra.peff.net/\n\nAs it gets complicated to correctly track the data length, instead transfer\nthe data through parent process and cut the pipe as the specified length is\nreached. Do it only when CONTENT_LENGTH is set, otherwise pass the input\ndirectly to the forked commands.\n\nAdd tests for cases:\n\n* CONTENT_LENGTH is set, script's stdin has more data, with all combinations\n of variations: fetch or push, plain or compressed body, correct or truncated\n input.\n\n* CONTENT_LENGTH is specified to a value which does not fit into ssize_t.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Max Kirillov \n---\n help.c | 1 +\n http-backend.c | 32 ++++-\n t/t5562-http-backend-content-length.sh | 155 +++++++++++++++++++++++++\n t/t5562/invoke-with-content-length.pl | 37 ++++++\n 4 files changed, 223 insertions(+), 2 deletions(-)\n create mode 100755 t/t5562-http-backend-content-length.sh\n create mode 100755 t/t5562/invoke-with-content-length.pl\n\ndiff --git a/help.c b/help.c\nindex dd35fcc133..e469f5731c 100644\n--- a/help.c\n+++ b/help.c\n@@ -609,6 +609,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tprintf(\"no commit associated with this build\\n\");\n \t\tprintf(\"sizeof-long: %d\\n\", (int)sizeof(long));\n+\t\tprintf(\"sizeof-size_t: %d\\n\", (int)sizeof(size_t));\n \t\t/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */\n \t}\n \treturn 0;\ndiff --git a/http-backend.c b/http-backend.c\nindex d0b6cb1b09..e88d29f62b 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -373,6 +373,8 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss\n \tunsigned char in_buf[8192];\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n+\tint req_len_defined = req_len >= 0;\n+\tsize_t req_remaining_len = req_len;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init_gzip_only(&stream);\n@@ -387,8 +389,15 @@ static void inflate_request(const char *prog_name, int out, int buffer_input, ss\n \t\t\t\tn = read_request(0, &full_request, req_len);\n \t\t\tstream.next_in = full_request;\n \t\t} else {\n-\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n+\t\t\tssize_t buffer_len;\n+\t\t\tif (req_len_defined && req_remaining_len <= sizeof(in_buf))\n+\t\t\t\tbuffer_len = req_remaining_len;\n+\t\t\telse\n+\t\t\t\tbuffer_len = sizeof(in_buf);\n+\t\t\tn = xread(0, in_buf, buffer_len);\n \t\t\tstream.next_in = in_buf;\n+\t\t\tif (req_len_defined && n > 0)\n+\t\t\t\treq_remaining_len -= n;\n \t\t}\n \n \t\tif (n <= 0)\n@@ -431,6 +440,23 @@ static void copy_request(const char *prog_name, int out, ssize_t req_len)\n \tfree(buf);\n }\n \n+static void pipe_fixed_length(const char *prog_name, int out, size_t req_len)\n+{\n+\tunsigned char buf[8192];\n+\tsize_t remaining_len = req_len;\n+\n+\twhile (remaining_len > 0) {\n+\t\tsize_t chunk_length = remaining_len > sizeof(buf) ? sizeof(buf) : remaining_len;\n+\t\tssize_t n = xread(0, buf, chunk_length);\n+\t\tif (n < 0)\n+\t\t\tdie_errno(\"Reading request failed\");\n+\t\twrite_to_child(out, buf, n, prog_name);\n+\t\tremaining_len -= n;\n+\t}\n+\n+\tclose(out);\n+}\n+\n static void run_service(const char **argv, int buffer_input)\n {\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n@@ -457,7 +483,7 @@ static void run_service(const char **argv, int buffer_input)\n \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tif (buffer_input || gzipped_request)\n+\tif (buffer_input || gzipped_request || req_len >= 0)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n \tif (start_command(&cld))\n@@ -468,6 +494,8 @@ static void run_service(const char **argv, int buffer_input)\n \t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n \t\tcopy_request(argv[0], cld.in, req_len);\n+\telse if (req_len >= 0)\n+\t\tpipe_fixed_length(argv[0], cld.in, req_len);\n \telse\n \t\tclose(0);\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nnew file mode 100755\nindex 0000000000..057dcb85d6\n--- /dev/null\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -0,0 +1,155 @@\n+#!/bin/sh\n+\n+test_description='test git-http-backend respects CONTENT_LENGTH'\n+. ./test-lib.sh\n+\n+test_lazy_prereq GZIP 'gzip --version'\n+\n+verify_http_result() {\n+\t# some fatal errors still produce status 200\n+\t# so check if there is the error message\n+\tif grep 'fatal:' act.err\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tif ! grep \"Status\" act.out >act\n+\tthen\n+\t\tprintf \"Status: 200 OK\\r\\n\" >act\n+\tfi\n+\tprintf \"Status: $1\\r\\n\" >exp &&\n+\ttest_cmp exp act\n+}\n+\n+test_http_env() {\n+\thandler_type=\"$1\"\n+\trequest_body=\"$2\"\n+\tshift\n+\tenv \\\n+\t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n+\t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl \\\n+\t\t \"$request_body\" git http-backend >act.out 2>act.err\n+}\n+\n+ssize_b100dots() {\n+\t# hardcoded ((size_t) SSIZE_MAX) + 1\n+\tcase \"$(build_option sizeof-size_t)\" in\n+\t8) echo 9223372036854775808;;\n+\t4) echo 2147483648;;\n+\t*) die \"Unexpected ssize_t size: $(build_option sizeof-size_t)\";;\n+\tesac\n+}\n+\n+test_expect_success 'setup' '\n+\texport HTTP_CONTENT_ENCODING=\"identity\" &&\n+\tgit config http.receivepack true &&\n+\ttest_commit c0 &&\n+\ttest_commit c1 &&\n+\thash_head=$(git rev-parse HEAD) &&\n+\thash_prev=$(git rev-parse HEAD~1) &&\n+\tprintf \"want %s\" \"$hash_head\" | packetize >fetch_body &&\n+\tprintf 0000 >>fetch_body &&\n+\tprintf \"have %s\" \"$hash_prev\" | packetize >>fetch_body &&\n+\tprintf done | packetize >>fetch_body &&\n+\ttest_copy_bytes 10 fetch_body.trunc &&\n+\thash_next=$(git commit-tree -p HEAD -m next HEAD^{tree}) &&\n+\tprintf \"%s %s refs/heads/newbranch\\\\0report-status\\\\n\" \"$_z40\" \"$hash_next\" | packetize >push_body &&\n+\tprintf 0000 >>push_body &&\n+\techo \"$hash_next\" | git pack-objects --stdout >>push_body &&\n+\ttest_copy_bytes 10 push_body.trunc &&\n+\t: >empty_body\n+'\n+\n+test_expect_success GZIP 'setup, compression related' '\n+\tgzip -c fetch_body >fetch_body.gz &&\n+\ttest_copy_bytes 10 fetch_body.gz.trunc &&\n+\tgzip -c push_body >push_body.gz &&\n+\ttest_copy_bytes 10 push_body.gz.trunc\n+'\n+\n+test_expect_success 'fetch plain' '\n+\ttest_http_env upload fetch_body &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain truncated' '\n+\ttest_http_env upload fetch_body.trunc &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'fetch plain empty' '\n+\ttest_http_env upload empty_body &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped' '\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env upload fetch_body.gz &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped truncated' '\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env upload fetch_body.gz.trunc &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'fetch gzipped empty' '\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env upload empty_body &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push plain' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_http_env receive push_body &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head\n+'\n+\n+test_expect_success 'push plain truncated' '\n+\ttest_http_env receive push_body.trunc &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'push plain empty' '\n+\ttest_http_env receive empty_body &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push gzipped' '\n+\ttest_when_finished \"git branch -D newbranch\" &&\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env receive push_body.gz &&\n+\tverify_http_result \"200 OK\" &&\n+\tgit rev-parse newbranch >act.head &&\n+\techo \"$hash_next\" >exp.head &&\n+\ttest_cmp act.head exp.head\n+'\n+\n+test_expect_success GZIP 'push gzipped truncated' '\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env receive push_body.gz.trunc &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success GZIP 'push gzipped empty' '\n+\ttest_env HTTP_CONTENT_ENCODING=\"gzip\" test_http_env receive empty_body &&\n+\t! verify_http_result \"200 OK\"\n+'\n+\n+test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n+\tNOT_FIT_IN_SSIZE=$(ssize_b100dots) &&\n+\tenv \\\n+\t\tCONTENT_TYPE=application/x-git-upload-pack-request \\\n+\t\tQUERY_STRING=/repo.git/git-upload-pack \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/git-upload-pack \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=POST \\\n+\t\tCONTENT_LENGTH=\"$NOT_FIT_IN_SSIZE\" \\\n+\t\tgit http-backend /dev/null 2>err &&\n+\tgrep \"fatal:.*CONTENT_LENGTH\" err\n+'\n+\n+test_done\ndiff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl\nnew file mode 100755\nindex 0000000000..6c2aae7692\n--- /dev/null\n+++ b/t/t5562/invoke-with-content-length.pl\n@@ -0,0 +1,37 @@\n+#!/usr/bin/perl\n+use 5.008;\n+use strict;\n+use warnings;\n+\n+my $body_filename = $ARGV[0];\n+my @command = @ARGV[1 .. $#ARGV];\n+\n+# read data\n+my $body_size = -s $body_filename;\n+$ENV{\"CONTENT_LENGTH\"} = $body_size;\n+open(my $body_fh, \"<\", $body_filename) or die \"Cannot open $body_filename: $!\";\n+my $body_data;\n+defined read($body_fh, $body_data, $body_size) or die \"Cannot read $body_filename: $!\";\n+close($body_fh);\n+\n+my $exited = 0;\n+$SIG{\"CHLD\"} = sub {\n+ $exited = 1;\n+};\n+\n+# write data\n+my $pid = open(my $out, \"|-\", @command);\n+{\n+ # disable buffering at $out\n+ my $old_selected = select;\n+ select $out;\n+ $| = 1;\n+ select $old_selected;\n+}\n+print $out $body_data or die \"Cannot write data: $!\";\n+\n+sleep 60; # is interrupted by SIGCHLD\n+if (!$exited) {\n+ close($out);\n+ die \"Command did not exit after reading whole body\";\n+}\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"353689","messageId":"20180727035013.GC1959@jessie.local","threadId":"48630","inReplyTo":"20180727034859.15769-1-max@max630.net","subject":"Re: [PATCH v9 0/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-07-27T03:50:13Z","receivedAt":"2018-07-27T03:50:18Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Only the 3rd patch has changed\n"},{"id":"353736","messageId":"xmqq4lgko9gu.fsf@gitster-ct.c.googlers.com","threadId":"48630","inReplyTo":"20180727035013.GC1959@jessie.local","subject":"Re: [PATCH v9 0/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T17:49:05Z","receivedAt":"2018-07-27T17:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> Only the 3rd patch has changed\n\nThanks.\n"},{"id":"354482","messageId":"CACsJy8DRNHVgYYH0AjdcU68PGg1anp5g+d7Up3cXp0bmDuC0Mg@mail.gmail.com","threadId":"48630","inReplyTo":"20180727034859.15769-3-max@max630.net","subject":"Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-04T06:34:08Z","receivedAt":"2018-08-04T06:34:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:\n> -static void inflate_request(const char *prog_name, int out, int buffer_input)\n> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **out)\n> +{\n> + unsigned char *buf = NULL;\n> + ssize_t cnt = 0;\n> +\n> + if (max_request_buffer < req_len) {\n> + die(\"request was larger than our maximum size (%lu): \"\n> + \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n> + max_request_buffer, (uintmax_t)req_len);\n\nPlease mark these strings for translation with _().\n-- \nDuy\n"},{"id":"354487","messageId":"20180804112814.GA2060@jessie.local","threadId":"48630","inReplyTo":"CACsJy8DRNHVgYYH0AjdcU68PGg1anp5g+d7Up3cXp0bmDuC0Mg@mail.gmail.com","subject":"Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-08-04T11:28:14Z","receivedAt":"2018-08-04T11:35:41Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sat, Aug 04, 2018 at 08:34:08AM +0200, Duy Nguyen wrote:\n> On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:\n>> + if (max_request_buffer < req_len) {\n>> + die(\"request was larger than our maximum size (%lu): \"\n>> + \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n>> + max_request_buffer, (uintmax_t)req_len);\n> \n> Please mark these strings for translation with _().\n\nIt has been discussed in [1]. Since it is not a local user\nfacing part, probably should not be translated.\n\n[1] https://public-inbox.org/git/20180610150727.GE27650@jessie.local/\n\n-- \nMax\n"},{"id":"354498","messageId":"xmqqk1p6rqub.fsf@gitster-ct.c.googlers.com","threadId":"48630","inReplyTo":"20180804112814.GA2060@jessie.local","subject":"Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-04T17:20:28Z","receivedAt":"2018-08-04T17:20:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> On Sat, Aug 04, 2018 at 08:34:08AM +0200, Duy Nguyen wrote:\n>> On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:\n>>> + if (max_request_buffer < req_len) {\n>>> + die(\"request was larger than our maximum size (%lu): \"\n>>> + \"%\" PRIuMAX \"; try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n>>> + max_request_buffer, (uintmax_t)req_len);\n>> \n>> Please mark these strings for translation with _().\n>\n> It has been discussed in [1]. Since it is not a local user\n> facing part, probably should not be translated.\n>\n> [1] https://public-inbox.org/git/20180610150727.GE27650@jessie.local/\n\nI'd support that design decision, FWIW.\n"}]}