{"thread":{"id":"54532","subject":"[PATCH] upload-pack: allow stateless client EOF just prior to haves","startedAt":"2020-10-30T01:51:02Z","lastAt":"2020-11-04T18:25:48Z","messageCount":18,"participants":["Daniel Duvall","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408691","messageId":"1604022059-18527-1-git-send-email-dan@mutual.io","threadId":"54532","inReplyTo":null,"subject":"[PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T01:40:59Z","receivedAt":"2020-10-30T01:51:02Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"During stateless packfile negotiation, it is normal behavior for\nstateless RPC clients (e.g. git-remote-curl) to send multiple\nupload-pack requests with the first containing only the\nwants/shallows/deepens/filters followed by a flush.\n\nWhen run in stateless mode, continuing on without first checking that\nthe client request has reached EOF can result in a bad file descriptor\nduring get_common_commits.\n\nInstead, upload-pack should gently peek for an EOF between the sending\nof shallow/unshallow lines (followed by flush) and the reading of client\nhaves. If the client has hung up at this point, exit normally.\n\nSigned-off-by: Daniel Duvall <dan@mutual.io>\n---\n t/t9904-upload-pack-stateless-timely-eof.sh | 24 ++++++++++++++++++++++++\n upload-pack.c                               | 13 ++++++++++++-\n 2 files changed, 36 insertions(+), 1 deletion(-)\n create mode 100755 t/t9904-upload-pack-stateless-timely-eof.sh\n\ndiff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\nnew file mode 100755\nindex 0000000..f8385a7\n--- /dev/null\n+++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush'\n+\n+. ./test-lib.sh\n+\n+D=$(pwd)\n+\n+test_expect_success 'upload-pack outputs flush and exits ok' '\n+\ttest_commit initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\thexsz=$(test_oid hexsz) &&\n+\n+\tprintf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n+\t\t$(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n+\n+\tgit upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n+\n+\tprintf \"0000\" >expect &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb..2b128e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t\treceive_needs(&data, &reader);\n-\t\tif (data.want_obj.nr) {\n+\n+\t\t/*\n+\t\t * An EOF at this exact point in negotiation should be\n+\t\t * acceptable from stateless clients as they will consume the\n+\t\t * shallow list before doing subsequent rpc with haves/etc.\n+\t\t */\n+\t\tif (data.stateless_rpc)\n+\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n+\n+\t\tif (data.want_obj.nr &&\n+\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n+\t\t\treader.options ^= PACKET_READ_GENTLE_ON_EOF;\n \t\t\tget_common_commits(&data, &reader);\n \t\t\tcreate_pack_file(&data, NULL);\n \t\t}\n-- \n2.6.1\n\n"},{"id":"408692","messageId":"CAPig+cTOkd248WKh5KyVfScRf8fqcOQPj4V3waGQ3XWEcugiKg@mail.gmail.com","threadId":"54532","inReplyTo":"1604022059-18527-1-git-send-email-dan@mutual.io","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-30T02:59:48Z","receivedAt":"2020-10-30T03:00:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 29, 2020 at 9:51 PM Daniel Duvall <dan@mutual.io> wrote:\n> [...]\n> Instead, upload-pack should gently peek for an EOF between the sending\n> of shallow/unshallow lines (followed by flush) and the reading of client\n> haves. If the client has hung up at this point, exit normally.\n>\n> Signed-off-by: Daniel Duvall <dan@mutual.io>\n> ---\n> diff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +\n> +test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush'\n> +\n> +. ./test-lib.sh\n> +\n> +D=$(pwd)\n\nWhat is the purpose of this assignment? It doesn't seem to be used\nanywhere in this script.\n\n> +test_expect_success 'upload-pack outputs flush and exits ok' '\n> +       test_commit initial &&\n> +       head=$(git rev-parse HEAD) &&\n> +       hexsz=$(test_oid hexsz) &&\n> +\n> +       printf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n> +               $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n> +\n> +       git upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n> +\n> +       printf \"0000\" >expect &&\n> +\n> +       test_cmp expect actual\n> +'\n> +\n> +test_done\n"},{"id":"408693","messageId":"CANo+1gvybEMEhQV2OWD8q3B9s=ztnJi_bFKo0eVVWwDRbYmAEA@mail.gmail.com","threadId":"54532","inReplyTo":"CAPig+cTOkd248WKh5KyVfScRf8fqcOQPj4V3waGQ3XWEcugiKg@mail.gmail.com","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T03:31:14Z","receivedAt":"2020-10-30T03:31:30Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"On Thu, Oct 29, 2020 at 8:00 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Thu, Oct 29, 2020 at 9:51 PM Daniel Duvall <dan@mutual.io> wrote:\n> > [...]\n> > Instead, upload-pack should gently peek for an EOF between the sending\n> > of shallow/unshallow lines (followed by flush) and the reading of client\n> > haves. If the client has hung up at this point, exit normally.\n> >\n> > Signed-off-by: Daniel Duvall <dan@mutual.io>\n> > ---\n> > diff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\n> > @@ -0,0 +1,24 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +D=$(pwd)\n>\n> What is the purpose of this assignment? It doesn't seem to be used\n> anywhere in this script.\n\nIt's an artifact of my copying/pasting a previous test script as a\ntemplate. I didn't see it used in that script either, so I assumed it\nwas needed somewhere in the test libs. If that's not the case, I can\ndefinitely remove it.\n\n>\n> > +test_expect_success 'upload-pack outputs flush and exits ok' '\n> > +       test_commit initial &&\n> > +       head=$(git rev-parse HEAD) &&\n> > +       hexsz=$(test_oid hexsz) &&\n> > +\n> > +       printf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n> > +               $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n> > +\n> > +       git upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n> > +\n> > +       printf \"0000\" >expect &&\n> > +\n> > +       test_cmp expect actual\n> > +'\n> > +\n> > +test_done\n"},{"id":"408715","messageId":"1604029402-19593-1-git-send-email-dan@mutual.io","threadId":"54532","inReplyTo":"1604022059-18527-1-git-send-email-dan@mutual.io","subject":"[PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T03:43:22Z","receivedAt":"2020-10-30T03:44:42Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"During stateless packfile negotiation, it is normal behavior for\nstateless RPC clients (e.g. git-remote-curl) to send multiple\nupload-pack requests with the first containing only the\nwants/shallows/deepens/filters followed by a flush.\n\nWhen run in stateless mode, continuing on without first checking that\nthe client request has reached EOF can result in a bad file descriptor\nduring get_common_commits.\n\nInstead, upload-pack should gently peek for an EOF between the sending\nof shallow/unshallow lines (followed by flush) and the reading of client\nhaves. If the client has hung up at this point, exit normally.\n\nSigned-off-by: Daniel Duvall <dan@mutual.io>\n---\n t/t9904-upload-pack-stateless-timely-eof.sh | 22 ++++++++++++++++++++++\n upload-pack.c                               | 13 ++++++++++++-\n 2 files changed, 34 insertions(+), 1 deletion(-)\n create mode 100755 t/t9904-upload-pack-stateless-timely-eof.sh\n\ndiff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\nnew file mode 100755\nindex 0000000..9a116c6\n--- /dev/null\n+++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'upload-pack outputs flush and exits ok' '\n+\ttest_commit initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\thexsz=$(test_oid hexsz) &&\n+\n+\tprintf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n+\t\t$(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n+\n+\tgit upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n+\n+\tprintf \"0000\" >expect &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb..2b128e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t\treceive_needs(&data, &reader);\n-\t\tif (data.want_obj.nr) {\n+\n+\t\t/*\n+\t\t * An EOF at this exact point in negotiation should be\n+\t\t * acceptable from stateless clients as they will consume the\n+\t\t * shallow list before doing subsequent rpc with haves/etc.\n+\t\t */\n+\t\tif (data.stateless_rpc)\n+\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n+\n+\t\tif (data.want_obj.nr &&\n+\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n+\t\t\treader.options ^= PACKET_READ_GENTLE_ON_EOF;\n \t\t\tget_common_commits(&data, &reader);\n \t\t\tcreate_pack_file(&data, NULL);\n \t\t}\n-- \n2.6.1\n\n"},{"id":"408717","messageId":"20201030044012.GA3259692@coredump.intra.peff.net","threadId":"54532","inReplyTo":"1604022059-18527-1-git-send-email-dan@mutual.io","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-30T04:40:12Z","receivedAt":"2020-10-30T04:40:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 29, 2020 at 06:40:59PM -0700, Daniel Duvall wrote:\n\n> During stateless packfile negotiation, it is normal behavior for\n> stateless RPC clients (e.g. git-remote-curl) to send multiple\n> upload-pack requests with the first containing only the\n> wants/shallows/deepens/filters followed by a flush.\n\nHmm, is this normal? I'd expect it to send a \"done\" after the flush, and\nindeed that's what happens if I try it. I see:\n\nE.g., here's GIT_TRACE_CURL output from a v0 request (fetching into an\nempty repo):\n\n Send data: 00a8want 4b379b7a1b97790f805c365b1e58b75b70d3d904 multi_ack_\n Send data: detailed no-done side-band-64k thin-pack ofs-delta deepen-si\n Send data: nce deepen-not agent=git/2.29.2.477.g2cec8aa0af.00000009done\n Send data: .\n Info: upload completely sent off: 181 out of 181 bytes\n\nHowever, if I add --depth 1 to my fetch, I get:\n\n  Send data: 00a8want 4b379b7a1b97790f805c365b1e58b75b70d3d904 multi_ack_\n  Send data: detailed no-done side-band-64k thin-pack ofs-delta deepen-si\n  Send data: nce deepen-not agent=git/2.29.2.477.g2cec8aa0af.000cdeepen 1\n  Send data: 0000\n  Info: upload completely sent off: 184 out of 184 bytes\n\nWhich maybe makes sense if we need the shallow response from the server\nto determine the next step of the request. It doesn't matter for my\ntrivial case here (we end up resending the same request with a \"done\"\nadded), but I guess it could in other cases.\n\nThis client side code is in fetch-pack.c:\n\n  static int find_common(struct fetch_negotiator *negotiator,\n  [...]\n  {\n          [...]\n          if (args->deepen) {\n\t\t  [...]\n                  send_request(args, fd[1], &req_buf);\n                  while (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n                          if (skip_prefix(reader.line, \"shallow \", &arg)) {\n\t\t\t\t[...etc...]\n                  }\n          } else if (!args->stateless_rpc)\n                  send_request(args, fd[1], &req_buf);\n\n\nand dates all the way back to 249b2004d8 (Smart fetch over HTTP: client\nside, 2009-10-30). Curiously we don't send it in non-stateless-rpc mode,\neven if we're shallow. So I'm a little puzzled why we need the response\nfirst in the stateless case, as we start writing \"have\" lines at the\nserver whether before we get the shallow lines back from the server in\neither case.\n\nSo I'm not entirely convinced there isn't a small bug lurking on the\nclient side here. Not one that produces a wrong answer, but one which\nwastes an extra round trip to the server. I won't be at all surprised if\nthere is some subtle timing dependency here, though, and the extra\nrequest really is necessary.\n\nBut even if it is a client-side bug, it has been in the wild for a long\ntime, and it may be worth having the server side handle this more\ngracefully.\n\n> When run in stateless mode, continuing on without first checking that\n> the client request has reached EOF can result in a bad file descriptor\n> during get_common_commits.\n\nWhen you say \"bad file descriptor\" that makes me think we're getting\nEBADF after trying to use a closed descriptor. But we'd die() as soon as\nthe pkt-line code tries to read and gets eof, right?\n\nThat's also worth fixing, but I want to make sure I understand the\nproblem completely. I think this part of the commit message would be a\ngood place to talk about the real-world effects:\n\n  - the client doesn't care; by definition it has hung up at this point\n    and will keep going with its next request\n\n  - likewise, the server doesn't care in terms of its response; by\n    definition it is stateless, so the next request the client makes\n    will start fresh\n\n  - it is annoying for server admins who get a bunch of useless logs\n    with \"remote end hung up unexpected\", or if they are tracking exit\n    codes from upload-pack\n\nAs somebody who has admin'd a busy git site, unexpected client network\ndrops are just a fact of life and you have to look past them in your\nlogs. But I still think it's worth keeping it as uncluttered as possible\nand having upload-pack handle this without an error message.\n\n> Instead, upload-pack should gently peek for an EOF between the sending\n> of shallow/unshallow lines (followed by flush) and the reading of client\n> haves. If the client has hung up at this point, exit normally.\n\nShould we do this only if we saw a deepen line? From my reading of the\nclient code, that's the only thing that would cause this early request.\nI don't know if there's any particular advantage to being more strict\nhere.\n\nIf we're _not_ going to be strict, then I actually wonder if we ought to\nsimply teach get_common_commits() that seeing an EOF is OK in stateless\nmode, wherever it comes. It can't possibly impact the correctness of the\nprotocol conversation (since we're stateless and the client is gone),\nbut maybe it's useful if you're trying to count how often clients really\ndo hang up.\n\n> --- /dev/null\n> +++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n\nWe usually try to group related tests by number. Maybe t5705 would be a\nbetter spot? I also wondered if this could go into t5704, but its title\nis \"protocol violations\". It's not clear to me yet if this is a\nviolation that happens to be mostly harmless, or something we need to be\ndoing. :)\n\n> +test_expect_success 'upload-pack outputs flush and exits ok' '\n> +\ttest_commit initial &&\n> +\thead=$(git rev-parse HEAD) &&\n> +\thexsz=$(test_oid hexsz) &&\n> +\n> +\tprintf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n> +\t\t$(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n\nWe have a helper function that makes this a bit easier to read:\n\ndiff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\nindex f8385a7ebd..1108401e8f 100755\n--- a/t/t9904-upload-pack-stateless-timely-eof.sh\n+++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n@@ -9,10 +9,13 @@ D=$(pwd)\n test_expect_success 'upload-pack outputs flush and exits ok' '\n \ttest_commit initial &&\n \thead=$(git rev-parse HEAD) &&\n-\thexsz=$(test_oid hexsz) &&\n \n-\tprintf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n-\t\t$(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n+\t{\n+\t\tpacketize \"want $head\" &&\n+\t\tpacketize \"shallow $head\" &&\n+\t\tpacketize \"deepen 1\" &&\n+\t\tprintf \"0000\"\n+\t} >request &&\n \n \tgit upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n \n\n> +\tgit upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n\nYou can just use \".\" here, which is a little shorter. It's not entirely\ncosmetic; the difference between $PWD and $(pwd) on Windows always trips\nme up, so I try to avoid using either whenever I can. ;)\n\nIt would be nice if we could test this through a real use of Git, but it\nmight not be worth the hassle. I guess we'd have to mine the apache logs\nin one of our http test scripts to see if upload-pack failed. And I\nguess if we _do_ change the client side to stop sending the extra\nrequest but want to treat historical clients more gracefully, we'd still\nneed a manual test like this.\n\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n>  \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n\nThe actual fix looks correct to me, modulo the alternatives I raised\nearlier.\n\nThis function only handles the v0 protocol. For v2, we end up in\nupload_pack_v2(). But my reading of the client side do_fetch_pack_v2()\nis that it _doesn't_ send this extra request. And a simple test seems to\nconfirm it. Which gives me further pause as to whether the extra request\nis necessary for v0.\n\n\nWell, that review ended up a bit longer than I had imagined. So let me\nadd what I should have said at the top: welcome to the Git list, and\nthanks for looking into this issue. :)\n\n-Peff\n"},{"id":"408719","messageId":"CANo+1gv0otfjRexAVW6E+yPEvGLo55_tQWN-81mQMm4U0seCJQ@mail.gmail.com","threadId":"54532","inReplyTo":"20201030044012.GA3259692@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T07:47:29Z","receivedAt":"2020-10-30T07:47:45Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"Thanks for the welcome! I appreciate the lengthy feedback, because\nabove all else I'd love to understand the low-level workings of git\nbetter than I do now.\n\nOn Thu, Oct 29, 2020 at 9:40 PM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Oct 29, 2020 at 06:40:59PM -0700, Daniel Duvall wrote:\n>\n> > During stateless packfile negotiation, it is normal behavior for\n> > stateless RPC clients (e.g. git-remote-curl) to send multiple\n> > upload-pack requests with the first containing only the\n> > wants/shallows/deepens/filters followed by a flush.\n>\n> Hmm, is this normal? I'd expect it to send a \"done\" after the flush, and\n> indeed that's what happens if I try it. I see:\n>\n> E.g., here's GIT_TRACE_CURL output from a v0 request (fetching into an\n> empty repo):\n>\n>  Send data: 00a8want 4b379b7a1b97790f805c365b1e58b75b70d3d904 multi_ack_\n>  Send data: detailed no-done side-band-64k thin-pack ofs-delta deepen-si\n>  Send data: nce deepen-not agent=git/2.29.2.477.g2cec8aa0af.00000009done\n>  Send data: .\n>  Info: upload completely sent off: 181 out of 181 bytes\n>\n> However, if I add --depth 1 to my fetch, I get:\n>\n>   Send data: 00a8want 4b379b7a1b97790f805c365b1e58b75b70d3d904 multi_ack_\n>   Send data: detailed no-done side-band-64k thin-pack ofs-delta deepen-si\n>   Send data: nce deepen-not agent=git/2.29.2.477.g2cec8aa0af.000cdeepen 1\n>   Send data: 0000\n>   Info: upload completely sent off: 184 out of 184 bytes\n>\n> Which maybe makes sense if we need the shallow response from the server\n> to determine the next step of the request. It doesn't matter for my\n> trivial case here (we end up resending the same request with a \"done\"\n> added), but I guess it could in other cases.\n\nRight. This is what I was observing too—in a trivial case, the same\nroundtrip being made again with nothing additional but \"done\". I\nshould have clarified that I thought it was normal only when a depth\nwas provided. I've been learning the packfile negotiation protocols as\nI've been debugging this issue, so I probably should have reserved my\nassertion of \"normal\" for when I have a firm grasp of them. :)\n\n>\n> > When run in stateless mode, continuing on without first checking that\n> > the client request has reached EOF can result in a bad file descriptor\n> > during get_common_commits.\n>\n> When you say \"bad file descriptor\" that makes me think we're getting\n> EBADF after trying to use a closed descriptor. But we'd die() as soon as\n> the pkt-line code tries to read and gets eof, right?\n\nRight. It's still a valid file descriptor but a premature die() upon\nEOF. I'll clarify that.\n\n>\n> That's also worth fixing, but I want to make sure I understand the\n> problem completely. I think this part of the commit message would be a\n> good place to talk about the real-world effects:\n>\n>   - the client doesn't care; by definition it has hung up at this point\n>     and will keep going with its next request\n\nThat's true in the case where the server doesn't surface the non-zero\nexit code from git-upload-pack—which results in git-http-backend\nexisting non-zero as well. An apache setup I used in testing doesn't\nseem to care about the failure—it responds 200 and so the client is\nhappy—but in our (dayjob) case, we're running a Phabricator instance\nwhich handles a non-zero exit from git-http-backend by responding 500,\nand the client dies.\n\n$ git fetch --depth 1\nhttps://phabricator.wikimedia.org/source/phabricator.git HEAD\nerror: RPC failed; HTTP 500 curl 22 The requested URL returned error: 500\nfatal: the remote end hung up unexpectedly\n\n>\n>   - likewise, the server doesn't care in terms of its response; by\n>     definition it is stateless, so the next request the client makes\n>     will start fresh\n\nThat makes sense to me.\n\n>\n>   - it is annoying for server admins who get a bunch of useless logs\n>     with \"remote end hung up unexpected\", or if they are tracking exit\n>     codes from upload-pack\n\nYes! Phabricator tries to work around this by detecting this stderr\noutput with a regex, which doesn't seem to hold up too well over time.\n\nSee https://secure.phabricator.com/D21484\n\nThis is actually how I started down this rabbithole. It seemed odd to\nme that git-http-backend was exiting non-zero in the first place. Then\nagain, I didn't much understand packfile negotiation, so it was just a\nhunch that there was some kind of bug. Knowing more about the protocol\nnow, it definitely seems buggy.\n\n>\n> As somebody who has admin'd a busy git site, unexpected client network\n> drops are just a fact of life and you have to look past them in your\n> logs. But I still think it's worth keeping it as uncluttered as possible\n> and having upload-pack handle this without an error message.\n>\n> > Instead, upload-pack should gently peek for an EOF between the sending\n> > of shallow/unshallow lines (followed by flush) and the reading of client\n> > haves. If the client has hung up at this point, exit normally.\n>\n> Should we do this only if we saw a deepen line? From my reading of the\n> client code, that's the only thing that would cause this early request.\n> I don't know if there's any particular advantage to being more strict\n> here.\n\nI'm not sure what would be more correct. It seems to go against the\n\"server doesn't care\" in a stateless case to be that strict, but maybe\nthere are additional benefits to strictness I'm not aware of.\n\n>\n> If we're _not_ going to be strict, then I actually wonder if we ought to\n> simply teach get_common_commits() that seeing an EOF is OK in stateless\n> mode, wherever it comes. It can't possibly impact the correctness of the\n> protocol conversation (since we're stateless and the client is gone),\n> but maybe it's useful if you're trying to count how often clients really\n> do hang up.\n\nI originally took that approach, but gently handling an EOF in the\nget_common_commits loop resulted in a NAK being sent back because of:\n\n                if (packet_reader_read(reader) != PACKET_READ_NORMAL) {\n                        [...]\n                        if (data->have_obj.nr == 0 || data->multi_ack)\n                                packet_write_fmt(1, \"NAK\\n\");\n                        [...]\n                        if (data->stateless_rpc)\n                                exit(0);\n                        [...]\n                }\n\nwhich the client died on with an \"expected shallow list\" message. I\ndidn't see a straightforward way of modifying the conditions _inside_\nthe loop while ensuring I wasn't changing any expected behavior upon\nEOF.\n\n>\n> > --- /dev/null\n> > +++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n>\n> We usually try to group related tests by number. Maybe t5705 would be a\n> better spot? I also wondered if this could go into t5704, but its title\n> is \"protocol violations\". It's not clear to me yet if this is a\n> violation that happens to be mostly harmless, or something we need to be\n> doing. :)\n\nAh! Okay, I was wondering about that, whether the numbered prefixes\nwere serial or otherwise meaningful. I'll try to group it\nappropriately.\n\n>\n> > +test_expect_success 'upload-pack outputs flush and exits ok' '\n> > +     test_commit initial &&\n> > +     head=$(git rev-parse HEAD) &&\n> > +     hexsz=$(test_oid hexsz) &&\n> > +\n> > +     printf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n> > +             $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n>\n> We have a helper function that makes this a bit easier to read:\n>\n> diff --git a/t/t9904-upload-pack-stateless-timely-eof.sh b/t/t9904-upload-pack-stateless-timely-eof.sh\n> index f8385a7ebd..1108401e8f 100755\n> --- a/t/t9904-upload-pack-stateless-timely-eof.sh\n> +++ b/t/t9904-upload-pack-stateless-timely-eof.sh\n> @@ -9,10 +9,13 @@ D=$(pwd)\n>  test_expect_success 'upload-pack outputs flush and exits ok' '\n>         test_commit initial &&\n>         head=$(git rev-parse HEAD) &&\n> -       hexsz=$(test_oid hexsz) &&\n>\n> -       printf \"%04xwant %s\\n%04xshallow %s\\n000ddeepen 1\\n0000\" \\\n> -               $(($hexsz + 10)) $head $(($hexsz + 13)) $head >request &&\n> +       {\n> +               packetize \"want $head\" &&\n> +               packetize \"shallow $head\" &&\n> +               packetize \"deepen 1\" &&\n> +               printf \"0000\"\n> +       } >request &&\n>\n>         git upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n>\n>\n> > +     git upload-pack --stateless-rpc \"$(pwd)\" <request >actual &&\n>\n> You can just use \".\" here, which is a little shorter. It's not entirely\n> cosmetic; the difference between $PWD and $(pwd) on Windows always trips\n> me up, so I try to avoid using either whenever I can. ;)\n\nVery cool re: the helpers. I'll make those changes.\n\n>\n> It would be nice if we could test this through a real use of Git, but it\n> might not be worth the hassle. I guess we'd have to mine the apache logs\n> in one of our http test scripts to see if upload-pack failed. And I\n> guess if we _do_ change the client side to stop sending the extra\n> request but want to treat historical clients more gracefully, we'd still\n> need a manual test like this.\n\nYeah I just went with the minimal case I'm familiar with, but I'm game\nto set up a more thorough case with some guidance.\n\n>\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n> >                                  PACKET_READ_DIE_ON_ERR_PACKET);\n>\n> The actual fix looks correct to me, modulo the alternatives I raised\n> earlier.\n>\n> This function only handles the v0 protocol. For v2, we end up in\n> upload_pack_v2(). But my reading of the client side do_fetch_pack_v2()\n> is that it _doesn't_ send this extra request. And a simple test seems to\n> confirm it. Which gives me further pause as to whether the extra request\n> is necessary for v0.\n\nWhen I do a trace using v2, I see two roundtrip requests as well. I\nhaven't tested the exit status of git-upload-pack in that case\nhowever. It's getting late for me but I'll investigate tomorrow.\n\n>\n>\n> Well, that review ended up a bit longer than I had imagined. So let me\n> add what I should have said at the top: welcome to the Git list, and\n> thanks for looking into this issue. :)\n\nThanks again! I appreciate all the feedback.\n"},{"id":"408723","messageId":"20201030090902.GA3268509@coredump.intra.peff.net","threadId":"54532","inReplyTo":"CANo+1gv0otfjRexAVW6E+yPEvGLo55_tQWN-81mQMm4U0seCJQ@mail.gmail.com","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-30T09:09:02Z","receivedAt":"2020-10-30T09:09:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2020 at 12:47:29AM -0700, Daniel Duvall wrote:\n\n> >   - the client doesn't care; by definition it has hung up at this point\n> >     and will keep going with its next request\n> \n> That's true in the case where the server doesn't surface the non-zero\n> exit code from git-upload-pack—which results in git-http-backend\n> existing non-zero as well. An apache setup I used in testing doesn't\n> seem to care about the failure—it responds 200 and so the client is\n> happy—but in our (dayjob) case, we're running a Phabricator instance\n> which handles a non-zero exit from git-http-backend by responding 500,\n> and the client dies.\n> \n> $ git fetch --depth 1\n> https://phabricator.wikimedia.org/source/phabricator.git HEAD\n> error: RPC failed; HTTP 500 curl 22 The requested URL returned error: 500\n> fatal: the remote end hung up unexpectedly\n\nThanks, that's a really helpful data point. The flaw in my reasoning is\nthat the client connection is bidirectional: just because the client\nhung up does not mean they are not still listening to the remainder of\nwhat the server has to say.\n\nIt sounds like Phabricator should probably be ignoring the exit code\nfrom http-backend. Or at least doing so when the CGI managed to produce\nan HTTP status code, and assuming that Git's response was either\nwell-formed, or that the client will realize it was broken. (Or possibly\nhttp-backend should be less eager to pass along a non-zero exit code,\nbut it amounts to the same thing).\n\nBut regardless, we should make sure that upload-pack is doing the right\nthing at least for this case.\n\n> > If we're _not_ going to be strict, then I actually wonder if we ought to\n> > simply teach get_common_commits() that seeing an EOF is OK in stateless\n> > mode, wherever it comes. It can't possibly impact the correctness of the\n> > protocol conversation (since we're stateless and the client is gone),\n> > but maybe it's useful if you're trying to count how often clients really\n> > do hang up.\n> \n> I originally took that approach, but gently handling an EOF in the\n> get_common_commits loop resulted in a NAK being sent back because of:\n> \n>                 if (packet_reader_read(reader) != PACKET_READ_NORMAL) {\n>                         [...]\n>                         if (data->have_obj.nr == 0 || data->multi_ack)\n>                                 packet_write_fmt(1, \"NAK\\n\");\n>                         [...]\n>                         if (data->stateless_rpc)\n>                                 exit(0);\n>                         [...]\n>                 }\n> \n> which the client died on with an \"expected shallow list\" message. I\n> didn't see a straightforward way of modifying the conditions _inside_\n> the loop while ensuring I wasn't changing any expected behavior upon\n> EOF.\n\nI was thinking something more like:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2b128e4ad8..f6d3ef3e13 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -520,10 +520,18 @@ static int get_common_commits(struct upload_pack_data *data,\n \n \tfor (;;) {\n \t\tconst char *arg;\n+\t\tenum packet_read_status status;\n \n \t\treset_timeout(data->timeout);\n \n-\t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL) {\n+\t\tstatus = packet_reader_read(reader);\n+\t\tif (status == PACKET_READ_EOF) {\n+\t\t\tif (data->stateless_rpc)\n+\t\t\t\texit(0);\n+\t\t\tdie(\"the remote end hung up unexpectedly\");\n+\t\t}\n+\n+\t\tif (status != PACKET_READ_NORMAL) {\n \t\t\tif (data->multi_ack == MULTI_ACK_DETAILED\n \t\t\t    && got_common\n \t\t\t    && !got_other\n\nBut after reading the rest of your response, I think it probably does\nmake sense to be more strict here, and only handle the unexpected EOF\nbefore entering get_common_commits(). I don't _think_ it should matter\nto the client, because this whole \"status != PACKET_READ_NORMAL\" path\nwould never be entered with the current code (we'd die() when we see the\nEOF inside packet_reader_read()).\n\n> > This function only handles the v0 protocol. For v2, we end up in\n> > upload_pack_v2(). But my reading of the client side do_fetch_pack_v2()\n> > is that it _doesn't_ send this extra request. And a simple test seems to\n> > confirm it. Which gives me further pause as to whether the extra request\n> > is necessary for v0.\n> \n> When I do a trace using v2, I see two roundtrip requests as well. I\n> haven't tested the exit status of git-upload-pack in that case\n> however. It's getting late for me but I'll investigate tomorrow.\n\nThere's an extra round-trip in v2: we probe for v2 and get the\ncapabilities in the initial request, then we get the ref advertisement,\nthen we start the object negotiation. In v0 the ref advertisement is\nlumped into the initial response.\n\nHere's what I see with:\n\n  $ git init\n  $ GIT_TRACE_CURL=$PWD/trace.out \\\n      git -c protocol.version=2 fetch --depth 1 \\\n      https://github.com/git/git master\n  $ perl -lne '/(Send data:|Info: upload completely sent) .*/ and print $&' trace.out\n  Send data: 0014command=ls-refs.0024agent=git/2.29.2.477.g2cec8aa0af0001\n  Send data: 0009peel.000csymrefs.0016ref-prefix master.001bref-prefix re\n  Send data: fs/master.0020ref-prefix refs/tags/master.0021ref-prefix ref\n  Send data: s/heads/master.0023ref-prefix refs/remotes/master.0028ref-pr\n  Send data: efix refs/remotes/master/HEAD.001aref-prefix refs/tags/.0000\n  Info: upload completely sent off: 300 out of 300 bytes\n  Send data: 0011command=fetch0024agent=git/2.29.2.477.g2cec8aa0af0001000\n  Send data: dthin-pack000dofs-delta000cdeepen 10032want ad27df6a5cff694a\n  Send data: dd500ab8c7f97234feb4a91f.0009done.0000\n  Info: upload completely sent off: 158 out of 158 bytes\n\nSo the first one POST is getting the ref advertisement, and the second\none is the full negotiation request, including the \"done\".\n\nI'm still uncertain whether it could all be done in one request for v0.\nBut one possible solution is: let's not care. If v2 does it correctly,\nthat's the future anyway (or present; it's now the default in v2.29).\nAnd the change you're proposing in upload-pack would be desirable anyway\nto help deal with older clients.\n\nIf that's the route we go, we should make sure the commit message\nexplains it.\n\n-Peff\n"},{"id":"408767","messageId":"xmqq7dr7369a.fsf@gitster.c.googlers.com","threadId":"54532","inReplyTo":"1604029402-19593-1-git-send-email-dan@mutual.io","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-30T18:42:25Z","receivedAt":"2020-10-30T18:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Duvall <dan@mutual.io> writes:\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 3b858eb..2b128e4 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n>  \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n>  \n>  \t\treceive_needs(&data, &reader);\n> -\t\tif (data.want_obj.nr) {\n> +\n> +\t\t/*\n> +\t\t * An EOF at this exact point in negotiation should be\n> +\t\t * acceptable from stateless clients as they will consume the\n> +\t\t * shallow list before doing subsequent rpc with haves/etc.\n> +\t\t */\n> +\t\tif (data.stateless_rpc)\n> +\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n> +\n> +\t\tif (data.want_obj.nr &&\n> +\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n> +\t\t\treader.options ^= PACKET_READ_GENTLE_ON_EOF;\n\nI am a bit puzzled why it is sensible to\n\n  (1) unconditionally\n  (2) toggle\n\nthe GENTLE bit.\n\n>  \t\t\tget_common_commits(&data, &reader);\n>  \t\t\tcreate_pack_file(&data, NULL);\n>  \t\t}\n\nIf we are not doing stateless_rpc, we call get_common_commits() with\na reader that is gentle on eof, which is not an intended behaviour\nchange, no?\n\nI would have understood if this were more like\n\n\tif (data.stateless_rpc)\n\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n\n\tif (data.want_obj.nr &&\n\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n\t\tif (data.stateless_rpc)\n\t\t\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n\ni.e. only when we know we set the bit when the bit was originally\nclear, revert to the original state.\n\n\tNote. initially I thought we may need to check the original\n\tvalue of the bit in reader.options before flipping it on,\n\tbut this packet_reader has freshly been initialized in the\n\tinner block we see here, so we know that nobody other than\n\tthis new code would have set the bit.\n\nOr for that matter, just unconditionally turn it off, e.g.\n\n\tif (data.want_obj.nr &&\n\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n\t\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n\nPuzzled...\n"},{"id":"408775","messageId":"20201030223504.45978-1-dan@mutual.io","threadId":"54532","inReplyTo":"1604022059-18527-1-git-send-email-dan@mutual.io","subject":"[PATCH v2] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T22:35:04Z","receivedAt":"2020-10-30T22:35:17Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"During stateless packfile negotiation where a depth is given, stateless RPC\nclients (e.g. git-remote-curl) will send multiple upload-pack requests with\nthe first containing only the wants/shallows/deepens/filters and the\nsubsequent containing haves/done.\n\nWhen upload-pack handles such requests, entering get_common_commits without\nfirst whether the client has hung up can result in unexpected EOF during the\nnegotiation loop and a die() with message \"fatal: the remote end hung up\nunexpectedly\".\n\nReal world effects include:\n\n - A client speaking to git-http-backend via a server that doesn't check the\n   exit codes of CGIs (e.g. mod_cgi) doesn't know and doesn't care about the\n   fatal. It continues to process the response body as normal.\n\n - A client speaking to a server that does check the exit code and returns an\n   errant HTTP status as a result will fail with the message \"error: RPC\n   failed; HTTP 500 curl 22 The requested URL returned error: 500.\"\n\n - Admins running servers that surface the failure must workaround it by\n   patching code that handles execution of git-http-backend to ignore exit\n   codes or take other heuristic approaches.\n\n - Admins may have to deal with \"hung up unexpectedly\" log spam related to the\n   failures even in cases where the exit code isn't surfaced as an HTTP\n   server-side error status.\n\nTo avoid these EOF related fatals, have upload-pack gently peek for an EOF\nbetween the sending of shallow/unshallow lines (followed by flush) and the\nreading of client haves. If the client has hung up at this point, exit\nnormally.\n\nSigned-off-by: Daniel Duvall <dan@mutual.io>\n\n---\n\nChanges in v2:\n - Replaced unconditional flipping (XOR) of PACKET_READ_GENTLE_ON_EOF bit w/\n   `&= ~` to flip it back off (as it was when reader was initialized in\n   previous clause)\n - Renamed test filename to group with other upload-pack related tests\n - Refactored test using packetize helper\n - Clarified in commit message that file descriptor is still valid but client\n   hangup/EOF is the core issue\n - Added possible real-world effects of bug to commit message as suggested\n\n---\n t/t5705-upload-pack-stateless-shallow-eof.sh | 24 ++++++++++++++++++++\n upload-pack.c                                | 13 ++++++++++-\n 2 files changed, 36 insertions(+), 1 deletion(-)\n create mode 100755 t/t5705-upload-pack-stateless-shallow-eof.sh\n\ndiff --git a/t/t5705-upload-pack-stateless-shallow-eof.sh b/t/t5705-upload-pack-stateless-shallow-eof.sh\nnew file mode 100755\nindex 0000000000..cc9d4baa0b\n--- /dev/null\n+++ b/t/t5705-upload-pack-stateless-shallow-eof.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='stateless upload-pack gently handles EOF just after want/shallow/depth/flush'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'upload-pack outputs flush and exits ok' '\n+\ttest_commit initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\n+\t{\n+\t\tpacketize \"want $head\" &&\n+\t\tpacketize \"shallow $head\" &&\n+\t\tpacketize \"deepen 1\" &&\n+\t\tprintf \"0000\"\n+\t} >request &&\n+\n+\tprintf \"0000\" >expect &&\n+\tgit upload-pack --stateless-rpc . <request >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..5dc8e1f844 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t\treceive_needs(&data, &reader);\n-\t\tif (data.want_obj.nr) {\n+\n+\t\t/*\n+\t\t * An EOF at this exact point in negotiation should be\n+\t\t * acceptable from stateless clients as they will consume the\n+\t\t * shallow list before doing subsequent rpc with haves/etc.\n+\t\t */\n+\t\tif (data.stateless_rpc)\n+\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n+\n+\t\tif (data.want_obj.nr &&\n+\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n+\t\t\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n \t\t\tget_common_commits(&data, &reader);\n \t\t\tcreate_pack_file(&data, NULL);\n \t\t}\n-- \n2.29.1.1.ge14d223\n\n"},{"id":"408776","messageId":"CANo+1gsquxyCSGhkjmtN2AazyuBN8ht_msxyVq4aFVFse-y1CQ@mail.gmail.com","threadId":"54532","inReplyTo":"xmqq7dr7369a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T22:38:50Z","receivedAt":"2020-10-30T22:39:05Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"On Fri, Oct 30, 2020 at 11:42 AM Junio C Hamano <gitster@pobox.com> wrote:\n> I am a bit puzzled why it is sensible to\n>\n>   (1) unconditionally\n>   (2) toggle\n>\n> the GENTLE bit.\n\nThat was pure carelessness on my part in flipping it instead of using\n`&= ~`. Fixed in v2\n"},{"id":"408777","messageId":"xmqqzh4346sb.fsf@gitster.c.googlers.com","threadId":"54532","inReplyTo":"20201030223504.45978-1-dan@mutual.io","subject":"Re: [PATCH v2] upload-pack: allow stateless client EOF just prior to haves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-30T23:45:40Z","receivedAt":"2020-10-30T23:45:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">  t/t5705-upload-pack-stateless-shallow-eof.sh | 24 ++++++++++++++++++++\n>  upload-pack.c                                | 13 ++++++++++-\n>  2 files changed, 36 insertions(+), 1 deletion(-)\n>  create mode 100755 t/t5705-upload-pack-stateless-shallow-eof.sh\n\nYuck.  Adding a new test file, only to hold a single test?\n\nI'd rather see it folded in an existing test, if it makes sense.\n\nOf course if we do not have any test for upload-pack, that's a\ndifferent story, but I suspect we already do test the command in\nexisting scripts.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 3b858eb457..5dc8e1f844 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n>  \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n>  \n>  \t\treceive_needs(&data, &reader);\n> -\t\tif (data.want_obj.nr) {\n> +\n> +\t\t/*\n> +\t\t * An EOF at this exact point in negotiation should be\n> +\t\t * acceptable from stateless clients as they will consume the\n> +\t\t * shallow list before doing subsequent rpc with haves/etc.\n> +\t\t */\n> +\t\tif (data.stateless_rpc)\n> +\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n> +\n> +\t\tif (data.want_obj.nr &&\n> +\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n> +\t\t\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n>  \t\t\tget_common_commits(&data, &reader);\n>  \t\t\tcreate_pack_file(&data, NULL);\n>  \t\t}\n\nThanks.\n"},{"id":"408778","messageId":"20201031023901.48193-1-dan@mutual.io","threadId":"54532","inReplyTo":"1604022059-18527-1-git-send-email-dan@mutual.io","subject":"[PATCH v3] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-31T02:39:02Z","receivedAt":"2020-10-31T02:39:37Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"During stateless packfile negotiation where a depth is given, stateless\nRPC clients (e.g. git-remote-curl) will send multiple upload-pack\nrequests with the first containing only the\nwants/shallows/deepens/filters and the subsequent containing haves/done.\n\nWhen upload-pack handles such requests, entering get_common_commits\nwithout checking whether the client has hung up can result in unexpected\nEOF during the negotiation loop and a die() with message \"fatal: the\nremote end hung up unexpectedly\".\n\nReal world effects include:\n\n - A client speaking to git-http-backend via a server that doesn't check\n   the exit codes of CGIs (e.g. mod_cgi) doesn't know and doesn't care\n   about the fatal. It continues to process the response body as normal.\n\n - A client speaking to a server that does check the exit code and\n   returns an errant HTTP status as a result will fail with the message\n   \"error: RPC failed; HTTP 500 curl 22 The requested URL returned error:\n   500.\"\n\n - Admins running servers that surface the failure must workaround it by\n   patching code that handles execution of git-http-backend to ignore exit\n   codes or take other heuristic approaches.\n\n - Admins may have to deal with \"hung up unexpectedly\" log spam related\n   to the failures even in cases where the exit code isn't surfaced as an\n   HTTP server-side error status.\n\nTo avoid these EOF related fatals, have upload-pack gently peek for an\nEOF between the sending of shallow/unshallow lines (followed by flush)\nand the reading of client haves. If the client has hung up at this\npoint, exit normally.\n\nSigned-off-by: Daniel Duvall <dan@mutual.io>\n---\n\nChanges in v2:\n - Replaced unconditional flipping (XOR) of PACKET_READ_GENTLE_ON_EOF bit w/\n   `&= ~` to flip it back off (as it was when reader was initialized in\n   previous clause)\n - Renamed test filename to group with other upload-pack related tests\n - Refactored test using packetize helper\n - Clarified in commit message that file descriptor is still valid but client\n   hangup/EOF is the core issue\n - Added possible real-world effects of bug to commit message as suggested\n\nChanges in v3:\n - Moved test into existing t/t5530-upload-pack-error.sh\n\n---\n t/t5530-upload-pack-error.sh | 17 +++++++++++++++++\n upload-pack.c                | 13 ++++++++++++-\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 205a2631e7..9dd2d2457a 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -88,6 +88,23 @@ test_expect_success 'upload-pack fails due to error in pack-objects enumeration'\n \tgrep \"pack-objects died\" output.err\n '\n \n+test_expect_success 'upload-pack tolerates EOF just after stateless client wants' '\n+\ttest_commit initial &&\n+\thead=$(git rev-parse HEAD) &&\n+\n+\t{\n+\t\tpacketize \"want $head\" &&\n+\t\tpacketize \"shallow $head\" &&\n+\t\tpacketize \"deepen 1\" &&\n+\t\tprintf \"0000\"\n+\t} >request &&\n+\n+\tprintf \"0000\" >expect &&\n+\n+\tgit upload-pack --stateless-rpc . <request >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create empty repository' '\n \n \tmkdir foo &&\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b858eb457..5dc8e1f844 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1344,7 +1344,18 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t\t   PACKET_READ_DIE_ON_ERR_PACKET);\n \n \t\treceive_needs(&data, &reader);\n-\t\tif (data.want_obj.nr) {\n+\n+\t\t/*\n+\t\t * An EOF at this exact point in negotiation should be\n+\t\t * acceptable from stateless clients as they will consume the\n+\t\t * shallow list before doing subsequent rpc with haves/etc.\n+\t\t */\n+\t\tif (data.stateless_rpc)\n+\t\t\treader.options |= PACKET_READ_GENTLE_ON_EOF;\n+\n+\t\tif (data.want_obj.nr &&\n+\t\t    packet_reader_peek(&reader) != PACKET_READ_EOF) {\n+\t\t\treader.options &= ~PACKET_READ_GENTLE_ON_EOF;\n \t\t\tget_common_commits(&data, &reader);\n \t\t\tcreate_pack_file(&data, NULL);\n \t\t}\n-- \n2.29.2\n\n"},{"id":"408779","messageId":"CANo+1guedZiZVbnANsSPCiYbKWpJ_tjJ7vc04Ap7EVZzzp869A@mail.gmail.com","threadId":"54532","inReplyTo":"xmqqzh4346sb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-31T02:42:27Z","receivedAt":"2020-10-31T02:42:41Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"On Fri, Oct 30, 2020 at 4:45 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> >  t/t5705-upload-pack-stateless-shallow-eof.sh | 24 ++++++++++++++++++++\n> >  upload-pack.c                                | 13 ++++++++++-\n> >  2 files changed, 36 insertions(+), 1 deletion(-)\n> >  create mode 100755 t/t5705-upload-pack-stateless-shallow-eof.sh\n>\n> I'd rather see it folded in an existing test, if it makes sense.\n\nWith v3 I've moved it into t/t5530-upload-pack-error.sh. That test\nfile seemed to mostly contain errant cases, but I thought it might be\nclose enough as this was previously an errant case. If there's a\nbetter place for it I'm happy to move it again.\n"},{"id":"408780","messageId":"xmqqv9er3u6l.fsf@gitster.c.googlers.com","threadId":"54532","inReplyTo":"CANo+1guedZiZVbnANsSPCiYbKWpJ_tjJ7vc04Ap7EVZzzp869A@mail.gmail.com","subject":"Re: [PATCH v2] upload-pack: allow stateless client EOF just prior to haves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-31T04:17:54Z","receivedAt":"2020-10-31T04:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Duvall <dan@mutual.io> writes:\n\n> On Fri, Oct 30, 2020 at 4:45 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> >  t/t5705-upload-pack-stateless-shallow-eof.sh | 24 ++++++++++++++++++++\n>> >  upload-pack.c                                | 13 ++++++++++-\n>> >  2 files changed, 36 insertions(+), 1 deletion(-)\n>> >  create mode 100755 t/t5705-upload-pack-stateless-shallow-eof.sh\n>>\n>> I'd rather see it folded in an existing test, if it makes sense.\n>\n> With v3 I've moved it into t/t5530-upload-pack-error.sh. That test\n> file seemed to mostly contain errant cases, but I thought it might be\n> close enough as this was previously an errant case. If there's a\n> better place for it I'm happy to move it again.\n\n5530 is good.  It is not too big or full of unrelated tests, and it\nis about sanity in the low level protocol exchange.\n\nThanks.\n"},{"id":"409042","messageId":"xmqqeelaqh7m.fsf@gitster.c.googlers.com","threadId":"54532","inReplyTo":"20201030090902.GA3268509@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T21:10:53Z","receivedAt":"2020-11-03T21:16:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm still uncertain whether it could all be done in one request for v0.\n> But one possible solution is: let's not care. If v2 does it correctly,\n> that's the future anyway (or present; it's now the default in v2.29).\n> And the change you're proposing in upload-pack would be desirable anyway\n> to help deal with older clients.\n>\n> If that's the route we go, we should make sure the commit message\n> explains it.\n\nYeah, I'd agree that punting on v0 and making sure the current\nversion would work well is good enough.\n\nI lost track and am not sure what's the current status of the topic\nis.  Is v3 [*1*] the latest and satisfactory one?\n\nThanks.\n\n\n[Reference]\n*1* https://lore.kernel.org/git/20201031023901.48193-1-dan@mutual.io/\n"},{"id":"409077","messageId":"20201104133342.GB3030480@coredump.intra.peff.net","threadId":"54532","inReplyTo":"xmqqeelaqh7m.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:33:42Z","receivedAt":"2020-11-04T13:33:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2020 at 01:10:53PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I'm still uncertain whether it could all be done in one request for v0.\n> > But one possible solution is: let's not care. If v2 does it correctly,\n> > that's the future anyway (or present; it's now the default in v2.29).\n> > And the change you're proposing in upload-pack would be desirable anyway\n> > to help deal with older clients.\n> >\n> > If that's the route we go, we should make sure the commit message\n> > explains it.\n> \n> Yeah, I'd agree that punting on v0 and making sure the current\n> version would work well is good enough.\n> \n> I lost track and am not sure what's the current status of the topic\n> is.  Is v3 [*1*] the latest and satisfactory one?\n\nYeah, I just read over v3 again and it looks good to me. Thanks, Daniel!\n\n-Peff\n"},{"id":"409081","messageId":"CANo+1gu2pi+JXV6a3fBFA+w_zCfQb80fJze2T3Ed1ri7+=KZGA@mail.gmail.com","threadId":"54532","inReplyTo":"20201104133342.GB3030480@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-11-04T14:06:44Z","receivedAt":"2020-11-04T14:07:01Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"On Wed, Nov 4, 2020 at 5:33 AM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Nov 03, 2020 at 01:10:53PM -0800, Junio C Hamano wrote:\n>\n> > Yeah, I'd agree that punting on v0 and making sure the current\n> > version would work well is good enough.\n> >\n> > I lost track and am not sure what's the current status of the topic\n> > is.  Is v3 [*1*] the latest and satisfactory one?\n>\n> Yeah, I just read over v3 again and it looks good to me. Thanks, Daniel!\n>\n> -Peff\n\nContributing this tiny patch was a rewarding experience, getting to\nknow Git more intimately and collaborating with you all. Thanks so\nmuch!\n\nKindly,\nDaniel\n"},{"id":"409121","messageId":"xmqqft5pm11z.fsf@gitster.c.googlers.com","threadId":"54532","inReplyTo":"CANo+1gu2pi+JXV6a3fBFA+w_zCfQb80fJze2T3Ed1ri7+=KZGA@mail.gmail.com","subject":"Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T18:25:44Z","receivedAt":"2020-11-04T18:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Duvall <dan@mutual.io> writes:\n\n> On Wed, Nov 4, 2020 at 5:33 AM Jeff King <peff@peff.net> wrote:\n>>\n>> On Tue, Nov 03, 2020 at 01:10:53PM -0800, Junio C Hamano wrote:\n>>\n>> > Yeah, I'd agree that punting on v0 and making sure the current\n>> > version would work well is good enough.\n>> >\n>> > I lost track and am not sure what's the current status of the topic\n>> > is.  Is v3 [*1*] the latest and satisfactory one?\n>>\n>> Yeah, I just read over v3 again and it looks good to me. Thanks, Daniel!\n>>\n>> -Peff\n>\n> Contributing this tiny patch was a rewarding experience, getting to\n> know Git more intimately and collaborating with you all. Thanks so\n> much!\n\nThanks, both of you.  Marked v3 to be ready for 'next'.\n"}]}