Commit Diff

Commit:
c811fd62d1fbec7ac5e58e10b242c3d4f8b4cdb4
From:
Stefan Sperling <stsp@stsp.name>
Date:
Message:
fix an issue where 'git fetch' would error or hang against gotd If Git has more than 16 have-lines to send it will send a flush-pkt followed by more have-lines. Due to a misunderstanding on my part, gotd didn't like this because it assumed that the flush-pkt terminates the list. Add a test coverage in a new file which we can use to test Git interop issues. Fixes a problem seen by Thomas Adam upon git fetch from got.g.o.
Actions:
Patch | Tree
commit - 6e5370b0f38658d91de41782c4fec170db11606c
commit + c811fd62d1fbec7ac5e58e10b242c3d4f8b4cdb4
blob - 5e75f4801579643302462cdd46c22f21d2bf3880
blob + 772c35e98bb5ed0874ea08d5d0d4437001a7480e
--- gotd/session_read.c
+++ gotd/session_read.c
@@ -57,8 +57,7 @@ enum gotd_session_read_state {
 	GOTD_STATE_EXPECT_LIST_REFS,
 	GOTD_STATE_EXPECT_CAPABILITIES,
 	GOTD_STATE_EXPECT_WANT,
-	GOTD_STATE_EXPECT_HAVE,
-	GOTD_STATE_EXPECT_DONE,
+	GOTD_STATE_EXPECT_HAVE_OR_DONE,
 	GOTD_STATE_DONE,
 };
 
