commit c811fd62d1fbec7ac5e58e10b242c3d4f8b4cdb4 from: Stefan Sperling date: Sun May 05 09:51:27 2024 UTC 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. 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 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 +# +# 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