From 296b847c0d6de63353e236cfbf94163d24155529 Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 18 Nov 2016 15:30:49 -0500 Subject: [PATCH 1/2] remote-curl: don't hang when a server dies before any output In the event that a HTTP server closes the connection after giving a 200 but before giving any packets, we don't want to hang forever waiting for a response that will never come. Instead, we should die immediately. One case where this happens is when attempting to fetch a dangling object by its object name. In this case, the server dies before sending any data. Prior to this patch, fetch-pack would wait for data from the server, and remote-curl would wait for fetch-pack, causing a deadlock. Despite this patch, there is other possible malformed input that could cause the same deadlock (e.g. a half-finished pktline, or a pktline but no trailing flush). There are a few possible solutions to this: 1. Allowing remote-curl to tell fetch-pack about the EOF (so that fetch-pack could know that no more data is coming until it says something else). This is tricky because an out-of-band signal would be required, or the http response would have to be re-framed inside another layer of pkt-line or something. 2. Make remote-curl understand some of the protocol. It turns out that in addition to understanding pkt-line, it would need to watch for ack/nak. This is somewhat fragile, as information about the protocol would end up in two places. Also, pkt-lines which are already at the length limit would need special handling. Both of these solutions would require a fair amount of work, whereas this hack is easy and solves at least some of the problem. Still to do: it would be good to give a better error message than "fatal: The remote end hung up unexpectedly". Signed-off-by: David Turner Signed-off-by: Junio C Hamano --- remote-curl.c | 8 ++++++++ t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/remote-curl.c b/remote-curl.c index f14c41f4c0..ee4423659f 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -400,6 +400,7 @@ struct rpc_state { size_t pos; int in; int out; + int any_written; struct strbuf result; unsigned gzip_request : 1; unsigned initial_buffer : 1; @@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize, { size_t size = eltsize * nmemb; struct rpc_state *rpc = buffer_; + if (size) + rpc->any_written = 1; write_or_die(rpc->in, ptr, size); return size; } @@ -659,6 +662,8 @@ retry: curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc); + + rpc->any_written = 0; err = run_slot(slot, NULL); if (err == HTTP_REAUTH && !large_request) { credential_fill(&http_auth); @@ -667,6 +672,9 @@ retry: if (err != HTTP_OK) err = -1; + if (!rpc->any_written) + err = -1; + curl_slist_free_all(headers); free(gzip_body); return err; diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 1ec5b2747a..43665ab4a8 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' ' test_line_count = 2 posts ' +test_expect_success 'test allowreachablesha1inwant' ' + test_when_finished "rm -rf test_reachable.git" && + server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + master_sha=$(git -C "$server" rev-parse refs/heads/master) && + git -C "$server" config uploadpack.allowreachablesha1inwant 1 && + + git init --bare test_reachable.git && + git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && + git -C test_reachable.git fetch origin "$master_sha" +' + +test_expect_success 'test allowreachablesha1inwant with unreachable' ' + test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" && + + #create unreachable sha + echo content >file2 && + git add file2 && + git commit -m two && + git push public HEAD:refs/heads/doomed && + git push public :refs/heads/doomed && + + server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + master_sha=$(git -C "$server" rev-parse refs/heads/master) && + git -C "$server" config uploadpack.allowreachablesha1inwant 1 && + + git init --bare test_reachable.git && + git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && + test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" +' + test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' ' ( cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && From f8edeaa05d8623a9f6dad408237496c51101aad8 Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 11 Nov 2016 12:23:48 -0500 Subject: [PATCH 2/2] upload-pack: optionally allow fetching any sha1 It seems a little silly to do a reachabilty check in the case where we trust the user to access absolutely everything in the repository. Also, it's racy in a distributed system -- perhaps one server advertises a ref, but another has since had a force-push to that ref, and perhaps the two HTTP requests end up directed to these different servers. Signed-off-by: David Turner Signed-off-by: Junio C Hamano --- Documentation/config.txt | 5 +++++ Documentation/git-fetch-pack.txt | 6 +++--- t/t5551-http-fetch-smart.sh | 22 ++++++++++++++++++++++ upload-pack.c | 10 +++++++++- 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index a0ab66aae7..b7f9991cc7 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2961,6 +2961,11 @@ uploadpack.allowReachableSHA1InWant:: calculating object reachability is computationally expensive. Defaults to `false`. +uploadpack.allowAnySHA1InWant:: + Allow `upload-pack` to accept a fetch request that asks for any + object at all. + Defaults to `false`. + uploadpack.keepAlive:: When `upload-pack` has started `pack-objects`, there may be a quiet period while `pack-objects` prepares the pack. Normally diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt index d45f6adc69..f7ebe36a7b 100644 --- a/Documentation/git-fetch-pack.txt +++ b/Documentation/git-fetch-pack.txt @@ -119,9 +119,9 @@ be in a separate packet, and the list must end with a flush packet. $GIT_DIR (e.g. "HEAD", "refs/heads/master"). When unspecified, update from all heads the remote side has. + -If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or -`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex -sha1s present on the remote. +If the remote has enabled the options `uploadpack.allowTipSHA1InWant`, +`uploadpack.allowReachableSHA1InWant`, or `uploadpack.allowAnySHA1InWant`, +they may alternatively be 40-hex sha1s present on the remote. SEE ALSO -------- diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 43665ab4a8..8d3db405c0 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -306,6 +306,28 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' ' test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" ' +test_expect_success 'test allowanysha1inwant with unreachable' ' + test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" && + + #create unreachable sha + echo content >file2 && + git add file2 && + git commit -m two && + git push public HEAD:refs/heads/doomed && + git push public :refs/heads/doomed && + + server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + master_sha=$(git -C "$server" rev-parse refs/heads/master) && + git -C "$server" config uploadpack.allowreachablesha1inwant 1 && + + git init --bare test_reachable.git && + git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && + test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" && + + git -C "$server" config uploadpack.allowanysha1inwant 1 && + git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" +' + test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' ' ( cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && diff --git a/upload-pack.c b/upload-pack.c index e0db8b42be..7597ba3405 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -46,6 +46,8 @@ static int no_progress, daemon_mode; #define ALLOW_TIP_SHA1 01 /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */ #define ALLOW_REACHABLE_SHA1 02 +/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */ +#define ALLOW_ANY_SHA1 07 static unsigned int allow_unadvertised_object_request; static int shallow_nr; static struct object_array have_obj; @@ -825,7 +827,8 @@ static void receive_needs(void) sha1_to_hex(sha1_buf)); if (!(o->flags & WANTED)) { o->flags |= WANTED; - if (!is_our_ref(o)) + if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1 + || is_our_ref(o))) has_non_tip = 1; add_object_array(o, NULL, &want_obj); } @@ -1008,6 +1011,11 @@ static int upload_pack_config(const char *var, const char *value, void *unused) allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; else allow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1; + } else if (!strcmp("uploadpack.allowanysha1inwant", var)) { + if (git_config_bool(var, value)) + allow_unadvertised_object_request |= ALLOW_ANY_SHA1; + else + allow_unadvertised_object_request &= ~ALLOW_ANY_SHA1; } else if (!strcmp("uploadpack.keepalive", var)) { keepalive = git_config_int(var, value); if (!keepalive)