On Sun, Dec 25, 2022 at 11:11:36AM +0100, Stefan Sperling wrote:
> On Sat, Dec 24, 2022 at 10:18:45PM +0300, Mikhail wrote:
> > This patch renames previously committed req_wrong_commit.sh to
> > wrong_req.sh and expands it to test some more wrong requests.
> > 
> > Tests list:
> > 
> > - request wrong commit id (as was in req_wrong_commit.sh)
> > - request line containing 0000 as length (flush packet with payload)
> > - request line containing 0004 as length + body (empty request)
> > - request line containing 0002 as length + body (length is too small)
> > - request line containing ffff as length + body (length is too large)
> > - request line containing unknown capabilities
> > - request line containing unknown repository
> > - request line containing repository with 255 symbols as the name
> 
> Expanding test coverage is very useful, thank you!
> 
> Terms like "bad" or "invalid" are more commonly used for such requests,
> instead of "wrong".
> 
> If we are going to rename things, I would suggest to use the phrase
> "request_bad" consistently, instead of using a mix of "req_wrong" and
> "wrong_req".
> 
> Something like:
> 
> run_test test_request_bad_commit
> run_test test_request_bad_length_zero
> run_test test_request_bad_length_empty
> run_test test_request_bad_length_small
> run_test test_request_bad_length_large
> run_test test_request_bad_capabilities
> run_test test_request_bad_repository
> run_test test_request_bad_large_repo_name
> 
> and name the script file request_bad.sh
> 
> As minor tweaks above, say "capabilities" instead of "caps" (to avoid
> confusion with "capital letters") and "repo" instead of "rep" (we use
> "repo" consistently in our code as abbreviation of "repository").

Done, next version:

diff /home/misha/work/got
commit - 1abb18e1777172a9f4149a0f50c4cecfd024f02c
path + /home/misha/work/got
blob - 4df499c68e7f0c6fcb75510088da6bca9b323ba7
file + regress/gotd/Makefile
--- regress/gotd/Makefile
+++ regress/gotd/Makefile
@@ -1,7 +1,7 @@
 REGRESS_TARGETS=test_repo_read test_repo_read_group \
 	test_repo_read_denied_user test_repo_read_denied_group \
 	test_repo_read_bad_user test_repo_read_bad_group \
-	test_repo_write test_repo_write_empty test_req_wrong_commit
+	test_repo_write test_repo_write_empty test_request_bad
 NOOBJ=Yes
 
 .PHONY: ensure_root prepare_test_repo check_test_repo start_gotd
@@ -187,9 +187,9 @@ test_req_wrong_commit: prepare_test_repo_empty start_g
 	@$(GOTD_STOP_CMD) 2>/dev/null
 	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
 	
-test_req_wrong_commit: prepare_test_repo_empty start_gotd_ro
+test_request_bad: prepare_test_repo_empty start_gotd_ro
 	@-$(GOTD_TRAP); su -m ${GOTD_TEST_USER} -c \
-		'env $(GOTD_TEST_ENV) sh ./req_wrong_commit.sh'
+		'env $(GOTD_TEST_ENV) sh ./request_bad.sh'
 	@$(GOTD_STOP_CMD) 2>/dev/null
 
 .include <bsd.regress.mk>
