From: Stefan Sperling Subject: fix gotd rejecting multiple have lines To: gameoftrees@openbsd.org Date: Wed, 18 Jan 2023 12:14:23 +0100 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 < $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