Skip to content

Commit 42770de

Browse files
committed
qemu-static: document cleanup-ordering invariant for LOCK_SH inheritance
Codex reviewer flagged a real fragility in the simplified design: BSD flock is per-OFD, so a forked subshell inheriting our LOCK_SH fd shares the same lock entry. Closing the parent's fd does not release the lock while any inheriting child remains alive. If our cleanup handler ran before the build's other subshell-killing handlers, the LOCK_EX-LOCK_NB last-out probe could falsely block on our own stale child and skip the restore — leaving qemu-arm globally disabled. In practice this does not manifest because Armbian's add_cleanup_handler runs registrations in order, and the umount/SDCARD/MOUNT teardown handlers (registered earlier) execute mount(8)/umount(8) subprocesses that finish before returning. By the time our handler runs, the docker container has been killed and its mount-bound children with it. SIGINT-during-image-late-apt-install was empirically verified to clean up correctly: last-out fires, qemu-arm restored, no stale state. Document the dependency so a future refactor of cleanup ordering does not silently break the invariant. Note POSIX F_SETLK and explicit descendant-kill as alternative fixes if the ordering becomes hard to guarantee, with their respective footguns called out (POSIX releases all locks on the file when the process closes any fd to it, including shell builtin redirects like `echo 0 > file`). Assisted-by: Claude:claude-opus-4.7
1 parent fe2733b commit 42770de

1 file changed

Lines changed: 31 additions & 0 deletions

File tree

lib/functions/rootfs/qemu-static.sh

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,37 @@ function _native_armhf_setup_binfmt_elf() {
290290
# builder to leave (detected via LOCK_EX-LOCK_NB succeeding on a fresh fd —
291291
# meaning zero other SH holders) re-enables qemu-arm. Earlier-leavers do
292292
# nothing; their job ends when the kernel releases their LOCK_SH on fd close.
293+
#
294+
# Cleanup ordering invariant
295+
# This handler MUST run AFTER any cleanup that kills off subshells / other
296+
# children of THIS build. BSD flock (the syscall behind `flock(1)`) is
297+
# per-OFD, so a fork()ed subshell that inherits our `_native_armhf_lock_fd`
298+
# shares the SAME LOCK_SH lock entry — closing the parent's fd is not
299+
# enough to release the lock while any inheriting child is still alive.
300+
#
301+
# The `LOCK_EX-LOCK_NB` probe below would then falsely block on our own
302+
# stale child holding the SH side of the lock, and we would skip the
303+
# restore — leaving qemu-arm globally disabled with no live builder.
304+
#
305+
# In current Armbian compile.sh, `add_cleanup_handler` registrations run
306+
# in registration order during error/SIGINT cleanup. We register here
307+
# AFTER the umount/SDCARD/MOUNT teardown handlers (which exec mount(8) /
308+
# umount(8) subprocesses that finish before returning), so by the time we
309+
# run the docker container has been killed, its mount-bound children are
310+
# gone, and our inherited-fd children with them. Empirically verified by
311+
# SIGINT during image-late `apt install` chroot work — last-out fires
312+
# correctly.
313+
#
314+
# If a future refactor changes that ordering (e.g. moves the umount
315+
# handler to run AFTER us, or adds a long-lived child between this
316+
# handler and the death of the build's subshell tree), this invariant
317+
# breaks. The most robust fix in that case is to either (a) replace BSD
318+
# flock with POSIX `fcntl(F_SETLK)` via a python helper (per-process,
319+
# not per-OFD, so fork()'d children do not share the lock) or (b)
320+
# explicitly kill our own descendants before the LOCK_EX-LOCK_NB probe.
321+
# POSIX has its own footgun (closing ANY fd to the file by this process
322+
# releases all our locks on it, including via shell builtin redirects
323+
# like `echo 0 > file`), so neither path is free.
293324
function trap_handler_native_armhf_restore_qemu_arm() {
294325
[[ -n "${_native_armhf_lock_fd:-}" ]] || return 0
295326

0 commit comments

Comments
 (0)