Download raw body.
gotd: handle early client disconnections
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
gotd: handle early client disconnections