From: Mark Jamsek Subject: Re: fix gotd rejecting multiple have lines To: gameoftrees@openbsd.org Date: Wed, 18 Jan 2023 22:36:58 +1100 On 23-01-18 12:14PM, Stefan Sperling wrote: > 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? You never cease to amaze with how quick you resolve these! I can confirm the test passes with your fix and fails without it, 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 < +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 > -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68