blob - 97e05fec18b363301a607a121ab4b595f1fbd3c7
file + /dev/null
--- regress/gotd/req_wrong_commit.sh
+++ /dev/null
@@ -1,54 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2022 Mikhail Pchelin <misha@freebsd.org>
-#
-# Permission to use, copy, modify, and distribute this software for any
-# purpose with or without fee is hereby granted, provided that the above
-# copyright notice and this permission notice appear in all copies.
-#
-# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
-# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
-# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-
-. ../cmdline/common.sh
-. ./common.sh
-
-test_req_wrong_commit() {
-	local testroot=`test_init req_wrong_commit`
-
-	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
-side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
-		git-upload-pack '/test-repo' > $testroot/stdout \
-		2>$testroot/stderr
-
-	echo -n "0041ERR object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
-not found" > $testroot/stdout.expected
-
-	echo "gotsh: object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
-not found" > $testroot/stderr.expected
-
-	# We use OpenBSD cmp(1) offset extension
-	cmp -s $testroot/stdout $testroot/stdout.expected 112 0
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		echo "unexpected stdout" >&2
-		test_done "$testroot" "1"
-		return 1
-	fi
-
-	cmp -s $testroot/stderr $testroot/stderr.expected
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		echo "unexpected stderr" >&2
-		test_done "$testroot" "1"
-		return 1
-	fi
-	test_done "$testroot" "$ret"
-}
-
-test_parseargs "$@"
-run_test test_req_wrong_commit
blob - /dev/null
file + regress/gotd/request_bad.sh (mode 644)
--- /dev/null
+++ regress/gotd/request_bad.sh
@@ -0,0 +1,290 @@
+#!/bin/sh
+#
+# Copyright (c) 2022 Mikhail Pchelin <misha@freebsd.org>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+. ../cmdline/common.sh
+. ./common.sh
+
+# Non-existent commit
+test_request_bad_commit() {
+	local testroot=`test_init request_bad_commit`
+
+	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
+side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "0041ERR object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
+not found" > $testroot/stdout.expected
+
+	echo "gotsh: object aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
+not found" > $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 112 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
+# Zero pkt-len (as flush packet with payload)
+test_request_bad_length_zero() {
+	local testroot=`test_init test_request_bad_length_zero`
+
+	echo "0000want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
+side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "00000028ERR unexpected flush packet received" \
+		> $testroot/stdout.expected
+
+	echo "gotsh: unexpected flush packet received" \
+		> $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
+# 0004 (empty)
+test_request_bad_length_empty() {
+	local testroot=`test_init test_request_bad_length_empty`
+
+	echo "0004want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
+side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
+		> $testroot/stdout.expected
+
+	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
+# Pkt-len too small
+test_request_bad_length_small() {
+	local testroot=`test_init test_request_bad_length_small`
+
+	echo "0002want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
+side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
+		> $testroot/stdout.expected
+
+	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+# Pkt-len too large
+test_request_bad_length_large() {
+	local testroot=`test_init test_request_bad_length_large`
+
+	echo "ffffwant aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa multi_ack \
+side-band-64k ofs-delta" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "00000008NAK\n0021ERR read: Bad file descriptor" \
+		> $testroot/stdout.expected
+
+	echo "gotsh: read: Bad file descriptor" > $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
+# Unknown feature
+test_request_bad_capabilities() {
+	local testroot=`test_init test_request_bad_capabilities`
+
+	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
+bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/test-repo' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "00000025ERR unexpected want-line received" \
+		> $testroot/stdout.expected
+
+	echo "gotsh: unexpected want-line received" > $testroot/stderr.expected
+
+	# We use OpenBSD cmp(1) offset extension
+	cmp -s $testroot/stdout $testroot/stdout.expected 108 0
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
+# Unknown repository
+test_request_bad_repository() {
+	local testroot=`test_init test_request_bad_repository`
+
+	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
+bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/XXXX-XXXX' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "001fERR no git repository found" > $testroot/stdout.expected
+
+	echo "gotsh: no git repository found" > $testroot/stderr.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+
+}
+
+# Repository with name of 255 symbols
+test_request_bad_large_repo_name() {
+	local testroot=`test_init test_request_bad_large_repo_name`
+
+	echo "0054want aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaa \
+bbbbbbbbbbbbb ccccccccc" | ssh ${GOTD_DEVUSER}@127.0.0.1 \
+		git-upload-pack '/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' > $testroot/stdout \
+		2>$testroot/stderr
+
+	echo -n "0018ERR buffer too small" > $testroot/stdout.expected
+
+	echo "gotsh: buffer too small" > $testroot/stderr.expected
+
+	cmp -s $testroot/stdout $testroot/stdout.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stdout" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	cmp -s $testroot/stderr $testroot/stderr.expected
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "unexpected stderr" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+
+}
+test_parseargs "$@"
+run_test test_request_bad_commit
+run_test test_request_bad_length_zero
+run_test test_request_bad_length_empty
+run_test test_request_bad_length_small
+run_test test_request_bad_length_large
+run_test test_request_bad_capabilities
+run_test test_request_bad_repository
+run_test test_request_bad_large_repo_name