From: Andrew Hewus Fresh <andrew@afresh1.com>
Subject: Improve fw_update(8) package database lock failure
To: tech@openbsd.org
Date: Sun, 13 Oct 2024 16:57:29 -0700

One thing I was worried about was if the perl process locking the
package database exited and we didn't notice we could possibly corrupt
the package db.

Unfortunately, handling a co-process unexpectedly exiting doesn't seem
to be very robust in ksh, so we watch for SIGCHLD and check if we
started the lock process and whether that job still exists.
Unfortunately, we do that for _every_ child exit and since this is a
shell script there are lots, but it still seems OK.

If anyone has suggestions on improved handling for this, for example why
I seem to get infinite recursion if I don't disable the CHLD trap
handler (I assume we trap the exit from `jobs` or the $(...) subshell),
but it is annoying.

Comments, improvements, should I just ignore this, or an OK?


Index: fw_update.sh
===================================================================
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
diff -u -p -r1.57 fw_update.sh
--- fw_update.sh	12 Oct 2024 23:56:23 -0000	1.57
+++ fw_update.sh	13 Oct 2024 23:46:37 -0000
@@ -57,6 +57,7 @@ warn()   { echo    "$*" >&"$WARN_FD"; }
 
 cleanup() {
 	set +o errexit # ignore errors from killing ftp
+	trap "" CHLD
 
 	if [ -d "$FD_DIR" ]; then
 		echo "" >&"$STATUS_FD"
@@ -77,6 +78,20 @@ cleanup() {
 }
 trap cleanup EXIT
 
+chld_handler() {
+	ret=$?
+	trap "" CHLD
+	# Not sure why I can't use `! jobs %?lock_db >/dev/null`
+	# but it doesn't exit.`
+	if [ "${LOCKPID:-}" ] && [ ! "$( jobs %?lock_db )" ]; then
+		warn "Package database lock has failed"
+		return 1
+	fi
+	trap chld_handler CHLD
+	return $ret
+}
+trap chld_handler CHLD
+
 tmpdir() {
 	local _i=1 _dir
 
@@ -290,6 +305,8 @@ lock_db() {
 
 	set -o monitor
 	perl <<-'EOL' |&
+		# lock_db
+		# This above comment is so "jobs" can recognize it with a good name
 		no lib ('/usr/local/libdata/perl5/site_perl');
 		use v5.36;
 		use OpenBSD::PackageInfo qw< lock_db >;
@@ -312,7 +329,7 @@ lock_db() {
 		}
 		lock_db(0, 'OpenBSD::FwUpdateState');
 
-		say "$$ $waited";
+		say $waited;
 
 		# Wait for STDOUT to be readable, which won't happen
 		# but if our parent exits unexpectedly it will close.
@@ -320,9 +337,10 @@ lock_db() {
 		vec($rin, fileno(STDOUT), 1) = 1;
 		select $rin, '', '', undef;
 EOL
+	LOCKPID=$!
 	set +o monitor
 
-	read -rp LOCKPID _waited
+	read -rp _waited
 
 	if ((_waited)); then
 		! ((VERBOSE)) && status "${0##*/}:"