[PATCH] Fix upload-pack EOF death in normal stateless negotiation

[PATCH] Fix upload-pack EOF death in normal stateless negotiation

[Daniel Duvall]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

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

[-- Attachment #2: 0001-upload-pack-allow-stateless-client-EOF-just-prior-to.patch --]
[-- Type: application/octet-stream, Size: 2671 bytes --]

From df738e146fde368ca8935d41b586093dd164cc2a Mon Sep 17 00:00:00 2001
From: Daniel Duvall <dan@mutual.io>
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)" <request >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

Re: [PATCH] Fix upload-pack EOF death in normal stateless negotiation

[Bryan Turner]

On Thu, Oct 29, 2020 at 4:53 PM Daniel Duvall <dan@mutual.io> wrote:
>
> 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.

Have you read Documentation/SubmittingPatches? It has some guidelines
(like submitting patches inline, rather than as attachments) you'll
want to ensure you've followed.

>
> Kindly,
> Daniel
>
> [1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317

Re: [PATCH] Fix upload-pack EOF death in normal stateless negotiation

[Daniel Duvall]

Thanks, Bryan. I did read the guide but not thoroughly enough it
seems. I'll give it another read and re-submit.

On Thu, Oct 29, 2020, 5:32 PM Bryan Turner <bturner@atlassian.com> wrote:
>
> On Thu, Oct 29, 2020 at 4:53 PM Daniel Duvall <dan@mutual.io> wrote:
> >
> > 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.
>
> Have you read Documentation/SubmittingPatches? It has some guidelines
> (like submitting patches inline, rather than as attachments) you'll
> want to ensure you've followed.
>
> >
> > Kindly,
> > Daniel
> >
> > [1] https://discourse.phabricator-community.org/t/git-fetches-with-depth-over-http-results-in-500-errors/4317