{"thread":{"id":"54531","subject":"[PATCH] Fix upload-pack EOF death in normal stateless negotiation","startedAt":"2020-10-29T23:53:41Z","lastAt":"2020-10-30T01:26:20Z","messageCount":3,"participants":["Daniel Duvall","Bryan Turner"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408687","messageId":"CANo+1gtVRj30-JNPFpqc_m3BSEFzcb8-T+-uJTFvnNuBZYYxQQ@mail.gmail.com","threadId":"54531","inReplyTo":null,"subject":"[PATCH] Fix upload-pack EOF death in normal stateless negotiation","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-29T23:53:26Z","receivedAt":"2020-10-29T23:53:41Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"Hi there,\n\nI noticed while debugging an issue in Phabricator where shallow git\nfetches over HTTP were failing with 500 errors that `git-http-backend`\nseemed to exit abnormally after the first request even though the\nresponse was otherwise correct—and when the error is not surfaced to\nthe client, the packfile negotiation via `git-remote-curl` seems to\ncontinue normally.[1]\n\nI think this patch fixes the issue by having `upload-pack` gently\nhandle EOFs at a specific point in negotiation—after shallow/unshallow\nlines have been returned to the client (followed by flush) but before\nthe client sends its haves.\n\nThis is my first contribution here so hopefully I'm understanding the\npackfile negotiation protocol correctly and included my test in the\nright place.\n\nKindly,\nDaniel\n\n[1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317\n\n\nFrom df738e146fde368ca8935d41b586093dd164cc2a Mon Sep 17 00:00:00 2001\nFrom: Daniel Duvall \nDate: Thu, 29 Oct 2020 14:32:27 -0700\nSubject: [PATCH] upload-pack: allow stateless client EOF just prior to haves\n\nDuring 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---\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)\" 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":"408689","messageId":"CAGyf7-HOkt3bCkJ+qshb0AFZSt2f=NmvHb3Gh1m2dzLWpYHozQ@mail.gmail.com","threadId":"54531","inReplyTo":"CANo+1gtVRj30-JNPFpqc_m3BSEFzcb8-T+-uJTFvnNuBZYYxQQ@mail.gmail.com","subject":"Re: [PATCH] Fix upload-pack EOF death in normal stateless negotiation","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2020-10-30T00:32:20Z","receivedAt":"2020-10-30T00:32:34Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Thu, Oct 29, 2020 at 4:53 PM Daniel Duvall wrote:\n>\n> Hi there,\n>\n> I noticed while debugging an issue in Phabricator where shallow git\n> fetches over HTTP were failing with 500 errors that `git-http-backend`\n> seemed to exit abnormally after the first request even though the\n> response was otherwise correct—and when the error is not surfaced to\n> the client, the packfile negotiation via `git-remote-curl` seems to\n> continue normally.[1]\n>\n> I think this patch fixes the issue by having `upload-pack` gently\n> handle EOFs at a specific point in negotiation—after shallow/unshallow\n> lines have been returned to the client (followed by flush) but before\n> the client sends its haves.\n>\n> This is my first contribution here so hopefully I'm understanding the\n> packfile negotiation protocol correctly and included my test in the\n> right place.\n\nHave you read Documentation/SubmittingPatches? It has some guidelines\n(like submitting patches inline, rather than as attachments) you'll\nwant to ensure you've followed.\n\n>\n> Kindly,\n> Daniel\n>\n> [1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317\n"},{"id":"408690","messageId":"CANo+1gsdyU5sMRmV5OZxRZm5fqOFL3y-qFQN-PemdPKHFrJ8Mw@mail.gmail.com","threadId":"54531","inReplyTo":"CAGyf7-HOkt3bCkJ+qshb0AFZSt2f=NmvHb3Gh1m2dzLWpYHozQ@mail.gmail.com","subject":"Re: [PATCH] Fix upload-pack EOF death in normal stateless negotiation","fromName":"Daniel Duvall","fromEmail":"dan@mutual.io","sentAt":"2020-10-30T01:26:04Z","receivedAt":"2020-10-30T01:26:20Z","isPatch":true,"sender":{"key":"dan@mutual.io","avatar":"https://avatars.githubusercontent.com/u/239740?v=4"},"body":"Thanks, Bryan. I did read the guide but not thoroughly enough it\nseems. I'll give it another read and re-submit.\n\nOn Thu, Oct 29, 2020, 5:32 PM Bryan Turner wrote:\n>\n> On Thu, Oct 29, 2020 at 4:53 PM Daniel Duvall wrote:\n> >\n> > Hi there,\n> >\n> > I noticed while debugging an issue in Phabricator where shallow git\n> > fetches over HTTP were failing with 500 errors that `git-http-backend`\n> > seemed to exit abnormally after the first request even though the\n> > response was otherwise correct—and when the error is not surfaced to\n> > the client, the packfile negotiation via `git-remote-curl` seems to\n> > continue normally.[1]\n> >\n> > I think this patch fixes the issue by having `upload-pack` gently\n> > handle EOFs at a specific point in negotiation—after shallow/unshallow\n> > lines have been returned to the client (followed by flush) but before\n> > the client sends its haves.\n> >\n> > This is my first contribution here so hopefully I'm understanding the\n> > packfile negotiation protocol correctly and included my test in the\n> > right place.\n>\n> Have you read Documentation/SubmittingPatches? It has some guidelines\n> (like submitting patches inline, rather than as attachments) you'll\n> want to ensure you've followed.\n>\n> >\n> > Kindly,\n> > Daniel\n> >\n> > [1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317\n"}]}