From d72f80aa930cce661b14bca8b93a98b847cac5c5 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Sat, 7 Nov 2026 15:53:42 +0100 Subject: [PATCH 1/3] Fix #1124: close the pre-steal-inode race, the inode-reuse/non-preserving-rename gap, and the hand-back clobber in _lock_try_steal A code review of merged #1123 found three more gaps in the same function's stale-lock takeover: the "before" inode snapshot was captured after both the first pid read and the age check, leaving a window for another contender's full steal cycle to complete and be mistaken for the unchanged original placeholder; _lock_file_inode assumes rename preserves identity, which is false on FAT/exFAT under Cygwin/MSYS, SMB with noserverino, and FUSE without use_ino, where the identity check would hand back forever; and every hand-back mv could silently overwrite a pid another process had already written. The snapshot now runs before the first read; a one-time per-steal self-test (_lock_rename_preserves_identity) skips the identity comparison on a filesystem where it cannot answer meaningfully, falling back to the content re-read alone rather than refusing forever; and hand-back now reads the claim's own content back and writes it with noclobber, the same atomicity the real steal writes already use. Co-Authored-By: Max --- changelog.d/1124.fixed.md | 18 ++++ scripts/lib-lock.sh | 120 ++++++++++++++++++++++---- tests/test_lock_primitive.py | 162 +++++++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 17 deletions(-) create mode 100644 changelog.d/1124.fixed.md diff --git a/changelog.d/1124.fixed.md b/changelog.d/1124.fixed.md new file mode 100644 index 00000000..f54efdd0 --- /dev/null +++ b/changelog.d/1124.fixed.md @@ -0,0 +1,18 @@ +- Fixed: three follow-on gaps in `_lock_try_steal` (`scripts/lib-lock.sh`) + after #1122/#1123 (#1124). (1) The "before" inode snapshot was taken after + BOTH the first pid read and the age check had already run -- a window wide + enough for another contender to complete its own entire steal-and-rewrite + cycle inside it, after which the snapshot answered for the WINNER's fresh + pid file rather than for the original placeholder being judged, letting the + #1122 check see a spurious match and clobber a live lock. The snapshot now + runs before the first read even starts. (2) `_lock_file_inode` assumes + rename preserves a file's identity; not true on FAT/exFAT under + Cygwin/MSYS, SMB with noserverino, or FUSE without use_ino (#1103's own + setup), where the identity check could never match for the same file and + would hand back forever. A one-time self-test (`_lock_rename_preserves_identity`) + now detects this and skips the comparison rather than trust metadata never + confirmed meaningful on that filesystem. (3) Every hand-back `mv` of a + claim file back into place could silently overwrite a pid another process + had already written in the meantime; hand-back now reads the claim's own + content back and writes it with `noclobber`, the same atomicity the real + steal writes already use. diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index 9fa56d11..a4baa4dd 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -446,6 +446,63 @@ _lock_read_pid_timeout() { return 0 } +# Hand a claim file back to the lock's pid path without clobbering a pid +# some OTHER process already wrote there in the meantime (#1124). Reads +# the claim's own content back and writes it with `noclobber`, giving the +# restore the same atomicity the real steal writes already have below. +# Falls back to a plain `mv` only if the claim's content cannot even be +# read back. +_lock_handback() { + local _claim="$1" _dir="$2" _content + if _lock_read_pid_timeout "$_claim"; then + _content="$_LOCK_PID_READ_RESULT" + if ( set -o noclobber; echo "$_content" > "${_dir}/pid" ) 2>/dev/null; then + rm -f "$_claim" 2>/dev/null || true + return 0 + fi + rm -f "$_claim" 2>/dev/null || true + return 1 + fi + mv "$_claim" "${_dir}/pid" 2>/dev/null || true + return 1 +} + +# Whether renaming a file within preserves its identity (the value +# `_lock_file_inode` reports) across the move. True on every POSIX +# filesystem this file otherwise assumes; false on FAT/exFAT under +# Cygwin/MSYS (the "inode" there is a hash of the path, which rename +# changes by definition), SMB mounted with noserverino, and FUSE mounts +# without use_ino -- #1103's own dataless-placeholder setup is exactly +# this class (#1124). On any of those the #1122 identity check below would +# compare two values that can never match for the SAME file, and hand +# back on every steal attempt forever -- reopening #1103's own outage +# with the very check meant to prevent a different bug. +# +# Probed with a throwaway file IN itself so the answer is for the +# exact filesystem the steal is about to run on. Only reached from inside +# a steal attempt, already paying for a multi-second read timeout above +# it, and never on the uncontended acquire path, which never calls +# _lock_try_steal at all. +# +# A probe that itself fails to create or rename answers false -- the safe +# default: it disables a check this contender cannot even confirm works, +# rather than trusting metadata that was never verified meaningful here. +_lock_rename_preserves_identity() { + local _dir="$1" _probe _moved _i1 _i2 + _probe="${_dir}/.steal-probe.${_LOCK_SELF}" + _moved="${_probe}.moved" + rm -f "$_probe" "$_moved" 2>/dev/null || true + ( umask 077; : > "$_probe" ) 2>/dev/null || return 1 + _i1=$(_lock_file_inode "$_probe") + if ! mv "$_probe" "$_moved" 2>/dev/null; then + rm -f "$_probe" 2>/dev/null || true + return 1 + fi + _i2=$(_lock_file_inode "$_moved") + rm -f "$_moved" 2>/dev/null || true + [ -n "$_i1" ] && [ -n "$_i2" ] && [ "$_i1" = "$_i2" ] +} + # Take a stale lock away from a dead holder. Returns 0 only for the single # process that won the rename — everyone else gets 1 and keeps waiting. _lock_try_steal() { @@ -481,6 +538,23 @@ _lock_try_steal() { done fi + # Captured BEFORE the read below, not after it as an earlier version of + # this fix did -- #1124, a follow-on finding against #1122. The + # previous "before" snapshot ran only after the read below had ALREADY + # timed out and the age check further down had ALSO already run, which + # is a window wide enough (the read's own timeout is whole seconds; a + # rename-plus-rewrite elsewhere is microseconds) for some other + # contender to complete an entire steal-and-rewrite cycle inside it -- + # so the old snapshot could already be of THAT contender's brand-new + # pid file, not of the stuck placeholder the read below is about to + # judge. The #1122 comparison later on would then see "unchanged" for + # a file that was, in fact, swapped out from under this contender + # mid-function. `stat` is metadata-only and answers unconditionally, + # dataless placeholder or not, so capturing it this early costs + # nothing extra -- including on the fast, non-stealing path below, + # which simply never uses the value. + _pre_steal_inode=$(_lock_file_inode "${_dir}/pid") + if _lock_read_pid_timeout "${_dir}/pid"; then _pid="$_LOCK_PID_READ_RESULT" else @@ -530,12 +604,10 @@ _lock_try_steal() { # path again -- is what actually closes that gap: see the # comment right after the `mv`. Removing it reopens the race. [ "$(_lock_dir_age "$_dir")" -ge "$_LOCK_MAX_HOLD_SECONDS" ] || return 1 - # Captured BEFORE the rename, not derived from it: `stat` reads - # metadata only, so this still answers even though the content read - # above just failed. This is the "before" half of the #1122 check - # below -- what we are ABOUT to grab, independent of whether we can - # read what is inside it. - _pre_steal_inode=$(_lock_file_inode "${_dir}/pid") + # _pre_steal_inode was already captured above, before the read at + # the top of this function even started -- see the comment there + # for why (#1124). Re-deriving it here, after the age check, is + # exactly the bug that comment describes. mv "${_dir}/pid" "$_claim" 2>/dev/null || return 1 # Confirmed by direct repro (N=8 concurrent contenders) that # skipping this check lets 7 of 8 believe they won: this winner's @@ -552,7 +624,7 @@ _lock_try_steal() { # contender's signal that it grabbed someone else's win by # mistake and must hand it back rather than overwrite it. if _lock_read_pid_timeout "$_claim"; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true + _lock_handback "$_claim" "$_dir" return 1 fi # #1122: the re-read above failing is NOT by itself proof that @@ -580,10 +652,22 @@ _lock_try_steal() { # or either inode lookup itself coming back empty, is exactly the # ambiguity #1119/#1120 already decided must refuse rather than # guess: hand back. - _claim_inode=$(_lock_file_inode "$_claim") - if [ -z "$_pre_steal_inode" ] || [ -z "$_claim_inode" ] || [ "$_pre_steal_inode" != "$_claim_inode" ]; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true - return 1 + # On a filesystem where rename does not preserve identity (#1124: + # FAT/exFAT under Cygwin/MSYS, SMB with noserverino, FUSE without + # use_ino), $_pre_steal_inode and $_claim_inode could never match + # even for the exact same file -- comparing them anyway would hand + # back on every single steal attempt here, forever, reopening + # #1103's own permanent outage via the very check meant to prevent + # a different one. Skipped there rather than guessed at; the + # content re-read just above is the only guard left on such a + # filesystem, the same accepted-and-documented tradeoff as the + # pid-recycling limitation noted near the top of this file. + if _lock_rename_preserves_identity "$_dir"; then + _claim_inode=$(_lock_file_inode "$_claim") + if [ -z "$_pre_steal_inode" ] || [ -z "$_claim_inode" ] || [ "$_pre_steal_inode" != "$_claim_inode" ]; then + _lock_handback "$_claim" "$_dir" + return 1 + fi fi if ( set -o noclobber; echo "$_LOCK_SELF" > "${_dir}/pid" ) 2>/dev/null; then rm -f "$_claim" 2>/dev/null || true @@ -646,12 +730,12 @@ _lock_try_steal() { # "could not confirm" and "confirmed identical" collapse into the same # branch is precisely the self-review finding above, one line later. if ! _lock_read_pid_timeout "$_claim"; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true + _lock_handback "$_claim" "$_dir" return 1 fi _seen="$_LOCK_PID_READ_RESULT" if [ "$_seen" != "$_pid" ]; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true + _lock_handback "$_claim" "$_dir" return 1 fi @@ -708,10 +792,12 @@ _lock_dir_age() { # # Same BSD-vs-GNU `stat` split as _lock_dir_age above, and the exact same # #198 gotcha applies to `%i`: GNU's `-f` means FILESYSTEM status, not file -# status, and its own `%i` under that mode is "free inodes on the -# filesystem" -- a large, plausible-looking, completely unrelated digit -# string that the naive digits-only guard would otherwise accept as a real -# inode number. Captured separately, exactly like the mtime case, so `-f` +# status, and its own `%i` under that mode is the filesystem's own ID, +# printed in hex -- not the count of free inodes (that is `%d`), as an +# earlier version of this comment wrongly claimed (#1124). Still a +# plausible-looking, completely unrelated string the naive digits-only +# guard would otherwise accept whenever that hex ID happens to come out +# all-digits. Captured separately, exactly like the mtime case, so `-f` # is only ever tried once `-c` has already failed outright (true on BSD, # where `-c` is not a valid flag at all) rather than once `-c` has produced # a wrong-but-numeric answer. diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index db8bf2bc..01dd9282 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -2198,6 +2198,73 @@ def test_a_fast_failing_claim_reread_does_not_steal_a_racing_winners_pid_1122( ) +def test_a_persistently_unreadable_pid_still_steals_when_rename_does_not_preserve_identity_1124( + tmp_path, +): + """#1124: `_lock_file_inode` assumes rename preserves a file's identity. + Not true everywhere -- FAT/exFAT under Cygwin/MSYS, SMB with + noserverino, FUSE without use_ino (#1103's own setup). There the #1122 + identity comparison can never match for the SAME file, so a + persistently-unreadable but genuinely stuck pid file would hand back + on every steal attempt forever -- #1103's outage, reopened by the + check meant to prevent a different one. + + Simulated by replacing `_lock_file_inode` with a function returning a + fresh, never-repeating value on every call, regardless of its + argument -- a stand-in for a filesystem whose rename never preserves + identity. `_lock_rename_preserves_identity`'s own self-test uses the + same function and must see the same unreliability and answer false, + so `_lock_try_steal` must skip the comparison rather than trust it, + and still steal. + """ + reason = _chmod_unreadable_skip_reason() + if reason: + pytest.skip(reason) + lock_dir = tmp_path / "steal-no-identity.lock" + lock_dir.mkdir() + pid_file = lock_dir / "pid" + pid_file.write_text("555555", encoding="utf-8") + pid_file.chmod(0o000) + try: + result = _run( + f""" + export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 + export REMEMBER_LOCK_PID_READ_TIMEOUT=1 + source {LIB_LOCK_SH} + _counter_file="{lock_dir}/.inode-counter" + echo 0 > "$_counter_file" + _lock_file_inode() {{ + local _n + _n=$(cat "$_counter_file") + _n=$((_n + 1)) + echo "$_n" > "$_counter_file" + echo "$_n" + }} + if _lock_try_steal "{lock_dir}"; then + echo STOLE + else + echo REFUSED + fi + """, + timeout=15, + ) + assert "STOLE" in result.stdout, ( + f"a persistently unreadable, old-enough pid file must still " + f"be stealable even when this filesystem's identity check " + f"can never report a match for the same file -- refusing " + f"forever here reproduces #1103's outage: " + f"{result.stdout!r} {result.stderr!r}" + ) + finally: + if pid_file.exists(): + pid_file.chmod(0o644) + new_pid = pid_file.read_text(encoding="utf-8").strip() + assert new_pid.isdigit(), ( + f"after a real steal, the pid file must hold this contender's " + f"own numeric pid: {new_pid!r}" + ) + + def test_a_persistently_unreadable_claim_with_no_race_still_steals_1122( tmp_path, ): @@ -2247,3 +2314,98 @@ def test_a_persistently_unreadable_claim_with_no_race_still_steals_1122( f"after a real steal, the pid file must hold this contender's own " f"numeric pid, not be left empty or non-numeric: {new_pid!r}" ) + + +def test_a_winner_completing_during_the_first_read_is_not_mistaken_for_the_placeholder_1124( + tmp_path, +): + """#1124, follow-on finding against #1123/#1122: the #1122 "before" + inode snapshot was taken AFTER both the first pid read (line ~484 in + the pre-#1124 source) had already timed out AND the age check further + down had also already run -- a window wide enough, since the read's + own timeout is whole seconds and a rename-plus-rewrite elsewhere is + microseconds, for another contender to complete an entire steal cycle + inside it. If that happens, the "before" snapshot is already of the + winner's own brand-new pid file, not of the stuck placeholder the read + judged -- and the later #1122 comparison then sees "unchanged" for a + file that was, in fact, swapped out from under this contender. + + This reproduces the mechanism by wrapping `_lock_read_pid_timeout`, + the function whose first call IS the read being judged: the wrapper + runs the real read against the real (stuck, chmod 000) placeholder + first, getting the correct failure, then -- as a side effect timed to + land exactly when that read returns, before anything else in + `_lock_try_steal` runs -- swaps in a winner's own fresh pid file, + ALSO chmod 000 so the subsequent content re-read of the claim fails + fast too (landing on #1122's own shape, not #1119's hang shape), and + forcing the decision down to the inode comparison. Built via mktemp + + mv, never rm-then-create in place, for the same bitmap-allocator + reason #1122's own test documents: a distinct temp file guarantees a + distinct inode before the swap, so the swap is observably different by + construction, not by allocator luck. + + Pre-#1124, the "before" snapshot captured at this point is already of + the swapped-in winner's file, so the comparison sees a match and + steals the winner's live lock -- two holders. Fixed by moving the + snapshot to the top of `_lock_try_steal`, before the first read is + even attempted, so it answers for the placeholder the read is about + to judge, not for whatever happens to be at the path afterward. + """ + reason = _chmod_unreadable_skip_reason() + if reason: + pytest.skip(reason) + lock_dir = tmp_path / "steal-read-race.lock" + lock_dir.mkdir() + pid_file = lock_dir / "pid" + pid_file.write_text("111111", encoding="utf-8") + pid_file.chmod(0o000) + winner_pid_text = "333333" + try: + result = _run( + f""" + export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 + export REMEMBER_LOCK_PID_READ_TIMEOUT=1 + source {LIB_LOCK_SH} + eval "$(declare -f _lock_read_pid_timeout | sed '1s/^_lock_read_pid_timeout ()/_orig_lock_read_pid_timeout ()/')" + _swap_marker="{lock_dir}/.swap-done" + rm -f "$_swap_marker" + _lock_read_pid_timeout() {{ + _orig_lock_read_pid_timeout "$@" + _rc=$? + if [ ! -e "$_swap_marker" ]; then + touch "$_swap_marker" + _winner_tmp=$(mktemp "{lock_dir}/.winner-pid.XXXXXX") + printf '%s' "{winner_pid_text}" > "$_winner_tmp" + chmod 000 "$_winner_tmp" + mv -f "$_winner_tmp" "{lock_dir}/pid" + fi + return $_rc + }} + if _lock_try_steal "{lock_dir}"; then + echo STOLE + else + echo REFUSED + fi + """, + timeout=15, + ) + assert "REFUSED" in result.stdout, ( + f"a winner that completes its own steal-and-rewrite cycle " + f"while this contender's FIRST read is still in flight must " + f"not be clobbered just because the 'before' identity snapshot " + f"was taken late enough to already see the winner's file -- " + f"that produces two simultaneous holders: " + f"{result.stdout!r} {result.stderr!r}" + ) + assert pid_file.exists(), ( + f"the hand-back must restore A pid file, not leave the lock " + f"pid-less: {result.stdout!r} {result.stderr!r}" + ) + finally: + if pid_file.exists(): + pid_file.chmod(0o644) + assert pid_file.read_text(encoding="utf-8") == winner_pid_text, ( + f"the winner's own pid must survive untouched: got " + f"{pid_file.read_text(encoding='utf-8')!r}, expected " + f"{winner_pid_text!r}: {result.stdout!r} {result.stderr!r}" + ) From d966c07d2543d409c970084b891c3d496424d005 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:10:23 +0100 Subject: [PATCH 2/3] Self-review follow-up for #1124: stop the identity probe from resetting lock staleness, and stop the hand-back's own fallback from clobbering Two spawned reviewers (Explore and oss:auditor) against the first #1124 commit found three more gaps, all in code this same commit introduced: - The new filesystem probe (_lock_rename_preserves_identity) ran inside the lock directory itself. Creating/renaming/removing any directory entry updates that directory's mtime, which is exactly what _lock_dir_age reads to judge staleness everywhere in this file -- so the probe was resetting the clock on every single steal attempt, reproduced directly (before=120 after=0), which would have let a permanently stuck lock look freshly touched forever (reopening #1101/#1103). The probe now runs in the lock directory's parent instead, which updates the parent's mtime, never the lock dir's. - _lock_handback's own "could not read the claim's content back" fallback was still a bare, unguarded mv -- the exact clobbering pattern the noclobber write above it exists to close, just left open on the unreadable-content path, which is the realistic one most hand-backs take. It now falls back to a guarded restore (only mv when the destination is confirmed still absent) in every case, readable content or not. - Every call site had already attempted its own read of the claim before ever calling _lock_handback, so the helper re-reading it a second time doubled the read-timeout latency on exactly the unreadable path. It now takes content the caller already read as an optional argument instead of re-reading. Added a regression test reproducing the staleness-clock bug directly (red on the pre-fix probe location, green after), and a test driving _lock_handback's noclobber write with a destination that already holds a different, legitimate pid. A fourth, lower-severity finding (orphaned probe litter if a process is killed mid-probe) is documented in the code and logged to trap.d/ rather than fixed under this issue's time budget. Co-Authored-By: Max --- changelog.d/1124.fixed.md | 11 ++- scripts/lib-lock.sh | 80 +++++++++++++++------ tests/test_lock_primitive.py | 88 +++++++++++++++++++++++ trap.d/1124.steal-probe-litter-on-kill.md | 27 +++++++ 4 files changed, 184 insertions(+), 22 deletions(-) create mode 100644 trap.d/1124.steal-probe-litter-on-kill.md diff --git a/changelog.d/1124.fixed.md b/changelog.d/1124.fixed.md index f54efdd0..2f3d2183 100644 --- a/changelog.d/1124.fixed.md +++ b/changelog.d/1124.fixed.md @@ -15,4 +15,13 @@ claim file back into place could silently overwrite a pid another process had already written in the meantime; hand-back now reads the claim's own content back and writes it with `noclobber`, the same atomicity the real - steal writes already use. + steal writes already use (self-review also tightened this: hand-back + prefers content the caller already read, since every call site had + already attempted a read before reaching it, and falls back to a + guarded restore rather than ever overwriting blindly). Self-review of + this fix found that its own filesystem probe, if run inside the lock + directory being judged, reset that directory's own staleness clock on + every steal attempt -- any directory-entry change updates a directory's + mtime, and `_lock_dir_age` reads exactly that -- which would have let a + permanently stuck lock look freshly touched forever; the probe now runs + in the lock directory's parent instead. diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index a4baa4dd..19a77552 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -447,19 +447,31 @@ _lock_read_pid_timeout() { } # Hand a claim file back to the lock's pid path without clobbering a pid -# some OTHER process already wrote there in the meantime (#1124). Reads -# the claim's own content back and writes it with `noclobber`, giving the -# restore the same atomicity the real steal writes already have below. -# Falls back to a plain `mv` only if the claim's content cannot even be -# read back. +# some OTHER process already wrote there in the meantime (#1124). $3, when +# given, is content the CALLER already read back from $_claim -- never +# re-read here. Self-review found two problems with an earlier version +# that always re-read: every call site had ALREADY attempted its own read +# of $_claim before reaching here, so re-reading inside this function paid +# the full read-timeout bound a SECOND time on exactly the path where the +# first read already failed -- the common case, since an unreadable pid is +# the whole reason #1101/#1103/#1119/#1120 exist. And its "could not read +# back" fallback was a bare, unguarded `mv` -- precisely the clobbering +# pattern the noclobber write below exists to close, left open whenever +# content could not be read, which is the common case too. Both are fixed +# by making "no known content" and "write already found something there" +# converge on the SAME guarded restore path below. _lock_handback() { - local _claim="$1" _dir="$2" _content - if _lock_read_pid_timeout "$_claim"; then - _content="$_LOCK_PID_READ_RESULT" - if ( set -o noclobber; echo "$_content" > "${_dir}/pid" ) 2>/dev/null; then - rm -f "$_claim" 2>/dev/null || true - return 0 - fi + local _claim="$1" _dir="$2" _content="${3:-}" _write_ok=1 + if [ -z "$_content" ]; then + _write_ok=0 + elif ! ( set -o noclobber; echo "$_content" > "${_dir}/pid" ) 2>/dev/null; then + _write_ok=0 + fi + if [ "$_write_ok" = 1 ]; then + rm -f "$_claim" 2>/dev/null || true + return 0 + fi + if [ -e "${_dir}/pid" ]; then rm -f "$_claim" 2>/dev/null || true return 1 fi @@ -478,18 +490,44 @@ _lock_handback() { # back on every steal attempt forever -- reopening #1103's own outage # with the very check meant to prevent a different bug. # -# Probed with a throwaway file IN itself so the answer is for the -# exact filesystem the steal is about to run on. Only reached from inside -# a steal attempt, already paying for a multi-second read timeout above -# it, and never on the uncontended acquire path, which never calls -# _lock_try_steal at all. +# NOT probed inside itself (self-review finding, #1124): creating, +# renaming or removing ANY directory entry updates that directory's own +# mtime, and _lock_dir_age -- the staleness clock every caller of this +# function is in the middle of consulting -- is exactly 's mtime. +# Reproduced directly: sourcing this file, writing a pid, waiting 2s, then +# calling this probe against that same directory reset `_lock_dir_age` from +# 2 back to 0 -- the probe was resetting the very clock _lock_try_steal +# relies on to ever judge the lock stale at all, letting a permanently +# stuck lock look freshly touched on every single steal attempt and never +# age far enough to be reclaimed (the #1101/#1103 outage this whole file +# exists to close, reopened by this probe's own side effect). Probed in +# 's PARENT instead: creating/renaming/removing an entry in the +# parent updates the PARENT's own mtime, never 's, and the parent is +# the same filesystem sits on in every layout this file assumes. +# +# Only reached from inside a steal attempt, already paying for a +# multi-second read timeout above it, and never on the uncontended acquire +# path, which never calls _lock_try_steal at all. # # A probe that itself fails to create or rename answers false -- the safe # default: it disables a check this contender cannot even confirm works, # rather than trusting metadata that was never verified meaningful here. +# A process killed between creating the probe below and this function's own +# `rm -f` cleanup leaves a leftover ".steal-probe.$pid" (or its ".moved" +# sibling) behind permanently -- nothing here sweeps a dead owner's old +# probe the way the pid.stealing.* claims above are swept. Accepted rather +# than reaped (#1124 self-review): the probe now lives in 's PARENT, +# which may hold many unrelated lock directories' own debris, so a correct +# sweep needs care not to touch another lock's files -- and the leftover +# is harmless to correctness, since nothing ever reads it and a clean +# release's `rm -rf "$_dir"` does not reach it either, being a sibling of +# $_dir rather than inside it. Same shape as the pid-recycling limitation +# documented near the top of this file: logged rather than solved here. _lock_rename_preserves_identity() { - local _dir="$1" _probe _moved _i1 _i2 - _probe="${_dir}/.steal-probe.${_LOCK_SELF}" + local _dir="$1" _parent _probe _moved _i1 _i2 + _parent="${_dir%/*}" + [ "$_parent" = "$_dir" ] && _parent="." + _probe="${_parent}/.steal-probe.${_LOCK_SELF}" _moved="${_probe}.moved" rm -f "$_probe" "$_moved" 2>/dev/null || true ( umask 077; : > "$_probe" ) 2>/dev/null || return 1 @@ -624,7 +662,7 @@ _lock_try_steal() { # contender's signal that it grabbed someone else's win by # mistake and must hand it back rather than overwrite it. if _lock_read_pid_timeout "$_claim"; then - _lock_handback "$_claim" "$_dir" + _lock_handback "$_claim" "$_dir" "$_LOCK_PID_READ_RESULT" return 1 fi # #1122: the re-read above failing is NOT by itself proof that @@ -735,7 +773,7 @@ _lock_try_steal() { fi _seen="$_LOCK_PID_READ_RESULT" if [ "$_seen" != "$_pid" ]; then - _lock_handback "$_claim" "$_dir" + _lock_handback "$_claim" "$_dir" "$_seen" return 1 fi diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index 01dd9282..bf2ccd6f 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -2265,6 +2265,94 @@ def test_a_persistently_unreadable_pid_still_steals_when_rename_does_not_preserv ) +def test_identity_probe_does_not_reset_the_lock_directory_staleness_clock_1124( + tmp_path, +): + """#1124 self-review (Explore reviewer, finding 1): + `_lock_rename_preserves_identity`'s own throwaway probe must not live + inside the lock directory it is answering for -- creating, renaming + or removing ANY entry in a directory updates that directory's own + mtime, which is exactly what `_lock_dir_age` reads to decide staleness + everywhere in this file. A probe inside would reset the clock on + every single steal attempt, letting a permanently stuck lock look + freshly touched forever and never age far enough to be reclaimed -- + the #1101/#1103 outage reopened by the very helper meant to avoid a + different one. Probing in 's parent instead must leave 's + own mtime untouched. + """ + lock_dir = tmp_path / "probe-mtime.lock" + lock_dir.mkdir() + (lock_dir / "pid").write_text("999999", encoding="utf-8") + old_time = time.time() - 120 + os.utime(lock_dir, (old_time, old_time)) + result = _run( + f""" + source {LIB_LOCK_SH} + _lock_self_set + echo "before=$(_lock_dir_age "{lock_dir}")" + _lock_rename_preserves_identity "{lock_dir}" >/dev/null 2>&1 + echo "after=$(_lock_dir_age "{lock_dir}")" + """, + timeout=15, + ) + stdout = result.stdout + before = int(stdout.split("before=")[1].split()[0]) + after = int(stdout.split("after=")[1].split()[0]) + assert before >= 100, ( + f"the harness itself is broken if the pre-probe age was not " + f"already ~120s old: {stdout!r} {result.stderr!r}" + ) + assert after >= 100, ( + f"_lock_rename_preserves_identity's own throwaway probe must not " + f"touch the LOCK DIRECTORY's mtime -- creating/renaming/removing " + f"a directory entry updates that directory's own mtime, and " + f"_lock_dir_age reads exactly that, so probing inside the lock " + f"directory itself resets the staleness clock every later steal " + f"attempt on this same lock relies on, letting a permanently " + f"stuck lock look freshly touched forever (reopens #1101/#1103): " + f"before={before} after={after} {stdout!r} {result.stderr!r}" + ) + + +def test_handback_never_clobbers_a_pid_another_process_legitimately_wrote( + tmp_path, +): + """Self-review of #1124 (oss:auditor finding): `_lock_handback`'s whole + purpose is to restore a claim without clobbering a pid some OTHER + process already wrote to "${_dir}/pid" in the meantime. Exercises the + noclobber write directly, with readable content AND a destination that + already holds a different, legitimate pid -- the one branch nothing in + the original diff's own tests drove down. The write must decline, the + real pid must survive untouched, and the claim must still be dropped + rather than left as litter. + """ + lock_dir = tmp_path / "handback-no-clobber.lock" + lock_dir.mkdir() + claim = lock_dir / "pid.stealing.999999" + claim.write_text("111111", encoding="utf-8") + (lock_dir / "pid").write_text("222222", encoding="utf-8") + result = _run( + f""" + source {LIB_LOCK_SH} + _lock_handback "{claim}" "{lock_dir}" "111111" + """, + timeout=15, + ) + assert result.stdout == "", ( + f"unexpected stdout from a hand-back: {result.stdout!r} " + f"{result.stderr!r}" + ) + assert (lock_dir / "pid").read_text(encoding="utf-8") == "222222", ( + f"a hand-back must never overwrite a pid another process " + f"legitimately holds, even when the restoring content is known " + f"and readable: {(lock_dir / 'pid').read_text(encoding='utf-8')!r}" + ) + assert not claim.exists(), ( + "a hand-back that correctly declined to restore must still drop " + "its own claim file rather than leave it as litter" + ) + + def test_a_persistently_unreadable_claim_with_no_race_still_steals_1122( tmp_path, ): diff --git a/trap.d/1124.steal-probe-litter-on-kill.md b/trap.d/1124.steal-probe-litter-on-kill.md new file mode 100644 index 00000000..f3280cfb --- /dev/null +++ b/trap.d/1124.steal-probe-litter-on-kill.md @@ -0,0 +1,27 @@ +Self-review of #1124 (Explore reviewer, finding 4) found that +`_lock_rename_preserves_identity`'s own throwaway probe files +(`.steal-probe.$pid` / `.steal-probe.$pid.moved`, created in the lock +directory's PARENT so the probe does not reset the directory's own +staleness clock -- see the comment above that function) have no reaping +logic the way the `pid.stealing.*` claim files a few lines above do. + +A process killed (SIGKILL, OOM) between creating the probe and this +function's own cleanup leaves the leftover behind permanently. Not a +correctness hazard on its own -- nothing reads it back, and a clean +release's `rm -rf "$_dir"` does not reach it either, since the parent +directory is never removed. It is pure disk litter, same class the +pid.stealing.* sweep exists to prevent for claim files, just not applied +here. + +Ranked non-blocking (Low-Medium) because the probe now lives in the +lock directory's PARENT rather than inside it, which may be shared by +several unrelated lock directories -- a correct sweep needs to identify +and remove only entries whose owning pid is dead, without touching a +sibling lock's own files, which is more care than the existing +pid.stealing.* sweep needed (that one only ever sees entries belonging +to its own lock directory). + +Left as documented-and-accepted in the code comment (scripts/lib-lock.sh, +just above `_lock_rename_preserves_identity`) rather than solved under +this issue's time budget. Worth a dedicated reap pass if this ever +becomes an actual accumulation problem in practice. From e00455a03b4648eace0101a405f7f6128acf032e Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:23:02 +0100 Subject: [PATCH 3/3] Second-pass self-review for #1124: fix the empty-content conflation and the trailing-slash probe-location bug Second review round over the previous follow-up commit (same two spawns, over the follow-up diff alone, per fix_commit_scope.py's needs-second-pass verdict) found two more real gaps in that commit's own new code, independently from both reviewers: - _lock_handback used "[ -z "$_content" ]" to decide whether content was known. A pid file legitimately read as empty (a holder mid-acquisition, or one that died before writing its pid -- both real, documented cases) produces that same empty string from a SUCCESSFUL read, so it was routed into the weaker fallback restore path instead of the atomic noclobber write every other known value gets. Fixed by testing "$#" (was a 3rd argument supplied at all) instead of the value's emptiness. - _lock_rename_preserves_identity's parent derivation ("${_dir%/*}") strips only one trailing slash from a path that already ends in "/", landing back on the SAME directory rather than its parent -- silently reopening the staleness-clock bug the relocation exists to close, for any caller that happens to pass a trailing slash. Fixed with a portable (bash 3.2-safe) case/glob loop that strips all trailing slashes before deriving the parent. Both auditor findings about the fallback's own residual check-then-act window (narrower than the bare mv it replaced, not zero) are documented in the code and logged to trap.d/ rather than fixed, since closing it needs an atomic "create only if absent" primitive this function does not have for unknown content. Two new regression tests, both red on the pre-fix code and green after: one reproducing the empty-content misroute by making the claim's own on-disk bytes differ from the content passed, one reproducing the trailing-slash bug directly against _lock_dir_age. Co-Authored-By: Max --- changelog.d/1124.fixed.md | 8 +- scripts/lib-lock.sh | 37 ++++++++- tests/test_lock_primitive.py | 76 +++++++++++++++++++ .../1124.handback-fallback-residual-toctou.md | 25 ++++++ 4 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 trap.d/1124.handback-fallback-residual-toctou.md diff --git a/changelog.d/1124.fixed.md b/changelog.d/1124.fixed.md index 2f3d2183..cf15e0f8 100644 --- a/changelog.d/1124.fixed.md +++ b/changelog.d/1124.fixed.md @@ -24,4 +24,10 @@ every steal attempt -- any directory-entry change updates a directory's mtime, and `_lock_dir_age` reads exactly that -- which would have let a permanently stuck lock look freshly touched forever; the probe now runs - in the lock directory's parent instead. + in the lock directory's parent instead -- correctly even when the + directory argument carries a trailing slash. A third review round also + found that hand-back's own "is this content known" check treated a + legitimately empty pid read the same as "no read was ever attempted", + routing a real, known-empty value into a weaker restore path than every + other known value gets; fixed by testing whether the argument was + supplied at all, not whether its value happens to be empty. diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index 19a77552..8dff4f93 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -462,7 +462,16 @@ _lock_read_pid_timeout() { # converge on the SAME guarded restore path below. _lock_handback() { local _claim="$1" _dir="$2" _content="${3:-}" _write_ok=1 - if [ -z "$_content" ]; then + # $# (second-pass self-review, #1124), not "[ -z "$_content" ]": a pid + # file this function's caller legitimately read AS EMPTY (a holder + # mid-acquisition, or one that died before writing its pid -- both + # documented a few lines below) is content just as real as any other, + # and "${3:-}" collapsed that case into the SAME "unknown" bucket as + # "the caller never attempted a read at all", routing a real, known + # empty value into the weaker fallback below instead of the atomic + # noclobber write. "$#" answers "was a 3rd argument given AT ALL", + # regardless of its value, which is the distinction this needs. + if [ "$#" -lt 3 ]; then _write_ok=0 elif ! ( set -o noclobber; echo "$_content" > "${_dir}/pid" ) 2>/dev/null; then _write_ok=0 @@ -471,6 +480,17 @@ _lock_handback() { rm -f "$_claim" 2>/dev/null || true return 0 fi + # Narrower than the bare `mv` this replaces, not zero (second-pass + # self-review, #1124): there is no content here to restore via the + # atomic noclobber write above (content was never known, or was known + # and the write above already found something there), so the best + # available guard is check-then-act -- a legitimate writer landing in + # the few-instruction gap between this `-e` test and the `mv` below + # would still get clobbered. Accepted rather than closed: closing it + # would need an atomic "create only if absent, with THIS content" + # primitive for content this function does not have, the same + # tradeoff documented for the pid-recycling and inode-portability + # limitations elsewhere in this file. if [ -e "${_dir}/pid" ]; then rm -f "$_claim" 2>/dev/null || true return 1 @@ -525,6 +545,21 @@ _lock_handback() { # documented near the top of this file: logged rather than solved here. _lock_rename_preserves_identity() { local _dir="$1" _parent _probe _moved _i1 _i2 + # Trailing slash(es) stripped first (second-pass self-review, #1124): + # "${_dir%/*}" on a path that already ends in "/" strips only that + # slash, landing back on the SAME directory rather than its parent -- + # silently undoing the whole point of this relocation for any caller + # that happens to pass a trailing slash, reopening the staleness-clock + # bug the relocation exists to close. A `case`/glob loop, not bash's + # `${var: -N}` substring form: negative offsets there need bash >= 4.2, + # and this file is sourced under bash 3.2 (macOS's shipped /bin/bash). + while :; do + case "$_dir" in + */) _dir="${_dir%/}" ;; + *) break ;; + esac + done + [ -n "$_dir" ] || _dir="/" _parent="${_dir%/*}" [ "$_parent" = "$_dir" ] && _parent="." _probe="${_parent}/.steal-probe.${_LOCK_SELF}" diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index bf2ccd6f..82e5089e 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -2314,6 +2314,82 @@ def test_identity_probe_does_not_reset_the_lock_directory_staleness_clock_1124( ) +def test_identity_probe_still_avoids_the_lock_directory_with_a_trailing_slash( + tmp_path, +): + """Second-pass self-review of #1124 (oss:auditor finding): the parent + derivation "${_dir%/*}" strips only ONE trailing slash from a path + that already ends in "/", landing back on the SAME directory rather + than its parent -- silently reopening the staleness-clock bug this + relocation exists to close, for any caller that happens to pass a + trailing slash. Same probe as + test_identity_probe_does_not_reset_the_lock_directory_staleness_clock_1124, + with a trailing slash appended to the path handed to the function + under test. + """ + lock_dir = tmp_path / "probe-mtime-trailing-slash.lock" + lock_dir.mkdir() + (lock_dir / "pid").write_text("888888", encoding="utf-8") + old_time = time.time() - 120 + os.utime(lock_dir, (old_time, old_time)) + dir_arg = str(lock_dir) + "/" + result = _run( + f""" + source {LIB_LOCK_SH} + _lock_self_set + echo "before=$(_lock_dir_age "{lock_dir}")" + _lock_rename_preserves_identity "{dir_arg}" >/dev/null 2>&1 + echo "after=$(_lock_dir_age "{lock_dir}")" + """, + timeout=15, + ) + stdout = result.stdout + before = int(stdout.split("before=")[1].split()[0]) + after = int(stdout.split("after=")[1].split()[0]) + assert after >= 100, ( + f"a trailing slash on the directory argument must not defeat the " + f"parent-directory relocation: before={before} after={after} " + f"{stdout!r} {result.stderr!r}" + ) + + +def test_handback_treats_a_legitimately_empty_read_as_known_content( + tmp_path, +): + """Second-pass self-review of #1124 (both reviewers independently): + "${3:-}" collapses "the caller never attempted a read" and "the + caller read successfully and the pid file is legitimately empty" (a + holder mid-acquisition, or one that died before writing its pid -- + both real cases documented elsewhere in this file) into the SAME + "unknown content" bucket, routing a known-empty value into the + weaker fallback instead of the atomic noclobber write. The claim's + own on-disk content is deliberately set to something ELSE, so the + two branches are distinguishable: the fallback would restore the + claim's unrelated real bytes; the noclobber write restores the + EMPTY content that was actually passed. + """ + lock_dir = tmp_path / "handback-empty-content.lock" + lock_dir.mkdir() + claim = lock_dir / "pid.stealing.888888" + claim.write_text("999999", encoding="utf-8") + _run( + f""" + source {LIB_LOCK_SH} + _lock_handback "{claim}" "{lock_dir}" "" + """, + timeout=15, + ) + pid_file = lock_dir / "pid" + assert pid_file.exists(), "a known-content hand-back must still restore" + content = pid_file.read_text(encoding="utf-8") + assert content.strip() == "", ( + f"a legitimately empty read must be restored as empty via the " + f"noclobber write, not via the unknown-content fallback, which " + f"would restore the claim's own unrelated on-disk bytes instead: " + f"{content!r}" + ) + + def test_handback_never_clobbers_a_pid_another_process_legitimately_wrote( tmp_path, ): diff --git a/trap.d/1124.handback-fallback-residual-toctou.md b/trap.d/1124.handback-fallback-residual-toctou.md new file mode 100644 index 00000000..8f57c53a --- /dev/null +++ b/trap.d/1124.handback-fallback-residual-toctou.md @@ -0,0 +1,25 @@ +Second-pass self-review of #1124 (both Explore and oss:auditor, independently, +on the same follow-up commit) found that `_lock_handback`'s own fallback path +(`scripts/lib-lock.sh`, used when no content is known, or the noclobber write +already found something at the destination) is check-then-act rather than +atomic: `[ -e "${_dir}/pid" ]` then `mv "$_claim" "${_dir}/pid"`. A legitimate +writer landing in the few-instruction gap between the check and the `mv` +still gets clobbered -- narrower than the bare, unguarded `mv` this same +follow-up replaced (which clobbered unconditionally), but not zero. + +Both reviewers ranked this `fails-to-preserve`, non-blocking, ship-behind- +trap.d. Closing it fully would need an atomic "create only if absent" write +for a case where this function does not have the content to write (that is +exactly why it falls to `mv` here rather than the noclobber `echo` used +elsewhere) -- there is no portable bash primitive for "atomically move a +file into a path only if that path is still absent" the way there is for +"atomically write new content only if the path is absent" (`set -o +noclobber`). The comment just above the fallback in scripts/lib-lock.sh +documents this explicitly as an accepted tradeoff, the same shape as the +pid-recycling and inode-portability limitations already documented +elsewhere in this file. + +Worth a dedicated look if this residual window is ever observed to matter +in practice (it would need a legitimate writer to land in a gap measured in +shell instructions, immediately after this contender's own failed-or- +declined steal), but not solved under #1124's own time budget.