"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix gotd rejecting multiple have lines
To:
gameoftrees@openbsd.org
Date:
Wed, 18 Jan 2023 12:14:23 +0100

Download raw body.

Thread
While testing 'git pull' against got.gameoftrees.org I came across
this error:

  fetch-pack: protocol error: bad band #69
  fatal: protocol error: bad pack header
  gotsh: unexpected 'have' packet

This occurs if there is a deep enough branch history to trigger
git into sending lots of have-lines until the server sends an ack.
Because this communication happens asynchronously we might already
be expecting a 'done' packet while more have-lines keep arriving.

Fix this problem and add a test to hopefully trigger the issue reliably.
Because this problem depends on timing it might not be 100% reproducible.
But the test kept failing reliably on my laptop at least.

This might also fix the spurious 'unexpected flush packet' errors seen with
'got fetch' while already up-to-date, but I have not yet confirmed this.

ok?

 fix an issue where gotd fails to accept multiple have-lines from clients
 
diff 6da1c69cd7747c70dfbe29c9fb66fa03fa985459 f654fdbfc59bee89b4c114780e28be9aa19757f3
commit - 6da1c69cd7747c70dfbe29c9fb66fa03fa985459
commit + f654fdbfc59bee89b4c114780e28be9aa19757f3
blob - 356daf210a6e33f99485b698040f4993bd37d9d3
blob + c32e0bf5066a6f1b2b11a59ed1e4200d990d37ba
--- gotd/session.c
+++ gotd/session.c
@@ -1102,7 +1102,7 @@ session_dispatch_listener(int fd, short events, void *
 				err = ensure_client_is_writing(client);
 				if (err)
 					break;
-			} else {
+			} else if (client->state != GOTD_STATE_EXPECT_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected flush-pkt received");
 				break;
@@ -1123,7 +1123,7 @@ session_dispatch_listener(int fd, short events, void *
 				log_debug("uid %d: expecting packfile",
 				    client->euid);
 				err = recv_packfile(client);
-			} else {
+			} else if (client->state != GOTD_STATE_EXPECT_DONE) {
 				/* should not happen, see above */
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected client state");
blob - 6e4c3ef0627fe92f92f4091b2926040aa3d9d9db
blob + 02c4cb511fe756c9101538c93243233f34941dd2
--- lib/serve.c
+++ lib/serve.c
@@ -899,7 +899,8 @@ serve_read(int infd, int outfd, int gotd_sock, const c
 			break;
 		if (n == 0) {
 			if (curstate != STATE_EXPECT_MORE_WANT &&
-			    curstate != STATE_EXPECT_HAVE) {
+			    curstate != STATE_EXPECT_HAVE &&
+			    curstate != STATE_EXPECT_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unexpected flush packet received");
 				goto done;
@@ -930,16 +931,21 @@ serve_read(int infd, int outfd, int gotd_sock, const c
 			if (curstate == STATE_EXPECT_WANT)
 				curstate = STATE_EXPECT_MORE_WANT;
 		} else if (n >= 5 && strncmp(buf, "have ", 5) == 0) {
-			if (curstate != STATE_EXPECT_HAVE) {
+			if (curstate != STATE_EXPECT_HAVE &&
+			    curstate != STATE_EXPECT_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unexpected 'have' packet");
 				goto done;
 			}
-			err = recv_have(&have_ack, outfd, &ibuf, buf, n,
-			    chattygot);
-			if (err)
-				goto done;
-			seen_have = 1;
+			if (curstate == STATE_EXPECT_HAVE) {
+				err = recv_have(&have_ack, outfd, &ibuf,
+				    buf, n, chattygot);
+				if (err)
+					goto done;
+				seen_have = 1;
+				if (have_ack)
+					curstate = STATE_EXPECT_DONE;
+			}
 		} else if (n == 5 && strncmp(buf, "done\n", 5) == 0) {
 			if (curstate != STATE_EXPECT_HAVE &&
 			    curstate != STATE_EXPECT_DONE) {
blob - cb89c0c20a28919ad7363ff1bba5419fdad3c7f1
blob + 907efb48181d5468b2ba8bcf55d39ca905be2686
--- regress/gotd/repo_write.sh
+++ regress/gotd/repo_write.sh
@@ -143,5 +143,116 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_fetch_more_history() {
+	local testroot=`test_init fetch_more_history 1`
+
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	got checkout -q $testroot/repo-clone $testroot/wt >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got checkout failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Create some more commit history on the main branch.
+	# History needs to be deep enough to trick 'git pull' into sending
+	# a lot of 'have' lines, which triggered a bug in gotd.
+	for i in `jot 50`; do
+		echo "more alpha" >> $testroot/wt/alpha
+		(cd $testroot/wt && got commit -m 'more changes' > /dev/null)
+	done
+	got send -b main -q -r $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# create a second clone to test an incremental fetch with later
+	got clone -q -m ${GOTD_TEST_REPO_URL} $testroot/repo-clone2
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	# same for Git, which used to fail:
+	# fetch-pack: protocol error: bad band #69
+	# fatal: protocol error: bad pack header
+	# gotsh: unexpected 'have' packet
+	git clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone3 \
+		>$testroot/stdout 2>$testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Create more commit history on the main branch
+	echo "more alpha" >> $testroot/wt/alpha
+	(cd $testroot/wt && got commit -m 'make changes' > /dev/null)
+	echo "more beta" >> $testroot/wt/beta
+	(cd $testroot/wt && got commit -m 'more changes' > /dev/null)
+	(cd $testroot/wt && got rm epsilon/zeta > /dev/null)
+	(cd $testroot/wt && got commit -m 'rm epsilon/zeta' > /dev/null)
+	got send -b main -q -r $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Verify that the new changes can be fetched
+	got fetch -q -r $testroot/repo-clone2
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got fetch failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	got tree -R -r $testroot/repo-clone2 > $testroot/stdout
+	cat > $testroot/stdout.expected <<EOF
+alpha
+beta
+gamma/
+gamma/delta
+psi/
+psi/new
+EOF
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	# Verify that git pull works, too
+	(cd $testroot/repo-clone3 && git pull -q > $testroot/stdout \
+		2> $testroot/stderr)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git pull failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
+
 test_parseargs "$@"
 run_test test_send_basic
+run_test test_fetch_more_history