From 90e916efc9769e68883e252753ab8570ea2de8e5 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 31 Aug 2026 09:08:54 -0700 Subject: [PATCH] fix(windows): compose the taskkill identity guards into one fail-closed class fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvage hardening on top of the three cherry-picked contributor commits (#91297 gebilaowang404 + AlexMnrs, #96741 burak33bb, #98826 ayushnangia), closing the remaining unverified-PID kill sites as one class (#98814, #89614): - pid_is_hermes: token-boundary 'hermes' match (no more loose substring false-positives), and an explicit start-time expectation is now honored on POSIX too (a mismatched fingerprint is a recycled PID on any platform). - kill_process_tree: drop the guard on our OWN retained Popen child — a retained handle pins the PID, so the check could only false-refuse. - gateway.status.terminate_pid: POSIX force-kills also refuse when a caller-provided expected_start_time no longer matches. - kill_gateway_processes: re-verify the LIVE cmdline at kill time (the scan-time match is a TOCTOU window). - _reap_unsupervised_gateway_orphans: fingerprint orphans at scan time and require a still-matching identity before the delayed SIGKILL escalation. - whatsapp _kill_port_process: never kill a bare netstat/lsof-scanned PID unless the live process is actually a node bridge (was a stranger-kill). - browser daemon reap/close paths: pass the start-time fingerprint into ProcessRegistry._terminate_host_pid (previously unverified), and the session-close path now runs the same daemon identity verification as the orphan reaper. - tests/hermes_cli/test_taskkill_identity_windows_live.py: live Windows probes (real spawned processes, real psutil ancestry) wired into the on-demand windows-latest wine2e lane. Fixes #98814 Fixes #89614 --- .github/workflows/windows-venv-e2e.yml | 1 + gateway/status.py | 9 + hermes_cli/_subprocess_compat.py | 63 +++--- hermes_cli/gateway.py | 33 ++- plugins/platforms/whatsapp/adapter.py | 42 +++- tests/gateway/test_whatsapp_connect.py | 18 ++ tests/hermes_cli/test_gateway.py | 26 +++ tests/hermes_cli/test_stale_pid_guard.py | 58 +++--- .../test_taskkill_identity_windows_live.py | 192 ++++++++++++++++++ tests/tools/test_browser_orphan_reaper.py | 28 ++- tools/browser_tool.py | 36 +++- 11 files changed, 450 insertions(+), 56 deletions(-) create mode 100644 tests/hermes_cli/test_taskkill_identity_windows_live.py diff --git a/.github/workflows/windows-venv-e2e.yml b/.github/workflows/windows-venv-e2e.yml index 89b3ebf56e..f3ed1cd15d 100644 --- a/.github/workflows/windows-venv-e2e.yml +++ b/.github/workflows/windows-venv-e2e.yml @@ -58,4 +58,5 @@ jobs: set -uo pipefail uv run --no-sync python -m pytest \ tests/hermes_cli/test_venv_holder_windows_live.py \ + tests/hermes_cli/test_taskkill_identity_windows_live.py \ -o addopts= -v -p no:cacheprovider diff --git a/gateway/status.py b/gateway/status.py index 9b5b62b0e1..9704d74594 100644 --- a/gateway/status.py +++ b/gateway/status.py @@ -334,7 +334,16 @@ def terminate_pid( POSIX uses SIGTERM/SIGKILL. Windows uses taskkill /T /F for true force-kill because os.kill(..., SIGTERM) is not equivalent to a tree-killing hard stop. + + Identity guard: on Windows, ``force=True`` REQUIRES a matching + ``expected_start_time`` (fail closed — taskkill /T /F on a recycled PID + has killed svchost.exe and blue-screened the host, #89614). On POSIX an + expectation is optional, but when the caller provides one and it no + longer matches the live process, the kill is refused on every platform — + a mismatched fingerprint always means the PID was recycled. """ + if force and expected_start_time is not None and not _IS_WINDOWS: + _assert_process_start_time_matches(pid, expected_start_time) if force and _IS_WINDOWS: _assert_process_start_time_matches(pid, expected_start_time) # CREATE_NO_WINDOW: terminate_pid runs from the windowless pythonw.exe diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 016ed8cfc3..0a47394a66 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -29,6 +29,7 @@ guarantee. from __future__ import annotations import os +import re import shutil import subprocess import sys @@ -399,6 +400,23 @@ def _process_start_time(pid: int) -> int | None: return None +def _text_names_hermes(text: str) -> bool: + """True when *text* names Hermes at a path-segment / token boundary. + + A bare ``"hermes" in text`` substring test would also match unrelated + processes whose paths merely contain the letters (``...\\shermesa\\...``), + which is exactly the false-positive class this guard exists to prevent. + Instead, split on path separators and whitespace and require a segment + that *starts with* ``hermes`` (``hermes``, ``hermes.exe``, ``hermes_cli``, + ``hermes-agent``, ``hermes-runtime``) or the hidden-dir form + ``.hermes``/``.hermes-runtime``. + """ + for token in re.split(r"[\\/\s=,;\"']+", text.lower()): + if token.startswith("hermes") or token.startswith(".hermes"): + return True + return False + + def _process_command_is_hermes(pid: int) -> bool: """Best-effort check that *pid* currently runs Hermes code.""" try: @@ -407,7 +425,7 @@ def _process_command_is_hermes(pid: int) -> bool: process = psutil.Process(pid) command = " ".join(process.cmdline() or []) executable = process.exe() or "" - return "hermes" in f"{command} {executable}".lower() + return _text_names_hermes(f"{command} {executable}") except Exception: return False @@ -423,12 +441,19 @@ def pid_is_hermes( the caller captured a start-time fingerprint before the destructive action, the live process must still have the same ``(pid, start_time)`` identity. Any ambiguity fails closed. Non-Windows callers have no ``taskkill`` path, - so a valid PID is accepted there. + so a valid PID with no (or a matching) explicit expectation is accepted + there — but a caller-provided fingerprint that no longer matches is a + recycled PID on every platform and is always refused. """ if not isinstance(pid, int) or isinstance(pid, bool) or pid <= 0: return False if not IS_WINDOWS: - return True + if expected_start_time is None: + return True + try: + return _process_start_time(pid) == expected_start_time + except Exception: + return False try: current_start_time = _process_start_time(pid) @@ -447,11 +472,7 @@ def pid_is_hermes( return False -def kill_process_tree( - proc: "subprocess.Popen", - *, - expected_start_time: int | None = None, -) -> None: +def kill_process_tree(proc: "subprocess.Popen") -> None: """Best-effort terminate *proc* and its descendants on both platforms. ``proc.kill()`` alone only terminates the direct child. On Windows a @@ -523,20 +544,15 @@ def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None: except OSError: pass if IS_WINDOWS: + # No identity guard here on purpose: *proc* is our own retained + # ``Popen`` handle. The child cannot be reaped (and its PID cannot be + # recycled) while we still hold the handle, so an identity check could + # only ever false-refuse a legitimate cleanup. The fail-closed + # ``pid_is_hermes`` guard is for BARE pids from state files or process + # scans, where recycling is real. try: - live_start_time = expected_start_time - if live_start_time is None: - live_start_time = _process_start_time(proc.pid) - if live_start_time is None: - allowed = False - else: - allowed = pid_is_hermes( - proc.pid, - expected_start_time=live_start_time, - ) - if allowed: - subprocess.run( - ["taskkill", "/T", "/F", "/PID", str(proc.pid)], + subprocess.run( + ["taskkill", "/T", "/F", "/PID", str(proc.pid)], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, stdin=subprocess.DEVNULL, @@ -600,10 +616,7 @@ def bounded_probe_run( # Timeout OR any other communicate() failure (torn-down pipe, decode # error): terminate the child + descendants and drain bounded. Leaving # it running would leak the same suspended-descendant class this guards. - kill_process_tree( - proc, - expected_start_time=_process_start_time(proc.pid), - ) + kill_process_tree(proc) try: proc.communicate(timeout=1) except Exception: diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 96ebcaa11f..392c2e1e21 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -2153,6 +2153,14 @@ def kill_gateway_processes( try: expected_start_time = None if force: + # Re-verify at kill time, not just scan time: the cmdline + # match inside find_gateway_pids() is stale by the time we + # get here, and a recycled PID could otherwise be tree-killed + # (#89614 class). _capture_gateway_argv re-reads the LIVE + # cmdline and returns None for anything that no longer looks + # like a gateway — refuse those. + if _capture_gateway_argv(pid) is None: + continue from gateway.status import get_process_start_time expected_start_time = get_process_start_time(pid) @@ -2355,6 +2363,20 @@ def _reap_unsupervised_gateway_orphans(extra_exclude: set | None = None) -> bool if not orphans: return False + # Pin each orphan's identity NOW: the cmdline scan above matched at + # scan-time only, and the SIGKILL escalation below fires seconds later. + # A PID recycled inside that window must never be force-killed (#89614 + # class). Fingerprint capture is best-effort — SIGTERM below proceeds + # regardless (it targets the process verified by the scan an instant + # ago), but the delayed SIGKILL requires a still-matching fingerprint. + from gateway.status import get_process_start_time + + orphan_identity: dict[int, int] = {} + for pid in orphans: + start = get_process_start_time(pid) + if start is not None: + orphan_identity[pid] = start + reaped = False for pid in orphans: try: @@ -2374,7 +2396,16 @@ def _reap_unsupervised_gateway_orphans(extra_exclude: set | None = None) -> bool # running until a follow-up SIGKILL — wait, then force-kill any survivor # so the replacement can bind the port cleanly. survivors = _await_gateway_exit(orphans, pid_exists=_pid_exists) - _force_kill_survivors(survivors) + # Re-verify identity at kill time: the delayed SIGKILL only fires when + # the PID still names the process fingerprinted at scan time (fail-closed + # taskkill class fix — a recycled PID must never be force-killed). + verified_survivors = [] + for pid in survivors: + recorded = orphan_identity.get(pid) + if recorded is None or get_process_start_time(pid) != recorded: + continue + verified_survivors.append(pid) + _force_kill_survivors(verified_survivors) return reaped diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index 282cb7c0cb..2749217a1b 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -100,6 +100,27 @@ def _listener_pids_on_port(port: int) -> list: return pids +def _pid_looks_like_node_bridge(pid: int) -> bool: + """Fail-closed check that *pid* is plausibly a stale node bridge. + + ``_kill_port_process`` discovers PIDs from a netstat/lsof scan of a TCP + port — a bare number naming a *stranger* process (#89614 class: an + unverified scan-time PID force-killed later can be anything, including a + critical system process). Before any kill, require the live process to + actually look like our Baileys bridge: a ``node`` executable. Any + ambiguity (process gone, unreadable cmdline) refuses the kill. + """ + try: + import psutil + + proc = psutil.Process(pid) + name = (proc.name() or "").lower() + cmdline = " ".join(proc.cmdline() or []).lower() + return "node" in name or "node" in cmdline.split(" ", 1)[0] + except Exception: + return False + + def _kill_port_process(port: int) -> None: """Kill any process *listening* on the given TCP port (a stale bridge).""" try: @@ -117,9 +138,23 @@ def _kill_port_process(port: int) -> None: if len(parts) >= 5 and parts[3] == "LISTENING": local_addr = parts[1] if local_addr.endswith(f":{port}"): + try: + pid = int(parts[4]) + except ValueError: + continue + # Never taskkill a bare netstat-scanned PID: verify + # the live process is a node bridge first (fail + # closed). taskkill /F on a mistyped or recycled PID + # is unrecoverable. + if pid <= 0 or not _pid_looks_like_node_bridge(pid): + logger.warning( + "[whatsapp] Not killing PID %s on port %d: " + "process is not a node bridge (or identity " + "unverifiable)", pid, port) + continue try: subprocess.run( - ["taskkill", "/PID", parts[4], "/F"], + ["taskkill", "/PID", str(pid), "/F"], capture_output=True, timeout=5, creationflags=windows_hide_flags(), ) @@ -130,6 +165,11 @@ def _kill_port_process(port: int) -> None: # whose connection happens to involve this port number (a browser # tab on a local dev server, etc.) must never be killed. for pid in _listener_pids_on_port(port): + if not _pid_looks_like_node_bridge(pid): + logger.warning( + "[whatsapp] Not killing PID %s on port %d: process is " + "not a node bridge (or identity unverifiable)", pid, port) + continue try: os.kill(pid, signal.SIGTERM) except (ProcessLookupError, PermissionError, OSError): diff --git a/tests/gateway/test_whatsapp_connect.py b/tests/gateway/test_whatsapp_connect.py index 8ce6687b81..daf2b3345a 100644 --- a/tests/gateway/test_whatsapp_connect.py +++ b/tests/gateway/test_whatsapp_connect.py @@ -368,6 +368,8 @@ class TestKillPortProcess: kills = [] with patch("plugins.platforms.whatsapp.adapter._listener_pids_on_port", return_value=[55555]) as mock_listeners, \ + patch("plugins.platforms.whatsapp.adapter._pid_looks_like_node_bridge", + return_value=True), \ patch("plugins.platforms.whatsapp.adapter.os.kill", side_effect=lambda pid, sig: kills.append((pid, sig))): wa._kill_port_process(3000) @@ -375,6 +377,22 @@ class TestKillPortProcess: mock_listeners.assert_called_once_with(3000) assert kills == [(55555, signal.SIGTERM)] + @pytest.mark.linux_only + def test_non_bridge_listener_is_never_killed(self): + """#89614 class: a listener that is not a node bridge is refused.""" + from plugins.platforms.whatsapp import adapter as wa + + kills = [] + with patch("plugins.platforms.whatsapp.adapter._listener_pids_on_port", + return_value=[55555]), \ + patch("plugins.platforms.whatsapp.adapter._pid_looks_like_node_bridge", + return_value=False), \ + patch("plugins.platforms.whatsapp.adapter.os.kill", + side_effect=lambda pid, sig: kills.append((pid, sig))): + wa._kill_port_process(3000) + + assert kills == [] + # --------------------------------------------------------------------------- # Persistent HTTP session lifecycle diff --git a/tests/hermes_cli/test_gateway.py b/tests/hermes_cli/test_gateway.py index f62eb86eb5..5defbb01cd 100644 --- a/tests/hermes_cli/test_gateway.py +++ b/tests/hermes_cli/test_gateway.py @@ -493,6 +493,11 @@ class TestWaitForGatewayExit: calls = [] monkeypatch.setattr(gateway, "find_gateway_pids", lambda exclude_pids=None, all_profiles=False: [11, 22]) + # Kill-time re-verification: force-kills only proceed when the LIVE + # cmdline still looks like a gateway. + monkeypatch.setattr( + gateway, "_capture_gateway_argv", lambda pid: ["python", "-m", "hermes_cli.main", "gateway", "run"] + ) monkeypatch.setattr( gateway, "terminate_pid", @@ -504,6 +509,27 @@ class TestWaitForGatewayExit: assert killed == 2 assert calls == [(11, True), (22, True)] + def test_kill_gateway_processes_force_refuses_recycled_pid(self, monkeypatch): + """A scanned PID whose live argv no longer looks like a gateway is skipped.""" + calls = [] + + monkeypatch.setattr(gateway, "find_gateway_pids", lambda exclude_pids=None, all_profiles=False: [11, 22]) + monkeypatch.setattr( + gateway, + "_capture_gateway_argv", + lambda pid: None if pid == 11 else ["python", "-m", "hermes_cli.main", "gateway", "run"], + ) + monkeypatch.setattr( + gateway, + "terminate_pid", + lambda pid, force=False, **kwargs: calls.append((pid, force)), + ) + + killed = gateway.kill_gateway_processes(force=True) + + assert killed == 1 + assert calls == [(22, True)] + class TestStopProfileGateway: def test_stop_profile_gateway_keeps_pid_file_when_process_still_running(self, monkeypatch): diff --git a/tests/hermes_cli/test_stale_pid_guard.py b/tests/hermes_cli/test_stale_pid_guard.py index e73de38bd4..3323eedb26 100644 --- a/tests/hermes_cli/test_stale_pid_guard.py +++ b/tests/hermes_cli/test_stale_pid_guard.py @@ -35,6 +35,27 @@ class TestPidIsHermes: with mock.patch.object(_subprocess_compat, "IS_WINDOWS", False): assert _subprocess_compat.pid_is_hermes(1234) is True + def test_non_windows_still_rejects_recycled_identity(self): + # An explicit fingerprint mismatch is a recycled PID on any platform. + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", False), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=456 + ): + assert _subprocess_compat.pid_is_hermes( + 1234, expected_start_time=123 + ) is False + + def test_hermes_match_requires_token_boundary(self): + # "hermes" buried inside an unrelated path segment must not match. + assert _subprocess_compat._text_names_hermes( + r"c:\users\shermesa\app.exe" + ) is False + assert _subprocess_compat._text_names_hermes( + r"C:\Users\x\.hermes-runtime\python.exe -m hermes_cli.main" + ) is True + assert _subprocess_compat._text_names_hermes( + "/opt/hermes-agent/venv/bin/python" + ) is True + def test_invalid_pid_inputs_do_not_crash(self): with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True): assert _subprocess_compat.pid_is_hermes(-1) is False @@ -93,35 +114,24 @@ class TestPidIsHermes: class TestKillProcessTree: - """kill_process_tree must never taskkill a PID the probe rejects.""" + """kill_process_tree operates on our own retained Popen handle. + + A retained handle pins the PID (the child cannot be reaped while the + handle is open), so PID recycling is impossible there and the identity + guard deliberately does NOT apply — it could only false-refuse a + legitimate cleanup. These tests pin that contract for the legacy + Windows fallback path. + """ def _proc(self, pid=4321): return mock.Mock(pid=pid) - def test_foreign_pid_never_taskkilled(self): + def test_retained_handle_is_taskkilled_without_probe(self): with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( - _subprocess_compat, "pid_is_hermes", return_value=False - ) as guard, mock.patch.object( - _subprocess_compat, "_process_start_time", return_value=123 - ), mock.patch.object(_subprocess_compat.subprocess, "run") as run: - _subprocess_compat.kill_process_tree(self._proc()) - guard.assert_called_once_with(4321, expected_start_time=123) - run.assert_not_called() - - def test_probe_error_never_taskkilled(self): - with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( - _subprocess_compat, "pid_is_hermes", return_value=False - ), mock.patch.object(_subprocess_compat.subprocess, "run") as run: - _subprocess_compat.kill_process_tree(self._proc()) - run.assert_not_called() - - def test_hermes_pid_still_taskkilled(self): - with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( - _subprocess_compat, "pid_is_hermes", return_value=True - ), mock.patch.object( - _subprocess_compat, "_process_start_time", return_value=123 - ), mock.patch.object(_subprocess_compat.subprocess, "run") as run: - _subprocess_compat.kill_process_tree(self._proc()) + _subprocess_compat, "pid_is_hermes" + ) as guard, mock.patch.object(_subprocess_compat.subprocess, "run") as run: + _subprocess_compat._legacy_kill_process_tree(self._proc()) + guard.assert_not_called() run.assert_called_once() argv = run.call_args.args[0] assert argv[0] == "taskkill" diff --git a/tests/hermes_cli/test_taskkill_identity_windows_live.py b/tests/hermes_cli/test_taskkill_identity_windows_live.py new file mode 100644 index 0000000000..24321b62b3 --- /dev/null +++ b/tests/hermes_cli/test_taskkill_identity_windows_live.py @@ -0,0 +1,192 @@ +# -*- coding: utf-8 -*- +"""Live Windows probes for the fail-closed taskkill process-identity guard. + +Runs only on real Windows (the on-demand ``wine2e/**`` windows-latest lane). +These tests spawn REAL processes and drive the REAL guard code against the +live process table — the coverage the mocked Linux suites cannot provide. + +Class under test (#98814 / #89614): + +- ``gateway.status.terminate_pid(force=True)`` requires a matching + ``expected_start_time`` and must refuse (never taskkill) on a missing or + mismatched identity. +- ``hermes_cli._subprocess_compat.pid_is_hermes`` fails closed on foreign + processes and identity mismatches. +- ``hermes_cli.update_cmd._refuse_gateway_ancestor_tree_kill`` refuses to + nominate any ancestor of the current process for a tree-kill. +""" +import os +import subprocess +import sys +import time + +import pytest + +pytestmark = pytest.mark.skipif( + sys.platform != "win32", reason="live taskkill-identity probes are Windows-only" +) + + +def _spawn_sleeper(seconds: int = 60) -> subprocess.Popen: + return subprocess.Popen( + [sys.executable, "-c", f"import time; time.sleep({seconds})"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + ) + + +def _cleanup(proc: subprocess.Popen) -> None: + try: + proc.kill() + except OSError: + pass + try: + proc.wait(timeout=10) + except Exception: + pass + + +class TestTerminatePidIdentityLive: + def test_matching_identity_kills_real_process(self): + from gateway.status import get_process_start_time, terminate_pid + + proc = _spawn_sleeper() + try: + start = get_process_start_time(proc.pid) + assert start is not None, "live process must have a fingerprint" + terminate_pid(proc.pid, force=True, expected_start_time=start) + assert proc.wait(timeout=15) is not None + finally: + _cleanup(proc) + + def test_missing_expectation_refuses_and_process_survives(self): + from gateway.status import terminate_pid + + proc = _spawn_sleeper() + try: + with pytest.raises(OSError, match="start-time guard"): + terminate_pid(proc.pid, force=True) + assert proc.poll() is None, "refusal must leave the process running" + finally: + _cleanup(proc) + + def test_mismatched_identity_refuses_and_process_survives(self): + """The recycled-PID scenario: recorded identity != live identity.""" + from gateway.status import get_process_start_time, terminate_pid + + # Simulate recycling: capture the identity of a process that then + # dies, and respawn a DIFFERENT process. We can't force Windows to + # hand back the same PID, so assert the guard refuses when the stale + # fingerprint is presented against the new (different) live process. + victim = _spawn_sleeper(1) + stale_start = get_process_start_time(victim.pid) + victim.wait(timeout=30) + + impostor = _spawn_sleeper() + try: + live_start = get_process_start_time(impostor.pid) + assert live_start is not None + if stale_start == live_start: + pytest.skip("fingerprints collided; cannot express mismatch") + with pytest.raises(OSError, match="identity"): + terminate_pid( + impostor.pid, force=True, expected_start_time=stale_start + ) + assert impostor.poll() is None, "mismatch must never kill" + finally: + _cleanup(impostor) + + def test_dead_pid_identity_unavailable_refuses(self): + from gateway.status import get_process_start_time, terminate_pid + + proc = _spawn_sleeper(1) + pid = proc.pid + start = get_process_start_time(pid) + proc.wait(timeout=30) + # Give the OS a beat to drop the process object. + time.sleep(0.5) + if get_process_start_time(pid) == start: + pytest.skip("PID instantly recycled onto identical fingerprint") + with pytest.raises(OSError): + terminate_pid(pid, force=True, expected_start_time=start) + + +class TestPidIsHermesLive: + def test_foreign_real_process_is_refused(self): + """A live non-Hermes process (bare python sleeper in a temp-ish argv) + must never be judged safe for taskkill.""" + from hermes_cli._subprocess_compat import pid_is_hermes + + proc = _spawn_sleeper() + try: + # sys.executable in CI lives under a uv/hostedtoolcache path with + # no 'hermes' token; if the checkout path itself contains one this + # assertion is environment-dependent, so guard for it. + if "hermes" in sys.executable.lower(): + pytest.skip("interpreter path names hermes; probe would match") + assert pid_is_hermes(proc.pid) is False + finally: + _cleanup(proc) + + def test_stale_fingerprint_is_refused_even_for_hermes_argv(self): + from gateway.status import get_process_start_time + from hermes_cli._subprocess_compat import pid_is_hermes + + proc = _spawn_sleeper() + try: + live = get_process_start_time(proc.pid) + assert live is not None + assert ( + pid_is_hermes(proc.pid, expected_start_time=live + 12345) is False + ) + finally: + _cleanup(proc) + + def test_nonexistent_pid_is_refused(self): + from hermes_cli._subprocess_compat import pid_is_hermes + + assert pid_is_hermes(2**24) is False + + +class TestAncestorRefusalLive: + def test_real_parent_chain_is_refused(self, capsys): + """Walk the REAL psutil parent chain: every ancestor of this test + process must be refused as a tree-kill target (#98814).""" + import psutil + + from hermes_cli.gateway import _is_pid_ancestor_of_current_process + from hermes_cli.update_cmd import _refuse_gateway_ancestor_tree_kill + + ancestors = [os.getpid()] + parent = psutil.Process(os.getpid()).parent() + while parent is not None and len(ancestors) < 6: + ancestors.append(parent.pid) + parent = parent.parent() + + for pid in ancestors: + assert _is_pid_ancestor_of_current_process(pid) is True, pid + + refused = _refuse_gateway_ancestor_tree_kill( + ancestors, gateway_mode=False + ) + assert refused is True + out = capsys.readouterr().out + assert "taskkill /T" in out + assert "separate terminal" in out + + def test_unrelated_live_process_is_not_refused(self): + from hermes_cli.gateway import _is_pid_ancestor_of_current_process + from hermes_cli.update_cmd import _refuse_gateway_ancestor_tree_kill + + proc = _spawn_sleeper() + try: + assert _is_pid_ancestor_of_current_process(proc.pid) is False + assert ( + _refuse_gateway_ancestor_tree_kill( + [proc.pid], gateway_mode=False + ) + is False + ) + finally: + _cleanup(proc) diff --git a/tests/tools/test_browser_orphan_reaper.py b/tests/tools/test_browser_orphan_reaper.py index 33033c18a9..4ca34d3dea 100644 --- a/tests/tools/test_browser_orphan_reaper.py +++ b/tests/tools/test_browser_orphan_reaper.py @@ -79,10 +79,11 @@ class TestReapOrphanedBrowserSessions: terminate_calls = [] - def mock_terminate(pid): + def mock_terminate(pid, expected_start=None): terminate_calls.append(pid) with patch("gateway.status._pid_exists", return_value=True), \ + patch("gateway.status.get_process_start_time", return_value=777), \ patch("tools.browser_tool._verify_reapable_browser_daemon", return_value=True), \ patch("tools.process_registry.ProcessRegistry._terminate_host_pid", side_effect=mock_terminate): _reap_orphaned_browser_sessions() @@ -90,6 +91,28 @@ class TestReapOrphanedBrowserSessions: assert 12345 in terminate_calls assert not d.exists() + def test_unfingerprintable_daemon_is_refused(self, fake_tmpdir): + """No start-time fingerprint -> the kill is refused (fail closed). + + The reaper reads the PID from a world-writable temp dir; a PID whose + identity cannot be pinned could be recycled between the verify and the + tree-kill, so it must be left alone (and the socket dir kept for a + later sweep). + """ + from tools.browser_tool import _reap_orphaned_browser_sessions + + _make_socket_dir(fake_tmpdir, "h_perm7654321", pid=12345) + terminate_calls = [] + + with patch("gateway.status._pid_exists", return_value=True), \ + patch("gateway.status.get_process_start_time", return_value=None), \ + patch("tools.browser_tool._verify_reapable_browser_daemon", return_value=True), \ + patch("tools.process_registry.ProcessRegistry._terminate_host_pid", + side_effect=lambda pid, expected_start=None: terminate_calls.append(pid)): + _reap_orphaned_browser_sessions() + + assert terminate_calls == [] + def test_corrupt_pid_file_is_cleaned(self, fake_tmpdir): """PID file with non-integer content is cleaned up.""" @@ -446,9 +469,10 @@ class TestLeakedDaemonWithLiveOwner: kill_calls = [] with patch("gateway.status._pid_exists", return_value=True), \ + patch("gateway.status.get_process_start_time", return_value=777), \ patch("tools.browser_tool._verify_reapable_browser_daemon", return_value=True), \ patch("tools.process_registry.ProcessRegistry._terminate_host_pid", - side_effect=kill_calls.append): + side_effect=lambda pid, expected_start=None: kill_calls.append(pid)): _reap_orphaned_browser_sessions() assert 12345 in kill_calls diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 560fd937dd..da04ca1c47 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -2674,8 +2674,19 @@ def _reap_orphaned_browser_sessions(): # Use the process-tree termination helper so Chromium children # (renderer, GPU, etc.) are cleaned up, not just the daemon parent. try: + from gateway.status import get_process_start_time from tools.process_registry import ProcessRegistry - ProcessRegistry._terminate_host_pid(daemon_pid) + daemon_start = get_process_start_time(daemon_pid) + if daemon_start is None: + # Identity can't be fingerprinted — the verify above matched, + # but without a start time _terminate_host_pid cannot rule out + # a recycle between verify and kill. Refuse; a later sweep + # retries once the process table settles. + logger.warning( + "Refusing to reap browser daemon PID %d (session %s): " + "no start-time fingerprint available", daemon_pid, session_name) + continue + ProcessRegistry._terminate_host_pid(daemon_pid, daemon_start) logger.info("Reaped orphaned browser daemon PID %d (session %s)", daemon_pid, session_name) reaped += 1 @@ -5908,8 +5919,27 @@ def _cleanup_single_browser_session(task_id: str) -> None: try: from tools.process_registry import ProcessRegistry daemon_pid = int(Path(pid_file).read_text(encoding="utf-8").strip()) - ProcessRegistry._terminate_host_pid(daemon_pid) - logger.debug("Killed daemon pid %s for %s", daemon_pid, session_name) + # The .pid file lives in a world-writable temp dir and + # PIDs recycle: verify this really is our daemon for + # this session before tree-killing, and pin the + # identity with a start-time fingerprint so the kill + # refuses if the PID is swapped between check and kill. + if _verify_reapable_browser_daemon( + daemon_pid, socket_dir, session_name): + from gateway.status import get_process_start_time + daemon_start = get_process_start_time(daemon_pid) + if daemon_start is not None: + ProcessRegistry._terminate_host_pid( + daemon_pid, daemon_start) + logger.debug("Killed daemon pid %s for %s", daemon_pid, session_name) + else: + logger.debug( + "Skipped daemon kill for %s: no start-time " + "fingerprint for pid %s", session_name, daemon_pid) + else: + logger.debug( + "Skipped daemon kill for %s: pid %s failed identity " + "verification", session_name, daemon_pid) except (ProcessLookupError, ValueError, PermissionError, OSError): logger.debug("Could not kill daemon pid for %s (already dead or inaccessible)", session_name) shutil.rmtree(socket_dir, ignore_errors=True)