fix(rootfs/qemu-static): close 3 races flagged by codex review of 5359f27

Codex review of the previous lock-pattern commit (5359f27) found three
real holes:

1. trap_handler released the owner lock BEFORE taking the control lock.
   Between fd close and flock(control) a fresh builder could enter setup,
   prune+count and see live_count == 1 (only itself), then observe the
   now-disabled qemu-arm and write '0' as its prior_state. Our subsequent
   restore would read that overwritten state and never re-enable
   qemu-arm — turning a coordinated handoff into permanent disablement.
   Fix: take control.lock first, release owner UNDER control, then count
   and restore. The whole transition is now atomic.

2. The post-disable arch-test failure path released the owner lock
   manually and then called the trap handler. The handler's first
   guard ([[ -n ${_native_armhf_owner_fd:-} ]] || return 0) saw the
   already-cleared global and exited without restore — leaving qemu-arm
   disabled and the state file leaking. Fix: drop control.lock and let
   the handler do the release+count+restore under its own control lock,
   matching the normal cleanup path.

3. Stale active window after process kill (SIGKILL / OOM / kill -9
   between echo 0 and trap firing): kernel releases the fd so the
   owner lock is gone, but the state file remains. The next first-owner
   would observe the leftover-disabled qemu-arm and record disabled as
   its new prior_state — clean shutdown then never re-enables. Fix: in
   the first-owner branch, before saving prior_state, check for an
   existing state file. If found and prior was '1', reconcile qemu-arm
   back to enabled and remove the stale file before observing our own
   prior_state. Crash recovery is now self-healing on the next build.

Same Edit additionally cleans up the disable-failure case: when
'echo 0 > qemu-arm' itself fails (no CAP_SYS_ADMIN), no state actually
changed, so leave state file and owner lock alone — the cleanup handler
will observe current==prior==1 and no-op the restore. The previous code
released owner outside control and rm-ed the state file, both of which
introduced the same race the trap_handler change closes elsewhere.

