From: Omar Polo Subject: gotd: handle early client disconnections To: gameoftrees@openbsd.org Date: Sun, 22 Jan 2023 12:48:00 +0100 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 $testroot/stderr + cat < $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 $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