From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotsh serve_read error handling
To: gameoftrees@openbsd.org
Date: Sat, 21 Jan 2023 21:29:57 +1100

On 23-01-21 11:06AM, Stefan Sperling wrote:
> serve_read() is not jumping into the error path when a read error occurs.
> Fix this and adjust some tests accordingly which shows that the errors
> returned now are more reasonable.
> 
> The same problem exists in serve_write() but that an be fixed separately.
> 
> ok?

ok

> diff 695bc1ecccf24ece2b0b8a03a5f0d26ad5116a0e 11d907f5f1f58d1a983e27eb0de37b5d524427dd
> commit - 695bc1ecccf24ece2b0b8a03a5f0d26ad5116a0e
> commit + 11d907f5f1f58d1a983e27eb0de37b5d524427dd
> blob - 33fddb5601425a93824e79798b4bd2ee24a574a3
> blob + 043a4dd74cbffb62ce90eba9ec362db3ad6c61f5
> --- lib/serve.c
> +++ lib/serve.c
> @@ -896,7 +896,7 @@ serve_read(int infd, int outfd, int gotd_sock, const c
>  		buf[0] = '\0';
>  		err = got_pkt_readpkt(&n, infd, buf, sizeof(buf), chattygot);
>  		if (err)
> -			break;
> +			goto done;
>  		if (n == 0) {
>  			if (curstate != STATE_EXPECT_WANT &&
>  			    curstate != STATE_EXPECT_MORE_WANT &&
> blob - d01cd1bae40ee00fa47c8d4205b83088b4f36a73
> blob + 96339d43615d5d8fad7e4a300f64c7db2dc9cea8
> --- regress/cmdline/common.sh
> +++ regress/cmdline/common.sh
> @@ -24,6 +24,7 @@ export GOT_IGNORE_GITCONFIG=1
>  export GOT_LOG_DEFAULT_LIMIT=0
>  export GOT_TEST_ROOT="/tmp"
>  export GOT_IGNORE_GITCONFIG=1
> +export GOT_VERSION_STR=`got --version | cut -d ' ' -f2`
>  
>  export MALLOC_OPTIONS=S
>  
> blob - 2896db4ceb362abfd70987b33dcf5f158b418c91
> blob + 113aa4cf7bd50234a0d5ea376652934c0ef7b007
> --- regress/gotd/request_bad.sh
> +++ regress/gotd/request_bad.sh
> @@ -93,12 +93,16 @@ test_request_bad_length_empty() {
>  		| ssh ${GOTD_DEVUSER}@127.0.0.1 git-upload-pack '/test-repo' \
>  		> $testroot/stdout 2>$testroot/stderr
>  
> -	printf "00000008NAK\n0021ERR read: Bad file descriptor" \
> +	echo -n '006c0000000000000000000000000000000000000000 ' \
>  		> $testroot/stdout.expected
> +	printf "capabilities^{}\0" >> $testroot/stdout.expected
> +	echo -n " agent=got/${GOT_VERSION_STR} ofs-delta side-band-64k" \
> +		>> $testroot/stdout.expected
> +	echo -n '00000018ERR packet too short' >> $testroot/stdout.expected
>  
> -	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +	echo "gotsh: packet too short" > $testroot/stderr.expected
>  
> -	cmp -s $testroot/stdout.expected $testroot/stdout 0 108
> +	cmp -s $testroot/stdout.expected $testroot/stdout
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		echo "unexpected stdout" >&2
> @@ -125,12 +129,16 @@ test_request_bad_length_small() {
>  		| ssh ${GOTD_DEVUSER}@127.0.0.1 git-upload-pack '/test-repo' \
>  		> $testroot/stdout 2>$testroot/stderr
>  
> -	printf "00000008NAK\n0021ERR read: Bad file descriptor" \
> +	echo -n '006c0000000000000000000000000000000000000000 ' \
>  		> $testroot/stdout.expected
> +	printf "capabilities^{}\0" >> $testroot/stdout.expected
> +	echo -n " agent=got/${GOT_VERSION_STR} ofs-delta side-band-64k" \
> +		>> $testroot/stdout.expected
> +	echo -n '00000018ERR packet too short' >> $testroot/stdout.expected
>  
> -	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +	echo "gotsh: packet too short" > $testroot/stderr.expected
>  
> -	cmp -s $testroot/stdout.expected $testroot/stdout 0 108
> +	cmp -s $testroot/stdout.expected $testroot/stdout
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		echo "unexpected stdout" >&2
> @@ -157,12 +165,17 @@ test_request_bad_length_large() {
>  		| ssh ${GOTD_DEVUSER}@127.0.0.1 git-upload-pack '/test-repo' \
>  		> $testroot/stdout 2>$testroot/stderr
>  
> -	printf "00000008NAK\n0021ERR read: Bad file descriptor" \
> +	echo -n '006c0000000000000000000000000000000000000000 ' \
>  		> $testroot/stdout.expected
> +	printf "capabilities^{}\0" >> $testroot/stdout.expected
> +	echo -n " agent=got/${GOT_VERSION_STR} ofs-delta side-band-64k" \
> +		>> $testroot/stdout.expected
> +	echo -n '0000001eERR unexpected end of file' \
> +		>> $testroot/stdout.expected
>  
> -	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
> +	echo "gotsh: unexpected end of file" > $testroot/stderr.expected
>  
> -	cmp -s $testroot/stdout.expected $testroot/stdout 0 108
> +	cmp -s $testroot/stdout.expected $testroot/stdout
>  	ret=$?
>  	if [ $ret -ne 0 ]; then
>  		echo "unexpected stdout" >&2
> 

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