From: Mikhail <mp39590@gmail.com>
Subject: Re: gotd crash while requesting non-existent commit id
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 19 Dec 2022 22:23:50 +0300

On Sun, Dec 04, 2022 at 05:17:51PM +0300, Mikhail wrote:
> On Sun, Dec 04, 2022 at 01:44:38PM +0100, Stefan Sperling wrote:
> > > --- /dev/null
> > > +++ regress/gotd/repo_req_wrong_commit.sh
> > 
> > > +	# Before 36c7cfbb2a gotd was crashing after invalid commit in "want"
> > > +	# line, here we check that we can successfully stop working gotd.
> > > +	$GOTD_STOP_CMD
> > > +	ret=$?
> > 
> > GOTD_STOP_CMD requires root privileges to succeed, but gotd test scripts
> > run as GOTD_TEST_USER. So this command should always fail. If it does not
> > fail then there is a problem.
> 
> I use 'doas make server-regress' to run tests after 'doas' my LOGNMAE
> is 'root', so my GOTD_TEST_USER is also root, that's why it worked for
> me, thanks for noticing.
> 
> > I am wondering if this check could be done in the Makefile instead, where
> > GOTD_STOP_CMD runs as root?
> 
> Yeah, definitely, I've moved the check to root Makefile and it's done
> now as in other tests, if I back out the change which fixes the original
> crash from gotd/gotd.c, I see the following:
> 
> ==== repo_req_wrong_commit ====
> test_wrong_commit ok
> *** Error 1 in regress/gotd (Makefile:186 'repo_req_wrong_commit': @../../gotctl/obj/gotctl -f /home/gotdev/gotd.sock stop 2>/dev/null)
> FAILED
> 
> Is it enough?
> 
> Next attempt:

Renewed test case, now it incorporates stderr from gotsh.

diff /home/misha/work/got
commit - b2b1792329f5adf1e755614e710d49388845293e
path + /home/misha/work/got
blob - 637c3892a77b86d3be16a1e964a55ed285543c41
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_repo_write test_repo_write_empty repo_req_wrong_commit
 NOOBJ=Yes
 
 .PHONY: ensure_root prepare_test_repo check_test_repo start_gotd
@@ -187,4 +187,9 @@ test_repo_write_empty: 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'
 	
+repo_req_wrong_commit: prepare_test_repo_empty start_gotd_ro
+	@-$(GOTD_TRAP); su -m ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_req_wrong_commit.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+
 .include <bsd.regress.mk>
blob - /dev/null
file + regress/gotd/repo_req_wrong_commit.sh (mode 644)
--- /dev/null
+++ regress/gotd/repo_req_wrong_commit.sh
@@ -0,0 +1,54 @@
+#!/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_wrong_commit() {
+	local testroot=`test_init 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_wrong_commit