From: Mark Jamsek Subject: Re: gotd: handle early client disconnections To: Omar Polo Cc: gameoftrees@openbsd.org Date: Mon, 23 Jan 2023 00:08:40 +1100 On 23-01-22 12:48PM, Omar Polo wrote: > 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' I like this! Agree with your reasoning and changes, and I can reproduce without your fix. ok with tiny nit/question inline > 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); I can't see why the line has been wrapped; chattygot fits on the above line? > 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 + done > + > + got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \ > + 2> $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 + 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 > -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68