diff --git a/scripts/desktop-update/marker.ps1 b/scripts/desktop-update/marker.ps1 index 6c8e67b876..0db9e255a2 100644 --- a/scripts/desktop-update/marker.ps1 +++ b/scripts/desktop-update/marker.ps1 @@ -387,15 +387,18 @@ function Test-CheckoutLockHeld { # could not bind also locks one of the 16 lease bytes just past it (R5b): a leased # child outlives a killed owner and keeps the checkout busy. Probe all 1+16 bytes # in one LockFile (it fails if ANY of them is held) and give them straight back. + # A lock file that exists but cannot be opened or probed (denied, opened without + # sharing) counts as held, like marker.sh and the Desktop's own probe: reclaim + # needs a provably free checkout. $path = Get-CheckoutLockPath if (-not $path -or -not [System.IO.File]::Exists($path)) { return $false } $fs = $null try { $fs = [System.IO.File]::Open($path, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Read, ([System.IO.FileShare]::ReadWrite -bor [System.IO.FileShare]::Delete)) - } catch { return $false } + } catch [System.IO.FileNotFoundException], [System.IO.DirectoryNotFoundException] { return $false } catch { return $true } try { - try { $fs.Lock(1048576, 17) } catch [System.IO.IOException] { return $true } + try { $fs.Lock(1048576, 17) } catch { return $true } try { $fs.Unlock(1048576, 17) } catch {} return $false } finally { diff --git a/scripts/desktop-update/marker.sh b/scripts/desktop-update/marker.sh index 52e3bc2011..2157f52ae8 100644 --- a/scripts/desktop-update/marker.sh +++ b/scripts/desktop-update/marker.sh @@ -531,7 +531,9 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right # take and the drop are two consecutive syscalls in one process that opened # the file itself -- no process exit, wait or shell work in between. flock(1) # can only take it on a shell fd, so there the hold spans flock(1)'s exit and - # our close. No tool: held (fail closed, the release waits). + # our close. No tool, or a lock file that exists but cannot be opened (root- + # owned after `sudo hermes update`): held -- fail closed like the Desktop's + # own probe (checkout_held): reclaim needs a provably free checkout. local path rc tool="$MARKER_LOCK_TOOL_FORCED" path="$(checkout_lock_path)" [ -f "$path" ] || return 1 @@ -545,7 +547,7 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right case "$tool" in perl) perl -MFcntl=:flock -e ' - open(my $f, "<", $ARGV[0]) or exit 2; + open(my $f, "<", $ARGV[0]) or exit($!{ENOENT} ? 3 : 2); flock($f, LOCK_EX | LOCK_NB) or exit 1; flock($f, LOCK_UN); exit 0' "$path"; rc=$? ;; python3) @@ -553,6 +555,8 @@ checkout_lock_held() { # 0 iff some process holds the checkout kernel lock right import fcntl, os, sys try: fd = os.open(sys.argv[1], os.O_RDONLY) +except FileNotFoundError: + sys.exit(3) except OSError: sys.exit(2) try: @@ -561,12 +565,12 @@ except OSError: sys.exit(1) fcntl.flock(fd, fcntl.LOCK_UN)' "$path"; rc=$? ;; flock) - { exec 8<"$path"; } 2>/dev/null || return 1 + { exec 8<"$path"; } 2>/dev/null || { [ -e "$path" ]; return $?; } flock -x -n 8; rc=$? exec 8<&- ;; # closing our probe fd releases it when we did get it *) rc=1 ;; esac - [ "$rc" -eq 1 ] + [ "$rc" -ne 0 ] && [ "$rc" -ne 3 ] # 3: it vanished since the -f look } # ── helper ops for the Desktop (it never mutates the marker itself) ────────── diff --git a/tests/scripts/desktop_update/test_desktop_update_posix_handoff_protocol.py b/tests/scripts/desktop_update/test_desktop_update_posix_handoff_protocol.py index c44c8f227d..1f181d8508 100644 --- a/tests/scripts/desktop_update/test_desktop_update_posix_handoff_protocol.py +++ b/tests/scripts/desktop_update/test_desktop_update_posix_handoff_protocol.py @@ -337,6 +337,32 @@ def test_reclaim_without_a_marker_reports_held_while_the_checkout_lock_is_held(t assert _helper(tmp_path, home, install, "reclaim") == "absent" +@pytest.mark.skipif(os.geteuid() == 0, reason="root opens a mode-000 file") +@pytest.mark.parametrize("tool", ["perl", "python3", "flock"]) +def test_an_unreadable_checkout_lock_counts_as_held(tmp_path, tool): + """kshitij P2: a lock file that exists but cannot be opened (root-owned after `sudo hermes + update`) read `not held`, while the Desktop's own probe (checkout_held) reads `held` -- so the + local reclaim deleted a dead marker the remote helper kept. Fail closed, every probe tool.""" + if shutil.which(tool) is None: + pytest.skip(f"{tool} is not installed") + home, install = _install(tmp_path, legacy=True) + lock = install / ".hermes-update.lock" + probe = (f"log() {{ :; }}; MARKER=/dev/null INSTALL_ROOT={shlex.quote(str(install))} MARKER_LOCK_TOOL={tool}; " + f". {shlex.quote(str(MARKER_SH))}; checkout_lock_held && echo held || echo free") + def held() -> str: + return subprocess.run(["bash", "-c", probe], capture_output=True, text=True, timeout=30).stdout.strip() + assert held() == "free" # absent + lock.touch(); lock.chmod(0) + try: + assert held() == "held" + if tool == "perl": # end to end: the Desktop's reclaim keeps the dead marker + (home / ".hermes-update-in-progress").write_text(f"999999\n{int(time.time())}\nct:1.000\n", encoding="utf-8") + assert _helper(tmp_path, home, install, "reclaim") == "held" + finally: + lock.chmod(0o644) + assert held() == "free" + + def test_release_waits_for_the_survivor_then_removes_the_marker(tmp_path): home, install = _install(tmp_path, legacy=True) completion = tmp_path / "release-completion" diff --git a/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py b/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py index df4fdd3347..0f570aba0f 100644 --- a/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py +++ b/tests/scripts/desktop_update/test_desktop_update_windows_marker_ops.py @@ -224,6 +224,31 @@ def test_marker_op_reclaim_without_a_marker_reports_held_while_the_checkout_lock assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'absent\n') +@pytest.mark.platforms('windows') +def test_marker_op_reclaim_reports_held_while_the_checkout_lock_cannot_be_opened(tmp_path: Path) -> None: + """kshitij P2 (parity with marker.sh and the Desktop's probe): a lock file that exists but + cannot be opened -- here another process holds it with no sharing -- read `not held`, so the + reclaim deleted a dead marker the Desktop's own probe kept. It counts as held.""" + import ctypes + + install = tmp_path / 'hermes-agent' + install.mkdir() + lock = install / '.hermes-update.lock' + lock.touch() + dead = f'{_dead_pid()}\n{int(time.time())}\nct:5.000\n'.encode() + (tmp_path / MARKER).write_bytes(dead) + k32 = ctypes.WinDLL('kernel32', use_last_error=True) + k32.CreateFileW.restype = ctypes.c_void_p + handle = k32.CreateFileW(str(lock), 0x80000000, 0, None, 3, 0x80, None) # GENERIC_READ, no sharing + assert handle not in (None, ctypes.c_void_p(-1).value), ctypes.get_last_error() + try: + assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'held\n') + assert (tmp_path / MARKER).read_bytes() == dead + finally: + k32.CloseHandle(ctypes.c_void_p(handle)) + assert _op(tmp_path, '-MarkerOp', 'reclaim')[:2] == (0, 'reclaimed\n') + + @pytest.mark.platforms('windows') @pytest.mark.parametrize('lease', [1, 16]) def test_marker_op_reclaim_reports_held_while_a_leased_child_outlives_its_owner(tmp_path: Path, lease: int) -> None: