From: Daniel Duvall Date: Thu, 29 Oct 2020 23:53:26 GMT Subject: [PATCH] Fix upload-pack EOF death in normal stateless negotiation Message-ID: Hi there, I noticed while debugging an issue in Phabricator where shallow git fetches over HTTP were failing with 500 errors that `git-http-backend` seemed to exit abnormally after the first request even though the response was otherwise correct—and when the error is not surfaced to the client, the packfile negotiation via `git-remote-curl` seems to continue normally.[1] I think this patch fixes the issue by having `upload-pack` gently handle EOFs at a specific point in negotiation—after shallow/unshallow lines have been returned to the client (followed by flush) but before the client sends its haves. This is my first contribution here so hopefully I'm understanding the packfile negotiation protocol correctly and included my test in the right place. Kindly, Daniel [1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317 From df738e146fde368ca8935d41b586093dd164cc2a Mon Sep 17 00:00:00 2001 From: Daniel Duvall Date: Thu, 29 Oct 2020 14:32:27 -0700 Subject: [PATCH] upload-pack: allow stateless client EOF just prior to haves During stateless packfile negotiation, it is normal behavior for stateless RPC clients (e.g. git-remote-curl) to send multiple upload-pack requests with the first containing only the wants/shallows/deepens/filters followed by a flush. When run in stateless mode, continuing on without first checking that the client request has reached EOF can result in a bad file descriptor during get_common_commits. Instead, upload-pack should gently peek for an EOF between the sending of shallow/unshallow lines (followed by flush) and the reading of client haves. If the client has hung up at this point, exit normally. --- t/t9904-upload-pack-stateless-timely-eof.sh | 24 ++++++++++++++++++++++++ upload-pack.c | 13 ++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100755 t/t9904-upload-pack-stateless-timely-eof.sh diff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh new file mode 100755 index 0000000..f8385a7 --- /dev/null +++ b/t/t9904-upload-pack-stateless-timely-eof.sh @@ -0,0 +1,24 @@ +#!/bin/sh + +test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush' + +. ./test-lib.sh + +D=$(pwd) + +test_expect_success 'upload-pack outputs flush and exits ok' ' + test_commit initial && + head=$(git rev-parse HEAD) && + hexsz=$(test_oid hexsz) && + + printf "%04xwant %s\n%04xshallow %s\n000ddeepen 1\n0000" \ + $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request && + + git upload-pack --stateless-rpc "$(pwd)" actual && + + printf "0000" >expect && + + test_cmp expect actual +' + +test_done diff --git a/upload-pack.c b/upload-pack.c index 3b858eb..2b128e4 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options) PACKET_READ_DIE_ON_ERR_PACKET); receive_needs(&data, &reader); - if (data.want_obj.nr) { + + /* + * An EOF at this exact point in negotiation should be + * acceptable from stateless clients as they will consume the + * shallow list before doing subsequent rpc with haves/etc. + */ + if (data.stateless_rpc) + reader.options |= PACKET_READ_GENTLE_ON_EOF; + + if (data.want_obj.nr && + packet_reader_peek(&reader) != PACKET_READ_EOF) { + reader.options ^= PACKET_READ_GENTLE_ON_EOF; get_common_commits(&data, &reader); create_pack_file(&data, NULL); } -- 2.6.1