Reported-by: codex (PR #9769 lock-impl review-helper)
Assisted-by: Claude:claude-opus-4.7
Signed-off-by: Igor Velkov <325961+iav@users.noreply.github.com>

Author: iav

lib/functions/rootfs/qemu-static.sh

@@ -267,10 +267,31 @@ function _native_armhf_setup_binfmt_elf() {
 	live_count="$(_native_armhf_live_owner_count_locked)"
 
 	if [[ "${live_count}" -eq 1 ]]; then
-		# We are the only live owner — first-owner. Record current qemu-arm
-		# state so cleanup can restore it state-preserving (see codex review:
-		# never blindly write 1 — if prior was disabled, leave disabled; if
-		# prior was missing, don't try to recreate the handler).
+		# We are the only live owner — first-owner. Before recording our
+		# prior_state, reconcile any stale state file from a previous
+		# active window that ended without running cleanup (e.g. SIGKILL /
+		# OOM kill / `kill -9` of the build between the post-disable echo 0
+		# and the trap firing). Without this branch the crashed window's
+		# left-behind disabled state would be observed as our new
+		# prior_state, and our own clean shutdown would never re-enable
+		# qemu-arm — turning a single crash into a permanent disablement.
+		if [[ -f "${_NATIVE_ARMHF_PRIOR_STATE}" ]]; then
+			declare stale_prior
+			stale_prior="$(cat "${_NATIVE_ARMHF_PRIOR_STATE}" 2> /dev/null || echo missing)"
+			if [[ "${stale_prior}" == "1" && -e /proc/sys/fs/binfmt_misc/qemu-arm ]]; then
+				declare crashed_observed
+				crashed_observed="$(_native_armhf_observe_qemu_arm_state)"
+				if [[ "${crashed_observed}" != "1" ]]; then
+					echo 1 > /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null || true
+					display_alert "Native armhf via binfmt_elf" "stale active window detected (previous build crashed pre-cleanup, prior=${stale_prior}); qemu-arm reconciled to enabled before our first-owner observation" "wrn"
+				fi
+			fi
+			rm -f "${_NATIVE_ARMHF_PRIOR_STATE}"
+		fi
+
+		# Record current qemu-arm state so cleanup can restore it state-
+		# preserving (codex: never blindly write 1 — if prior was disabled,
+		# leave disabled; if prior was missing, don't recreate the handler).
 		declare prior_state
 		prior_state="$(_native_armhf_observe_qemu_arm_state)"
 		echo "${prior_state}" > "${_NATIVE_ARMHF_PRIOR_STATE}"
@@ -289,9 +310,13 @@ function _native_armhf_setup_binfmt_elf() {
 			1)
 				if ! echo 0 > /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null; then
 					display_alert "Native armhf via binfmt_elf" "could not disable qemu-arm (no CAP_SYS_ADMIN?); falling back to qemu-arm-static emulation" "wrn"
-					rm -f "${_NATIVE_ARMHF_PRIOR_STATE}"
+					# qemu-arm was not changed (write failed). State file
+					# (prior=1) and our owner lock stay; the cleanup
+					# handler — already registered above — will observe
+					# current==prior==1 and no-op the restore at build
+					# exit. Don't touch them here: releasing owner outside
+					# control would race with concurrent setup probes.
 					exec {control_fd}>&-
-					_native_armhf_release_owner_lock
 					return 1
 				fi
 				;;
@@ -311,13 +336,13 @@ function _native_armhf_setup_binfmt_elf() {
 	# arm64-compat-vdso, PR #9284) — qemu-arm interprets the stub.
 	if ! arch-test armhf > /dev/null 2>&1; then
 		display_alert "Native armhf via binfmt_elf" "post-disable verification failed (host kernel lacks COMPAT_VDSO — see extensions/arm64-compat-vdso); restoring and falling back to emulation" "wrn"
-		# Restore is via cleanup handler when owner_lock releases. Mark
-		# ourselves as not-owner-of-native-mode so the cleanup invocation
-		# that runs immediately on this return path triggers restore.
+		# Drop control lock so the handler can re-acquire it. Do NOT
+		# release the owner lock here — the handler must release it UNDER
+		# control so live_count reflects only OTHER owners and the
+		# state-preserving restore decision uses a consistent snapshot.
+		# Releasing owner outside the control section was the race codex
+		# flagged in the previous round.
 		exec {control_fd}>&-
-		_native_armhf_release_owner_lock
-		# Manually invoke the same restore path so the failed setup leaves
-		# binfmt_misc as we found it; subsequent owners (if any) re-evaluate.
 		trap_handler_native_armhf_restore_qemu_arm
 		return 1
 	fi
@@ -344,22 +369,30 @@ function _native_armhf_release_owner_lock() {
 # disabled state. Restore writes the prior state observed at first-owner
 # time (1 / 0 / missing) — never blindly 1.
 function trap_handler_native_armhf_restore_qemu_arm() {
-	# Drop our own ownership first so the live-count below reflects only
-	# OTHER live owners. Skip cleanup entirely if we never acquired.
+	# Skip cleanup entirely if we never acquired an owner lock.
 	[[ -n "${_native_armhf_owner_fd:-}" ]] || return 0
-	_native_armhf_release_owner_lock
 
-	# Take the control lock to make prune+count+restore atomic with respect
-	# to any new builder racing to become an owner right now.
+	# Take the control lock BEFORE releasing our owner lock. The previous
+	# revision did the reverse and codex flagged the race: between fd
+	# close and control acquire, a fresh setup could enter, see itself as
+	# the only live owner (live_count == 1), observe the now-disabled
+	# qemu-arm and write that as its prior_state — so our subsequent
+	# restore would read the overwritten state and fail to re-enable.
+	# Holding control across release+count+restore makes the transition
+	# atomic with respect to any concurrent setup.
 	declare control_fd
 	exec {control_fd}> "${_NATIVE_ARMHF_CONTROL_LOCK}"
-	flock "${control_fd}" || {
-		# Lock failed — best effort cleanup without restore. Worst case is
-		# qemu-arm stays in whatever state it is now; next build will
-		# reconcile.
+	if ! flock "${control_fd}"; then
+		# Lock failed — best effort. Drop our owner so we are not counted
+		# live forever; next build will reconcile.
 		exec {control_fd}>&-
+		_native_armhf_release_owner_lock
 		return 0
-	}
+	fi
+
+	# Drop our own ownership UNDER control. live_count below now reflects
+	# only OTHER live owners.
+	_native_armhf_release_owner_lock
 
 	declare live_count
 	live_count="$(_native_armhf_live_owner_count_locked)"