From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotd flush packets
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 20 Jan 2023 20:40:13 +0100

On Fri, Jan 20, 2023 at 08:04:51PM +0100, Omar Polo wrote:
> On 2023/01/20 19:57:59 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> >  - After sending our ref announcement we can receive a flush packet
> >    if the client decides it does not want anything.
> >  - The list of want lines during fetch is terminated by a flush packet.
> >  - The list of ref-updates lines during send is terminated by a flush packet.
> 
> that's my understanding as well

The previous patch was wrong in several ways.
The client can send multiple want/have lines, the patch only accepted one.
The client can send a flush packet before 'done'.
This broke 'git pull' from an older copy of the repo I keep around for testing.

This still prevents multiple flush packets in sequence, and still allows
lots of other shenanigans. I don't really want to invest more time into
fixing this before having some repeatable test cases, though.

diff refs/heads/main refs/heads/flushpkt
commit - 7713cc5e4f5544e81909670d592e89526ed86c9b
commit + 00f48d6b02f52795d1596995bb628a8fe45b9aa1
blob - 0bc48fac834b679822e3314db38590b9da8f3575
blob + c2bf744e16922aec4500424898af4499c5240308
--- gotd/session.c
+++ gotd/session.c
@@ -79,6 +79,7 @@ static struct gotd_session_client {
 	char				*packfile_path;
 	char				*packidx_path;
 	int				 nref_updates;
+	int				 accept_flush_pkt;
 } gotd_session_client;
 
 void gotd_session_sighdlr(int sig, short event, void *arg);
@@ -1037,10 +1038,12 @@ session_dispatch_listener(int fd, short events, void *
 				break;
 			if (!client->is_writing) {
 				client->state = GOTD_STATE_EXPECT_WANT;
+				client->accept_flush_pkt = 1;
 				log_debug("uid %d: expecting want-lines",
 				    client->euid);
 			} else if (client->is_writing) {
 				client->state = GOTD_STATE_EXPECT_REF_UPDATE;
+				client->accept_flush_pkt = 1;
 				log_debug("uid %d: expecting ref-update-lines",
 				    client->euid);
 			} else
@@ -1058,6 +1061,7 @@ session_dispatch_listener(int fd, short events, void *
 			err = ensure_client_is_reading(client);
 			if (err)
 				break;
+			client->accept_flush_pkt = 1;
 			err = forward_want(client, &imsg);
 			break;
 		case GOTD_IMSG_REF_UPDATE:
@@ -1077,6 +1081,7 @@ session_dispatch_listener(int fd, short events, void *
 			if (err)
 				break;
 			client->state = GOTD_STATE_EXPECT_MORE_REF_UPDATES;
+			client->accept_flush_pkt = 1;
 			break;
 		case GOTD_IMSG_HAVE:
 			if (client->state != GOTD_STATE_EXPECT_HAVE) {
@@ -1092,6 +1097,7 @@ session_dispatch_listener(int fd, short events, void *
 			err = forward_have(client, &imsg);
 			if (err)
 				break;
+			client->accept_flush_pkt = 1;
 			break;
 		case GOTD_IMSG_FLUSH:
 			if (client->state == GOTD_STATE_EXPECT_WANT ||
@@ -1109,6 +1115,19 @@ session_dispatch_listener(int fd, short events, void *
 				    "unexpected flush-pkt received");
 				break;
 			}
+			if (!client->accept_flush_pkt) {
+				err = got_error_msg(GOT_ERR_BAD_REQUEST,
+				    "unexpected flush-pkt received");
+				break;
+			}
+
+			/*
+			 * Accept just one flush packet at a time.
+			 * Future client state transitions will set this flag
+			 * again if another flush packet is expected.
+			 */
+			client->accept_flush_pkt = 0;
+
 			log_debug("received flush-pkt from uid %d",
 			    client->euid);
 			if (client->state == GOTD_STATE_EXPECT_WANT) {
@@ -1117,6 +1136,7 @@ session_dispatch_listener(int fd, short events, void *
 				    client->euid);
 			} else if (client->state == GOTD_STATE_EXPECT_HAVE) {
 				client->state = GOTD_STATE_EXPECT_DONE;
+				client->accept_flush_pkt = 1;
 				log_debug("uid %d: expecting 'done'",
 				    client->euid);
 			} else if (client->state ==
@@ -1144,6 +1164,7 @@ session_dispatch_listener(int fd, short events, void *
 			if (err)
 				break;
 			client->state = GOTD_STATE_DONE;
+			client->accept_flush_pkt = 1;
 			err = send_packfile(client);
 			break;
 		default:
@@ -1402,6 +1423,7 @@ session_main(const char *title, const char *repo_path,
 	gotd_session_client.fd = -1;
 	gotd_session_client.nref_updates = -1;
 	gotd_session_client.delta_cache_fd = -1;
+	gotd_session_client.accept_flush_pkt = 1;
 
 	imsg_init(&gotd_session.parent_iev.ibuf, GOTD_FILENO_MSG_PIPE);
 	gotd_session.parent_iev.handler = session_dispatch;