diff --git a/changelog.d/1124.fixed.md b/changelog.d/1124.fixed.md new file mode 100644 index 00000000..cf15e0f8 --- /dev/null +++ b/changelog.d/1124.fixed.md @@ -0,0 +1,33 @@ +- 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 (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 -- 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 9fa56d11..8dff4f93 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -446,6 +446,136 @@ _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). $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="${3:-}" _write_ok=1 + # $# (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 + fi + if [ "$_write_ok" = 1 ]; then + 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 + 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. +# +# 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" _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}" + _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 +611,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 +677,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 +697,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" "$_LOCK_PID_READ_RESULT" return 1 fi # #1122: the re-read above failing is NOT by itself proof that @@ -580,10 +725,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 +803,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" "$_seen" return 1 fi @@ -708,10 +865,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..82e5089e 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -2198,6 +2198,237 @@ 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_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_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, +): + """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, ): @@ -2247,3 +2478,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}" + ) 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. 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.