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 </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
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68