From 9f3c3eae448a55d3eb6b53348c8a6fb6fb8b38bb Mon Sep 17 00:00:00 2001 From: Shane Wolf Date: Thu, 20 Aug 2026 19:52:57 -0700 Subject: [PATCH 1/4] fix(bin): keep dash-prefixed paths out of option parsing in the Stop chain A login shell carries its own executable path as argv[0] with a leading dash (macOS login produces -/bin/zsh), and that string reaches the harness ancestry walk as an ordinary argument. BSD basename parses a leading dash as an option bundle and refuses it, so an unguarded call printed "basename: illegal option -- /" plus its usage ahead of every Stop-hook message and every session-lock acquisition the operator saw. The ancestry walk itself has taken -- since 6ec5e08, which is why the symptom no longer reproduces; the remaining path-tool calls in the same chain carried the same latent defect. Give basename, dirname, readlink and cd their -- separator throughout the turn-end guard, the Stop-owned auto-arm, the arm wrapper, the watcher, the session lock, and the shared lock helpers, so no path can be re-read as a flag. Pin the behavior with two regressions that run the real hook and the real lock acquisition beneath a live process whose argv[0] is a dash-prefixed path, assert no path-tool diagnostic reaches the operator, and assert the wake still delivers. Both fail with the original defect restored, reproducing the exact reported message. --- bin/fm-claude-stop-autoarm.sh | 2 +- bin/fm-lock.sh | 2 +- bin/fm-turnend-guard.sh | 4 +- bin/fm-wake-lib.sh | 23 ++++--- bin/fm-watch-arm.sh | 4 +- bin/fm-watch.sh | 8 +-- tests/fm-claude-stop-autoarm.test.sh | 94 ++++++++++++++++++++++++++++ 7 files changed, 117 insertions(+), 20 deletions(-) diff --git a/bin/fm-claude-stop-autoarm.sh b/bin/fm-claude-stop-autoarm.sh index 89ce011f6b..b8fc4b1e13 100755 --- a/bin/fm-claude-stop-autoarm.sh +++ b/bin/fm-claude-stop-autoarm.sh @@ -57,7 +57,7 @@ # the model. set -u -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" diff --git a/bin/fm-lock.sh b/bin/fm-lock.sh index 52d7c8aee4..b077ae8ded 100755 --- a/bin/fm-lock.sh +++ b/bin/fm-lock.sh @@ -7,7 +7,7 @@ # fm-lock.sh status print holder and liveness; always exits 0 set -u -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" diff --git a/bin/fm-turnend-guard.sh b/bin/fm-turnend-guard.sh index 43e7045706..bba6b79ae1 100755 --- a/bin/fm-turnend-guard.sh +++ b/bin/fm-turnend-guard.sh @@ -66,7 +66,7 @@ # fail-open only for an already verified failure episode. set -u -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" @@ -86,7 +86,7 @@ for arg in "$@"; do case "$arg" in --claude) CLAUDE_MODE=1 ;; --cursor) CURSOR_MODE=1 ;; - *) echo "usage: $(basename "$0") [--claude|--cursor]" >&2; exit 2 ;; + *) echo "usage: $(basename -- "$0") [--claude|--cursor]" >&2; exit 2 ;; esac done diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 28249b661f..5ff364d147 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Shared durable wake queue and portable lock helpers. -FM_WAKE_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +FM_WAKE_LIB_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" FM_WAKE_DEFAULT_ROOT="$(cd "$FM_WAKE_LIB_DIR/.." && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-${FM_ROOT:-$FM_WAKE_DEFAULT_ROOT}}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" @@ -314,11 +314,14 @@ fm_lock_role() { cat "$1/role" 2>/dev/null } +# Every path tool below takes `--`, and every `cd` takes it too: a path whose +# first character is a dash is otherwise parsed as an option bundle, which BSD +# basename rejects loudly enough to prefix an operator-visible message. fm_lock_abs_path() { local path=$1 dir base - dir=$(dirname "$path") - base=$(basename "$path") - dir=$(cd "$dir" 2>/dev/null && pwd -P) || return 1 + dir=$(dirname -- "$path") + base=$(basename -- "$path") + dir=$(cd -- "$dir" 2>/dev/null && pwd -P) || return 1 printf '%s/%s\n' "$dir" "$base" } @@ -338,17 +341,17 @@ fm_lock_prepare_owner() { fm_lock_link_owner() { local lockdir=$1 owner - owner=$(readlink "$lockdir" 2>/dev/null) || return 1 + owner=$(readlink -- "$lockdir" 2>/dev/null) || return 1 [ -n "$owner" ] || return 1 case "$owner" in /*) printf '%s\n' "$owner" ;; - *) printf '%s/%s\n' "$(dirname "$lockdir")" "$owner" ;; + *) printf '%s/%s\n' "$(dirname -- "$lockdir")" "$owner" ;; esac } fm_lock_points_to_owner() { local lockdir=$1 ownerdir=$2 actual - actual=$(readlink "$lockdir" 2>/dev/null) || return 1 + actual=$(readlink -- "$lockdir" 2>/dev/null) || return 1 [ "$actual" = "$ownerdir" ] } @@ -361,8 +364,8 @@ fm_lock_discard_owner() { fm_lock_remove_stray_owner_link() { local lockdir=$1 ownerdir=$2 stray - stray="$lockdir/$(basename "$ownerdir")" - if [ -L "$stray" ] && [ "$(readlink "$stray" 2>/dev/null || true)" = "$ownerdir" ]; then + stray="$lockdir/$(basename -- "$ownerdir")" + if [ -L "$stray" ] && [ "$(readlink -- "$stray" 2>/dev/null || true)" = "$ownerdir" ]; then rm -f "$stray" 2>/dev/null || true fi } @@ -1228,7 +1231,7 @@ fm_wake_signal_sig() { # -> "size:mtime" } fm_wake_signal_seen_path() { # - printf '%s/.seen-%s' "$1" "$(basename "$2" | tr '.' '_')" + printf '%s/.seen-%s' "$1" "$(basename -- "$2" | tr '.' '_')" } # 0 when 's current signature exactly matches its recorded seen marker, diff --git a/bin/fm-watch-arm.sh b/bin/fm-watch-arm.sh index d134f51940..cabe33b24b 100755 --- a/bin/fm-watch-arm.sh +++ b/bin/fm-watch-arm.sh @@ -60,7 +60,7 @@ # (secondmate homes run the same script) and would kill siblings. set -u -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=bin/fm-wake-lib.sh . "$SCRIPT_DIR/fm-wake-lib.sh" @@ -397,7 +397,7 @@ case "${1:-}" in case "$handling_watcher_pid" in ''|*[!0-9]*) echo "watcher: invalid successor watcher pid" >&2; exit 2 ;; esac [ "$#" -eq 4 ] || { echo "watcher: unexpected handling delivery arguments" >&2; exit 2; } ;; - *) echo "usage: $(basename "$0") [--restart | --handling-delivered GENERATION --watcher-pid PID]" >&2; exit 2 ;; + *) echo "usage: $(basename -- "$0") [--restart | --handling-delivered GENERATION --watcher-pid PID]" >&2; exit 2 ;; esac if [ "$mode" = handling-delivered ]; then diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index d1d59d3ceb..b9dd505b0b 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -73,7 +73,7 @@ # no-op through the watcher singleton lock. set -u -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" @@ -1011,7 +1011,7 @@ while :; do for c in "$STATE"/*.check.sh; do [ -e "$c" ] || continue is_pr_poll=0 - if [ "$(basename "$c")" = x-watch.check.sh ]; then + if [ "$(basename -- "$c")" = x-watch.check.sh ]; then if fmx_poll_shim_valid "$c" "$FM_HOME" "$FM_ROOT" \ && [ -f "$FM_ROOT/bin/fm-x-poll.sh" ] && [ ! -L "$FM_ROOT/bin/fm-x-poll.sh" ]; then FM_HOME="$FM_HOME" run_check_capture "$FM_ROOT/bin/fm-x-poll.sh" || exit 1 @@ -1021,7 +1021,7 @@ while :; do continue fi else - id=$(basename "$c" .check.sh) + id=$(basename -- "$c" .check.sh) if fm_pr_poll_snapshot_capture "$STATE" "$id" "$SCRIPT_DIR/fm-pr-poll.sh"; then is_pr_poll=1 provider=$FM_PR_POLL_SNAPSHOT_PROVIDER @@ -1101,7 +1101,7 @@ EOF if afk_present || signal_reason_is_actionable $files || ! signal_crew_provably_working $files; then while IFS=$(printf '\t') read -r sf sig f; do [ -n "$sf" ] || continue - fm_wake_append signal "$(basename "$f")" "$reason" || exit 1 + fm_wake_append signal "$(basename -- "$f")" "$reason" || exit 1 done < "$shim" <<'SH' +#!/usr/bin/env bash +set -u +if [ "${FM_LOGIN_SHELL_STAGE:-0}" = 0 ]; then + FM_LOGIN_SHELL_STAGE=1 exec -a "-$BASH" "$BASH" "$0" "$@" +fi +# Record what this platform's ps actually exposes, so the test can prove the +# dash-prefixed path really reached the walk instead of passing vacuously. +LC_ALL=C ps -o comm= -p $$ > "$FM_HOME/state/ancestor-comm" 2>/dev/null +LC_ALL=C ps -o args= -p $$ > "$FM_HOME/state/ancestor-args" 2>/dev/null +# No exec below: this process must SURVIVE as the dash-named ancestor, and the +# harness must be its own pid, or the whole chain collapses into a single +# process and the ancestry walk never sees a login shell at all. +# The trailing `exit` on each level defeats bash's last-command exec +# optimization, which would otherwise replace this shell (and the harness +# shell) in place and collapse the whole chain into one pid with no login +# shell left to walk past. +"$FAKE_CLAUDE" -c ' + printf "%s\n" "$$" > "$FM_HOME/state/.lock" + "$@" + exit $? +' fm-login-shell-shim "$@" +exit $? +SH + chmod +x "$shim" +} + +# 0 when either ps field recorded by the shim carries the dash-prefixed path. +# macOS ps reports argv[0] in comm=; procps reports the kernel exec name there +# and exposes argv[0] only through args=, so either field satisfies this. +assert_login_shell_argv0_reached_walk() { + local dir=$1 comm args + comm=$(cat "$dir/state/ancestor-comm" 2>/dev/null || true) + args=$(cat "$dir/state/ancestor-args" 2>/dev/null || true) + case "$comm$args" in + *-/*) return 0 ;; + esac + fail "no ps field carried the login-shell argv[0]; comm='$comm' args='$args'" +} + +assert_no_path_tool_diagnostic() { + local out=$1 what=$2 + case "$out" in + *"illegal option"*|*"basename:"*|*"dirname:"*|*"usage: basename"*|*"usage: dirname"*) + fail "$what printed a path-tool diagnostic: $out" + ;; + esac +} + +test_rewake_is_clean_under_login_shell_ancestor() { + local dir shim out status + dir=$(make_primary_dir "$TMP_ROOT/login-shell-rewake") + : > "$dir/state/task.meta" + write_arm_fixture "$dir" actionable + shim="$TMP_ROOT/login-shell-rewake-shim.sh" + write_login_shell_shim "$shim" + status=0 + out=$(printf '%s\n' '{"session_id":"sess-login-shell","stop_hook_active":false}' \ + | FM_HOME="$dir" "$shim" "$dir/bin/fm-claude-stop-autoarm.sh" 2>&1) || status=$? + assert_login_shell_argv0_reached_walk "$dir" + assert_no_path_tool_diagnostic "$out" "the Stop-owned auto-arm" + expect_code 2 "$status" "an actionable close must still rewake beneath a login-shell ancestor" + assert_contains "$out" "firstmate watcher wake" "the rewake banner must still be delivered" + pass "auto-arm: rewake beneath a login-shell ancestor carries no path-tool diagnostic" +} + +test_lock_acquire_is_clean_under_login_shell_ancestor() { + local dir shim out status + dir=$(make_primary_dir "$TMP_ROOT/login-shell-lock") + shim="$TMP_ROOT/login-shell-lock-shim.sh" + write_login_shell_shim "$shim" + status=0 + out=$(FM_HOME="$dir" "$shim" "$ROOT/bin/fm-lock.sh" 2>&1) || status=$? + assert_login_shell_argv0_reached_walk "$dir" + assert_no_path_tool_diagnostic "$out" "session-lock acquisition" + expect_code 0 "$status" "lock acquisition must succeed beneath a login-shell ancestor" + assert_contains "$out" "lock acquired: harness pid" "the lock must still report its harness pid" + pass "fm-lock: acquisition beneath a login-shell ancestor carries no path-tool diagnostic" +} + test_inert_in_child_worktree test_inert_without_session_lock test_reclaims_stale_session_lock_before_arming @@ -815,3 +907,5 @@ test_need_vanished_mid_cycle_closes_quietly test_afk_mid_cycle_suppresses_rewake test_active_in_marked_secondmate_home test_fm_lock_status_still_works_with_shared_lib +test_rewake_is_clean_under_login_shell_ancestor +test_lock_acquire_is_clean_under_login_shell_ancestor From 13235a0d67a55bf57b808e3832f3149f01440f24 Mon Sep 17 00:00:00 2001 From: Shane Wolf Date: Thu, 20 Aug 2026 20:00:19 -0700 Subject: [PATCH 2/4] no-mistakes(review): Harden Stop-chain root cd calls --- bin/fm-claude-stop-autoarm.sh | 2 +- bin/fm-lock.sh | 2 +- bin/fm-turnend-guard.sh | 2 +- bin/fm-wake-lib.sh | 2 +- bin/fm-watch.sh | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bin/fm-claude-stop-autoarm.sh b/bin/fm-claude-stop-autoarm.sh index b8fc4b1e13..06c1f4f155 100755 --- a/bin/fm-claude-stop-autoarm.sh +++ b/bin/fm-claude-stop-autoarm.sh @@ -58,7 +58,7 @@ set -u SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" -FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" +FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd -- "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" CONFIG="${FM_CONFIG_OVERRIDE:-$FM_HOME/config}" diff --git a/bin/fm-lock.sh b/bin/fm-lock.sh index b077ae8ded..7c9c9e7255 100755 --- a/bin/fm-lock.sh +++ b/bin/fm-lock.sh @@ -8,7 +8,7 @@ set -u SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" -FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" +FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd -- "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" LOCK="$STATE/.lock" diff --git a/bin/fm-turnend-guard.sh b/bin/fm-turnend-guard.sh index bba6b79ae1..47796f7740 100755 --- a/bin/fm-turnend-guard.sh +++ b/bin/fm-turnend-guard.sh @@ -67,7 +67,7 @@ set -u SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" -FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" +FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd -- "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" CONFIG="${FM_CONFIG_OVERRIDE:-$FM_HOME/config}" diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 5ff364d147..bcb9f2305f 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -2,7 +2,7 @@ # Shared durable wake queue and portable lock helpers. FM_WAKE_LIB_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" -FM_WAKE_DEFAULT_ROOT="$(cd "$FM_WAKE_LIB_DIR/.." && pwd)" +FM_WAKE_DEFAULT_ROOT="$(cd -- "$FM_WAKE_LIB_DIR/.." && pwd)" FM_ROOT="${FM_ROOT_OVERRIDE:-${FM_ROOT:-$FM_WAKE_DEFAULT_ROOT}}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-${STATE:-$FM_HOME/state}}" diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index b9dd505b0b..4560ab890f 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -74,7 +74,7 @@ set -u SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" -FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" +FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd -- "$SCRIPT_DIR/.." && pwd)}" FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" mkdir -p "$STATE" From 27f6e46715d991cf8f02b1a322d76d647040fc99 Mon Sep 17 00:00:00 2001 From: Shane Wolf Date: Fri, 21 Aug 2026 14:30:38 -0700 Subject: [PATCH 3/4] test(bin): pin the dash-prefixed lock-path readlink guard The Stop-chain hardening gave fm_lock_link_owner and fm_lock_points_to_owner a -- separator, but the regressions that shipped with it exercise only the basename ancestry walk, so both readlink guards were unpinned. Cover them through the lock helpers' own interface: a lock link whose path starts with a dash must resolve to its owner directory and compare equal. Removing -- from either helper fails the test, reporting the link as unreadable or as a mismatch respectively. Measured on macOS 26.4.1 (build 25E253): readlink "-weird" fails with "readlink: illegal option -- w" while readlink -- "-weird" prints the target, so -- is what makes a dash-prefixed link readable on BSD readlink. --- tests/fm-watcher-lock.test.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index a3628b1694..3f229d6d18 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -220,6 +220,32 @@ test_lock_single_winner_under_concurrency() { pass "concurrent fm_lock_try_acquire yields exactly one winner" } +# BSD readlink parses a leading dash as an option bundle, so a lock path whose +# first character is a dash is unreadable unless the call passes `--` first. +# Measured on macOS 26.4.1 (build 25E253): `readlink "-weird"` fails with +# `readlink: illegal option -- w` while `readlink -- "-weird"` prints the target. +# The lock helpers resolve their owner link through readlink, so this pins the +# guard that keeps a dash-prefixed lock path readable rather than silently +# unresolvable. Drop the `--` from fm_lock_link_owner or fm_lock_points_to_owner +# and this fails, because the helper reports the link as unreadable. +test_lock_helpers_resolve_dash_prefixed_lock_path() { + local dir state out + dir=$(make_case lock-dash-link) + state="$dir/state" + out=$(cd "$state" && FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + mkdir -p -- owner.d + ln -s -- "$PWD/owner.d" "-dash.lock" + resolved=$(fm_lock_link_owner "-dash.lock") || { printf "unreadable\n"; exit 1; } + [ "$resolved" = "$PWD/owner.d" ] || { printf "wrong-owner:%s\n" "$resolved"; exit 1; } + fm_lock_points_to_owner "-dash.lock" "$PWD/owner.d" || { printf "mismatch\n"; exit 1; } + printf "ok\n" + ' _ "$LIB" 2>&1) + [ "$out" = ok ] \ + || fail "a dash-prefixed lock link must resolve through the lock helpers, got: $out" + pass "lock helpers resolve a dash-prefixed lock path through readlink" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1107,6 +1133,7 @@ test_stale_watch_reclaim_publishes_before_clear test_live_stale_watch_lock_is_actionable test_guard_warnings test_lock_single_winner_under_concurrency +test_lock_helpers_resolve_dash_prefixed_lock_path test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 9be08967b0887d20424d65eed094b70622f1e792 Mon Sep 17 00:00:00 2001 From: Shane Wolf Date: Fri, 21 Aug 2026 15:45:21 -0700 Subject: [PATCH 4/4] revert(bin): drop the readlink separators from the lock helpers The maintainer's second review moved the objection from portability to necessity, and that argument holds. This branch exists to stop a dash-prefixed login-shell argv[0] from polluting Stop-hook messages, and the lock helpers were never on that path: every lock path is built under the home's state directory from FM_HOME and is absolute, so none can arrive dash-prefixed. Guarding readlink there was hardening beyond the defect, paid for by depending on -- support in one more BSD tool. Restore fm_lock_link_owner, fm_lock_points_to_owner and fm_lock_remove_stray_owner_link to plain readlink, matching main. Every basename, dirname and cd separator stays: those are the reported bug. Re-aim the lock regression accordingly. Pinning a separator that is no longer there would be wrong, and deleting the test outright would leave the owner-link helpers with no direct coverage at all, so it now pins what is in contract: an acquired absolute lock path resolves to its owner directory, agrees only with that directory, and stops resolving once released. The comment records why the dash-prefixed case is deliberately not covered. --- bin/fm-wake-lib.sh | 16 +++++++---- tests/fm-watcher-lock.test.sh | 52 +++++++++++++++++++++-------------- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index bcb9f2305f..aff31a1663 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -314,9 +314,13 @@ fm_lock_role() { cat "$1/role" 2>/dev/null } -# Every path tool below takes `--`, and every `cd` takes it too: a path whose -# first character is a dash is otherwise parsed as an option bundle, which BSD -# basename rejects loudly enough to prefix an operator-visible message. +# basename, dirname and cd take `--` here because they are reached from the +# ancestry walk, where a login shell's own argv[0] arrives dash-prefixed and BSD +# basename would otherwise reject it as an option bundle. The readlink calls +# below deliberately do NOT: every lock path is built under the home's state +# directory from FM_HOME and is absolute, so none can begin with a dash, and +# adding a separator there would be hardening beyond the defect at the cost of +# depending on `--` support in one more BSD tool. fm_lock_abs_path() { local path=$1 dir base dir=$(dirname -- "$path") @@ -341,7 +345,7 @@ fm_lock_prepare_owner() { fm_lock_link_owner() { local lockdir=$1 owner - owner=$(readlink -- "$lockdir" 2>/dev/null) || return 1 + owner=$(readlink "$lockdir" 2>/dev/null) || return 1 [ -n "$owner" ] || return 1 case "$owner" in /*) printf '%s\n' "$owner" ;; @@ -351,7 +355,7 @@ fm_lock_link_owner() { fm_lock_points_to_owner() { local lockdir=$1 ownerdir=$2 actual - actual=$(readlink -- "$lockdir" 2>/dev/null) || return 1 + actual=$(readlink "$lockdir" 2>/dev/null) || return 1 [ "$actual" = "$ownerdir" ] } @@ -365,7 +369,7 @@ fm_lock_discard_owner() { fm_lock_remove_stray_owner_link() { local lockdir=$1 ownerdir=$2 stray stray="$lockdir/$(basename -- "$ownerdir")" - if [ -L "$stray" ] && [ "$(readlink -- "$stray" 2>/dev/null || true)" = "$ownerdir" ]; then + if [ -L "$stray" ] && [ "$(readlink "$stray" 2>/dev/null || true)" = "$ownerdir" ]; then rm -f "$stray" 2>/dev/null || true fi } diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 3f229d6d18..4d92618de8 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -220,30 +220,42 @@ test_lock_single_winner_under_concurrency() { pass "concurrent fm_lock_try_acquire yields exactly one winner" } -# BSD readlink parses a leading dash as an option bundle, so a lock path whose -# first character is a dash is unreadable unless the call passes `--` first. -# Measured on macOS 26.4.1 (build 25E253): `readlink "-weird"` fails with -# `readlink: illegal option -- w` while `readlink -- "-weird"` prints the target. -# The lock helpers resolve their owner link through readlink, so this pins the -# guard that keeps a dash-prefixed lock path readable rather than silently -# unresolvable. Drop the `--` from fm_lock_link_owner or fm_lock_points_to_owner -# and this fails, because the helper reports the link as unreadable. -test_lock_helpers_resolve_dash_prefixed_lock_path() { +# The owner-link helpers resolve a lock symlink through readlink, which is +# deliberately called WITHOUT a `--` separator: every lock path is built under +# the home's state directory from FM_HOME and is absolute, so a dash-prefixed +# lock path is outside the contract these helpers accept and is not pinned here. +# What is in contract is the ordinary absolute owner link, which nothing else +# exercises directly: an acquired lock must report the owner directory it points +# at, agree only with that directory, and stop resolving once released. +test_lock_helpers_resolve_owner_link_for_absolute_lock_path() { local dir state out - dir=$(make_case lock-dash-link) + dir=$(make_case lock-owner-link) state="$dir/state" - out=$(cd "$state" && FM_STATE_OVERRIDE="$state" bash -c ' + out=$(FM_STATE_OVERRIDE="$state" bash -c ' . "$1" - mkdir -p -- owner.d - ln -s -- "$PWD/owner.d" "-dash.lock" - resolved=$(fm_lock_link_owner "-dash.lock") || { printf "unreadable\n"; exit 1; } - [ "$resolved" = "$PWD/owner.d" ] || { printf "wrong-owner:%s\n" "$resolved"; exit 1; } - fm_lock_points_to_owner "-dash.lock" "$PWD/owner.d" || { printf "mismatch\n"; exit 1; } + lockdir="$2/.owner-link.lock" + fm_lock_try_acquire "$lockdir" || { printf "acquire-failed\n"; exit 1; } + owner=$(fm_lock_link_owner "$lockdir") || { printf "unreadable\n"; exit 1; } + case "$owner" in + /*) ;; + *) printf "not-absolute:%s\n" "$owner"; exit 1 ;; + esac + [ -d "$owner" ] || { printf "owner-missing:%s\n" "$owner"; exit 1; } + fm_lock_points_to_owner "$lockdir" "$owner" || { printf "mismatch\n"; exit 1; } + if fm_lock_points_to_owner "$lockdir" "$owner.other"; then + printf "false-match\n" + exit 1 + fi + fm_lock_release "$lockdir" + if fm_lock_link_owner "$lockdir" >/dev/null 2>&1; then + printf "still-linked\n" + exit 1 + fi printf "ok\n" - ' _ "$LIB" 2>&1) + ' _ "$LIB" "$state" 2>&1) [ "$out" = ok ] \ - || fail "a dash-prefixed lock link must resolve through the lock helpers, got: $out" - pass "lock helpers resolve a dash-prefixed lock path through readlink" + || fail "an absolute lock path must resolve, match only its own owner, and clear on release, got: $out" + pass "lock helpers resolve and clear an absolute lock path's owner link" } test_lock_steals_dead_pid_lock() { @@ -1133,7 +1145,7 @@ test_stale_watch_reclaim_publishes_before_clear test_live_stale_watch_lock_is_actionable test_guard_warnings test_lock_single_winner_under_concurrency -test_lock_helpers_resolve_dash_prefixed_lock_path +test_lock_helpers_resolve_owner_link_for_absolute_lock_path test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed