git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Daniel Duvall <dan@mutual.io>
To: git@vger.kernel.org
Subject: [PATCH] Fix upload-pack EOF death in normal stateless negotiation
Date: Thu, 29 Oct 2020 16:53:26 -0700	[thread overview]
Message-ID: <CANo+1gtVRj30-JNPFpqc_m3BSEFzcb8-T+-uJTFvnNuBZYYxQQ@mail.gmail.com> (raw)

[-- 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


             reply	other threads:[~2020-10-29 23:53 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29 23:53 Daniel Duvall [this message]
2020-10-30  0:32 ` Bryan Turner
2020-10-30  1:26   ` Daniel Duvall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANo+1gtVRj30-JNPFpqc_m3BSEFzcb8-T+-uJTFvnNuBZYYxQQ@mail.gmail.com \
    --to=dan@mutual.io \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH] Fix upload-pack EOF death in normal stateless negotiation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).