"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotd: handle early client disconnections
To:
gameoftrees@openbsd.org
Date:
Sun, 22 Jan 2023 12:48:00 +0100

Download raw body.

Thread
gotd fails to handle the early disconnection of the clients.  this has
the effect of causing new connections to hit the limits when they
shouldn't.

The fix is easy, just the first hunk in the diff below, ie. don't
ignore GOT_ERR_EOF.  Nothing in gotd_request (except for
gotd_imsg_recv) can return it.  We may still have something left
queued up to send, but it shouldn't be an issue since the client
closed the connection anyway.

I've discovered this gotd issue by playing with gotsh and forgetting
the path, i.e. `ssh gotdev@localhost git-upload-pack'.

So, I'm bundling also a tweak for gotsh to error earlier (by exporting
parse_command call it before socket(2).)  This means that connection
with an invalid cmdline now are _not_ counted against the user limits,
but I don't think it's a big issue.  (before they were counted only
'sometimes'.)

    before:
    % ssh gotdev@localhost git-receive-pack
    gotsh: bad packet received

    after:
    % ssh gotdev@localhost git-receive-pack
    usage: gotsh -c 'git-receive-pack|git-upload-pack repository-path'

For last, there's a very simple test case for it.  It first try to hit
the limits via `ssh', and then only via nc(1).  It's not committable
as-is (and btw needs `permit nopass $yourself as gotdev' in doas.conf)
but helps in reproducing the issue at least :)
Will probably drop the second part.


diff /home/op/w/got
commit - 65c2e81071bab7f9018a062b4d590e67e1a6f021
path + /home/op/w/got
blob - 823a2d9d22e1f4f65df19bbbb7539f88ae4ce5a2
file + gotd/gotd.c
--- gotd/gotd.c
+++ gotd/gotd.c
@@ -649,8 +649,7 @@ gotd_request(int fd, short events, void *arg)
 	}
 
 	if (err) {
-		if (err->code != GOT_ERR_EOF)
-			disconnect_on_error(client, err);
+		disconnect_on_error(client, err);
 	} else {
 		gotd_imsg_event_add(&client->iev);
 	}
blob - acc4545ba9aeab1061576f340ba1ba16ed227d59
file + gotsh/gotsh.c
--- gotsh/gotsh.c
+++ gotsh/gotsh.c
@@ -67,7 +67,7 @@ main(int argc, char *argv[])
 	char *unix_socket_path_env = getenv("GOTD_UNIX_SOCKET");
 	int gotd_sock = -1;
 	struct sockaddr_un	 sun;
-	char *gitcmd = NULL;
+	char *gitcmd = NULL, *command = NULL, *repo_path = NULL;
 
 #ifndef PROFILE
 	if (pledge("stdio recvfd unix unveil", NULL) == -1)
@@ -79,17 +79,16 @@ main(int argc, char *argv[])
 			usage();
 		if (asprintf(&gitcmd, "%s %s", argv[0], argv[1]) == -1)
 			err(1, "asprintf");
+		error = got_serve_parse_command(&command, &repo_path, gitcmd);
 	} else {
-		if (argc != 3 || strcmp(argv[1], "-c") != 0 ||
-		    (strncmp(argv[2], GOT_SERVE_CMD_SEND,
-		    strlen(GOT_SERVE_CMD_SEND)) != 0 &&
-		    (strncmp(argv[2], GOT_SERVE_CMD_FETCH,
-		    strlen(GOT_SERVE_CMD_FETCH)) != 0)))
+		if (argc != 3 || strcmp(argv[1], "-c") != 0)
 			usage();
-		gitcmd = strdup(argv[2]);
-		if (gitcmd == NULL)
-			err(1, "strdup");
+		error = got_serve_parse_command(&command, &repo_path, argv[2]);
 	}
+	if (error && error->code == GOT_ERR_BAD_PACKET)
+		usage();
+	if (error)
+		goto done;
 
 	if (unix_socket_path_env) {
 		if (strlcpy(unix_socket_path, unix_socket_path_env,
@@ -123,10 +122,12 @@ main(int argc, char *argv[])
 	if (pledge("stdio recvfd", NULL) == -1)
 		err(1, "pledge");
 #endif
-	error = got_serve(STDIN_FILENO, STDOUT_FILENO, gitcmd, gotd_sock,
-	    chattygot);
+	error = got_serve(STDIN_FILENO, STDOUT_FILENO, command, repo_path,
+	    gotd_sock, chattygot);
 done:
 	free(gitcmd);
+	free(command);
+	free(repo_path);
 	if (gotd_sock != -1)
 		close(gotd_sock);
 	if (error) {
blob - 180c362c726680bac4fbe3b5fd47dca9a5e166ae
file + include/got_serve.h
--- include/got_serve.h
+++ include/got_serve.h
@@ -17,5 +17,6 @@ const struct got_error *got_serve(int infd, int outfd,
 #define GOT_SERVE_CMD_SEND "git-receive-pack"
 #define GOT_SERVE_CMD_FETCH "git-upload-pack"
 
-const struct got_error *got_serve(int infd, int outfd, const char *gitcmd,
-    int gotd_sock, int chattygot);
+const struct got_error *got_serve_parse_command(char **, char **, const char *);
+const struct got_error *got_serve(int infd, int outfd, const char *command,
+    const char *repo_path, int gotd_sock, int chattygot);
blob - 043a4dd74cbffb62ce90eba9ec362db3ad6c61f5
file + lib/serve.c
--- lib/serve.c
+++ lib/serve.c
@@ -64,8 +64,8 @@ static const struct got_error *
 #endif
 };
 
-static const struct got_error *
-parse_command(char **command, char **repo_path, const char *gitcmd)
+const struct got_error *
+got_serve_parse_command(char **command, char **repo_path, const char *gitcmd)
 {
 	const struct got_error *err = NULL;
 	size_t len, cmdlen, pathlen;
@@ -1482,23 +1482,18 @@ got_serve(int infd, int outfd, const char *gitcmd, int
 }
 
 const struct got_error *
-got_serve(int infd, int outfd, const char *gitcmd, int gotd_sock, int chattygot)
+got_serve(int infd, int outfd, const char *command, const char *repo_path,
+    int gotd_sock, int chattygot)
 {
 	const struct got_error *err = NULL;
-	char *command = NULL, *repo_path = NULL;
 
-	err = parse_command(&command, &repo_path, gitcmd);
-	if (err)
-		return err;
-
 	if (strcmp(command, GOT_SERVE_CMD_FETCH) == 0)
 		err = serve_read(infd, outfd, gotd_sock, repo_path, chattygot);
 	else if (strcmp(command, GOT_SERVE_CMD_SEND) == 0)
-		err = serve_write(infd, outfd, gotd_sock, repo_path, chattygot);
+		err = serve_write(infd, outfd, gotd_sock, repo_path,
+		    chattygot);
 	else
 		err = got_error(GOT_ERR_BAD_PACKET);
 
-	free(command);
-	free(repo_path);
 	return err;
 }
blob - 113aa4cf7bd50234a0d5ea376652934c0ef7b007
file + regress/gotd/request_bad.sh
--- regress/gotd/request_bad.sh
+++ regress/gotd/request_bad.sh
@@ -293,6 +293,48 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_request_bad_no_repo() {
+	local testroot=`test_init test_request_bad_no_repo`
+
+	for i in `seq 10`; do
+		ssh ${GOTD_DEVUSER}@127.0.0.1 git-upload-pack \
+			>/dev/null 2>/dev/null </dev/null
+	done
+
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \
+		2> $testroot/stderr
+	cat <<EOF > $testroot/stderr.expected
+got-fetch-pack: could not find any branches to fetch
+got: could not find any branches to fetch
+EOF
+
+	if ! cmp -s "$testroot/stderr.expected" "$testroot/stderr"; then
+		echo "got clone failed for unexpected reason" >&2
+		diff -u "$testroot/stderr.expected" "$testroot/stderr"
+		test_done "$testroot" 1
+		return
+	fi
+
+	rm -rf "$testroot/repo-clone"
+
+	# try again by connecting to the daemon itself
+	for i in `seq 10`; do
+		doas -u "${GOTD_DEVUSER}" nc -w 1 -U "${GOTD_SOCK}" \
+			2>/dev/null </dev/null
+	done
+
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \
+		2> $testroot/stderr
+	if ! cmp -s "$testroot/stderr.expected" "$testroot/stderr"; then
+		echo "got clone failed for unexpected reason" >&2
+		diff -u "$testroot/stderr.expected" "$testroot/stderr"
+		test_done "$testroot" 1
+		return
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_request_bad_commit
 run_test test_request_bad_length_zero
@@ -302,3 +344,4 @@ run_test test_request_bad_large_repo_name
 run_test test_request_bad_capabilities
 run_test test_request_bad_repository
 run_test test_request_bad_large_repo_name
+run_test test_request_bad_no_repo