{"thread":{"id":"48954","subject":"Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack","startedAt":"2018-07-25T12:02:30Z","lastAt":"2018-07-25T12:02:30Z","messageCount":1,"participants":["SZEDER Gábor"],"isPatch":true,"patchVersion":8,"patchTotal":3},"messages":[{"id":"353560","messageId":"20180725120206.19810-1-szeder.dev@gmail.com","threadId":"48954","inReplyTo":null,"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:02:06Z","receivedAt":"2018-07-25T12:02:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> 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 <gitster@pobox.com>\n> Signed-off-by: Max Kirillov <max@max630.net>\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 >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 >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 >fetch_body.gz.trunc &&\n> +\tgzip -k push_body &&\n> +\ttest_copy_bytes 10 <push_body.gz >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/zero >/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"}]}