@@ -545,7 +544,8 @@ session_dispatch_client(int fd, short events, void *ar
 			err = forward_want(client, &imsg);
 			break;
 		case GOTD_IMSG_HAVE:
-			if (gotd_session.state != GOTD_STATE_EXPECT_HAVE) {
+			if (gotd_session.state !=
+			    GOTD_STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected have-line received");
 				break;
@@ -559,8 +559,8 @@ session_dispatch_client(int fd, short events, void *ar
 			break;
 		case GOTD_IMSG_FLUSH:
 			if (gotd_session.state != GOTD_STATE_EXPECT_WANT && 
-			    gotd_session.state != GOTD_STATE_EXPECT_HAVE &&
-			    gotd_session.state != GOTD_STATE_EXPECT_DONE) {
+			    gotd_session.state !=
+			    GOTD_STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected flush-pkt received");
 				break;
@@ -581,15 +581,17 @@ session_dispatch_client(int fd, short events, void *ar
 			log_debug("received flush-pkt from uid %d",
 			    client->euid);
 			if (gotd_session.state == GOTD_STATE_EXPECT_WANT) {
-				gotd_session.state = GOTD_STATE_EXPECT_HAVE;
-				log_debug("uid %d: expecting have-lines",
-				    client->euid);
-			} else if (gotd_session.state == GOTD_STATE_EXPECT_HAVE) {
-				gotd_session.state = GOTD_STATE_EXPECT_DONE;
+				gotd_session.state =
+				    GOTD_STATE_EXPECT_HAVE_OR_DONE;
+				log_debug("uid %d: expecting have-lines "
+				    "or 'done'", client->euid);
+			} else if (gotd_session.state ==
+			    GOTD_STATE_EXPECT_HAVE_OR_DONE) {
 				client->accept_flush_pkt = 1;
-				log_debug("uid %d: expecting 'done'",
-				    client->euid);
-			} else if (gotd_session.state != GOTD_STATE_EXPECT_DONE) {
+				log_debug("uid %d: expecting more have-lines "
+				    "or 'done'", client->euid);
+			} else if (gotd_session.state !=
+			    GOTD_STATE_EXPECT_HAVE_OR_DONE) {
 				/* should not happen, see above */
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected client state");
@@ -597,8 +599,8 @@ session_dispatch_client(int fd, short events, void *ar
 			}
 			break;
 		case GOTD_IMSG_DONE:
-			if (gotd_session.state != GOTD_STATE_EXPECT_HAVE &&
-			    gotd_session.state != GOTD_STATE_EXPECT_DONE) {
+			if (gotd_session.state !=
+			    GOTD_STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_REQUEST,
 				    "unexpected flush-pkt received");
 				break;
blob - 16d6eeece5252ed4c35915742908b5fda451090d
blob + e03137e6800e1939de4210d638076544517c3445
--- lib/serve.c
+++ lib/serve.c
@@ -774,8 +774,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
 	enum protostate {
 		STATE_EXPECT_WANT,
 		STATE_EXPECT_MORE_WANT,
-		STATE_EXPECT_HAVE,
-		STATE_EXPECT_DONE,
+		STATE_EXPECT_HAVE_OR_DONE,
 		STATE_DONE,
 	};
 	enum protostate curstate = STATE_EXPECT_WANT;
@@ -798,8 +797,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
 		if (n == 0) {
 			if (curstate != STATE_EXPECT_WANT &&
 			    curstate != STATE_EXPECT_MORE_WANT &&
-			    curstate != STATE_EXPECT_HAVE &&
-			    curstate != STATE_EXPECT_DONE) {
+			    curstate != STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unexpected flush packet received");
 				goto done;
@@ -828,20 +826,19 @@ serve_read(int infd, int outfd, int gotd_sock, const c
 
 			if (curstate == STATE_EXPECT_WANT ||
 			    curstate == STATE_EXPECT_MORE_WANT ||
-			    curstate == STATE_EXPECT_HAVE) {
+			    curstate == STATE_EXPECT_HAVE_OR_DONE) {
 				err = forward_flushpkt(&ibuf);
 				if (err)
 					goto done;
 			}
-			if (curstate == STATE_EXPECT_HAVE && !have_ack) {
+			if (curstate == STATE_EXPECT_HAVE_OR_DONE &&
+			    !have_ack) {
 				err = send_nak(outfd, chattygot);
 				if (err)
 					goto done;
 			}
 			if (curstate == STATE_EXPECT_MORE_WANT)
-				curstate = STATE_EXPECT_HAVE;
-			else
-				curstate = STATE_EXPECT_DONE;
+				curstate = STATE_EXPECT_HAVE_OR_DONE;
 		} else if (n >= 5 && strncmp(buf, "want ", 5) == 0) {
 			if (curstate != STATE_EXPECT_WANT &&
 			    curstate != STATE_EXPECT_MORE_WANT) {
@@ -856,22 +853,18 @@ 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 &&
-			    curstate != STATE_EXPECT_DONE) {
+			if (curstate != STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unexpected 'have' packet");
 				goto done;
-			}
-			if (curstate == STATE_EXPECT_HAVE) {
-				err = recv_have(&have_ack, outfd, &ibuf,
-				    buf, n, chattygot);
-				if (err)
-					goto done;
-				seen_have = 1;
 			}
+			err = recv_have(&have_ack, outfd, &ibuf,
+			    buf, n, chattygot);
+			if (err)
+				goto done;
+			seen_have = 1;
 		} else if (n == 5 && strncmp(buf, "done\n", 5) == 0) {
-			if (curstate != STATE_EXPECT_HAVE &&
-			    curstate != STATE_EXPECT_DONE) {
+			if (curstate != STATE_EXPECT_HAVE_OR_DONE) {
 				err = got_error_msg(GOT_ERR_BAD_PACKET,
 				    "unexpected 'done' packet");
 				goto done;
blob - c38bd07b843aa78426c9e628f609604a4d815417
blob + 4482c735241818f4fa534feaaf319e44c6d23018
--- regress/gotd/Makefile
+++ regress/gotd/Makefile
@@ -5,7 +5,8 @@ REGRESS_TARGETS=test_repo_read test_repo_read_group \
 	test_repo_read_bad_user test_repo_read_bad_group \
 	test_repo_write test_repo_write_empty test_request_bad \
 	test_repo_write_protected test_repo_write_readonly \
-	test_email_notification test_http_notification
+	test_email_notification test_http_notification \
+	test_git_interop
 NOOBJ=Yes
 CLEANFILES=gotd.conf
 
@@ -277,4 +278,10 @@ test_http_notification: prepare_test_repo start_gotd_h
 		'env $(GOTD_TEST_ENV) sh ./http_notification.sh'
 	@$(GOTD_STOP_CMD) 2>/dev/null
 
+test_git_interop: prepare_test_repo start_gotd_rw
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./test_git_interop.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
 .include <bsd.regress.mk>
blob - /dev/null
blob + 96d81eef9e29a430bae977c16bfab9eceb33997f (mode 644)
--- /dev/null
+++ regress/gotd/test_git_interop.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+#
+# Copyright (c) 2024 Stefan Sperling <stsp@openbsd.org>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+. ../cmdline/common.sh
+. ./common.sh
+
+test_fetch_with_git_history_walk() {
+	local testroot=`test_init fetch_with_git_history_walk 1`
+
+	git clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Create new commits on a branch which doesn't exist in gotd repo.
+	# We want Git to send a flush-pkt followed by more have-lines. This
+	# requires at least 16 commits (fetch-pack.c:INITIAL_FLUSH 16).
+	git -C $testroot/repo-clone branch newbranch
+	for i in `seq 24`; do
+		echo $i >> $testroot/repo-clone/file$i
+		git -C $testroot/repo-clone add file$i
+		git_commit $testroot/repo-clone -m "add file$i"
+		ret=$?
+		if [ $ret -ne 0 ]; then
+			echo "git commit failed unexpectedly" >&2
+			test_done "$testroot" "1"
+			return 1
+		fi
+	done
+
+	git clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone2 \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# create new commits on main branch, and push to gotd
+	for i in `seq 2`; do
+		echo $i >> $testroot/repo-clone2/file$i
+		git -C $testroot/repo-clone2 add file$i
+		git_commit $testroot/repo-clone2 -m "add file$i"
+		ret=$?
+		if [ $ret -ne 0 ]; then
+			echo "git commit failed unexpectedly" >&2
+			test_done "$testroot" "1"
+			return 1
+		fi
+	done
+
+	git -C $testroot/repo-clone2 push -q origin main
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git push failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Fetching changes into the first repository clone should work.
+	# This used to fail because gotd rejected additional have-lines
+	# once Git had sent a flush-pkt.
+	git -C $testroot/repo-clone fetch -q 2> $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "git fetch failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	test_done "$testroot" "0"
+}
+
+run_test test_fetch_with_git_history_walk