From f97b0fd7fb5fd22a0351bec4d03f6100aa208cd2 Mon Sep 17 00:00:00 2001 From: gebilaowang404 <61959045+gebilaowang404@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:18:04 +0800 Subject: [PATCH] fix(hermes_cli): fail-closed PID-ownership guard before Windows taskkill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard every Windows `taskkill /PID` against stale/recycled PIDs (#89614: 8x 0xEF blue screens; a rebooted PID can be svchost.exe). Adopted the community patch by AlexMnrs (commit 0162465): shared psutil-based (pid, create_time) guard reusing the repo's existing get_process_start_time machinery: - fail closed on invalid/unknown/recycled identities (0/-1/None/bool/non-int) - capture identity at discovery, re-validate at kill time - all three sites through pid_is_hermes; taskkill stays hidden Sites: _subprocess_compat.kill_process_tree, dashboard_procs._kill_stale_dashboard_processes (win32), update_cmd._stop_process_trees. Refs #90471, #89614 Co-authored-by: Alex Monrás --- hermes_cli/_subprocess_compat.py | 86 +++++++- hermes_cli/dashboard_procs.py | 28 ++- hermes_cli/update_cmd.py | 57 ++++- tests/hermes_cli/test_stale_pid_guard.py | 205 ++++++++++++++++++ .../test_update_orphan_backend_reap.py | 14 +- .../hermes_cli/test_update_stale_dashboard.py | 12 +- 6 files changed, 381 insertions(+), 21 deletions(-) create mode 100644 tests/hermes_cli/test_stale_pid_guard.py diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 8a27dd3fca1c..3e91655b4a97 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -46,6 +46,7 @@ "bounded_git_probe", "bounded_probe_run", "noninteractive_git_env", + "pid_is_hermes", ] @@ -387,7 +388,70 @@ def noninteractive_git_env( # ----------------------------------------------------------------------------- -def kill_process_tree(proc: "subprocess.Popen") -> None: + +def _process_start_time(pid: int) -> int | None: + """Return the repository's stable process-start fingerprint, if available.""" + try: + from gateway.status import get_process_start_time + + return get_process_start_time(pid) + except Exception: + return None + + +def _process_command_is_hermes(pid: int) -> bool: + """Best-effort check that *pid* currently runs Hermes code.""" + try: + import psutil + + process = psutil.Process(pid) + command = " ".join(process.cmdline() or []) + executable = process.exe() or "" + return "hermes" in f"{command} {executable}".lower() + except Exception: + return False + + +def pid_is_hermes( + pid: int, + *, + expected_start_time: int | None = None, +) -> bool: + """Return whether it is safe to use ``taskkill`` for *pid*. + + The PID must be valid, currently exist, and identify a Hermes process. When + 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. + """ + if not isinstance(pid, int) or isinstance(pid, bool) or pid <= 0: + return False + if not IS_WINDOWS: + return True + + try: + current_start_time = _process_start_time(pid) + except Exception: + return False + if current_start_time is None: + return False + if ( + expected_start_time is not None + and current_start_time != expected_start_time + ): + return False + try: + return _process_command_is_hermes(pid) + except Exception: + return False + + +def kill_process_tree( + proc: "subprocess.Popen", + *, + expected_start_time: int | None = None, +) -> None: """Best-effort terminate *proc* and its descendants on both platforms. ``proc.kill()`` alone only terminates the direct child. On Windows a @@ -430,8 +494,19 @@ def kill_process_tree(proc: "subprocess.Popen") -> None: pass if IS_WINDOWS: try: - subprocess.run( - ["taskkill", "/T", "/F", "/PID", str(proc.pid)], + 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)], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, stdin=subprocess.DEVNULL, @@ -495,7 +570,10 @@ 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) + kill_process_tree( + proc, + expected_start_time=_process_start_time(proc.pid), + ) try: proc.communicate(timeout=1) except Exception: diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index 74a950db8b0e..90b53d41288e 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -400,13 +400,37 @@ def _kill_stale_dashboard_processes( failed: list[tuple[int, str]] = [] if sys.platform == "win32": + from gateway.status import get_process_start_time + from hermes_cli._subprocess_compat import pid_is_hermes, windows_hide_flags + + # Capture the identity immediately after discovery. A PID that is + # reused before the destructive action will fail the start-time check. + pid_start_times = { + pid: get_process_start_time(pid) + for pid in pids + } for pid in pids: try: + expected_start_time = pid_start_times.get(pid) + if expected_start_time is None: + failed.append((pid, "could not verify process identity")) + continue + if not pid_is_hermes( + pid, + expected_start_time=expected_start_time, + ): + failed.append((pid, "not hermes-owned or process identity changed")) + continue result = subprocess.run( ["taskkill", "/PID", str(pid), "/F"], - capture_output=True, - text=True, encoding="utf-8", errors="replace", + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.DEVNULL, + text=True, + encoding="utf-8", + errors="replace", timeout=10, + creationflags=windows_hide_flags(), ) if result.returncode == 0: killed.append(pid) diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 600b934d29c3..7404e82dfd04 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -4504,7 +4504,7 @@ def _leftover_pausable_gateway_pids( def _orphaned_desktop_backend_pids( matches: list[tuple[int, str, str]], -) -> list[int] | None: +) -> list[tuple[int, int]] | None: """PIDs from *matches* when every remaining holder is an ORPHANED backend. The venv-holder guard refuses on the Desktop app's ``serve`` backend by @@ -4552,7 +4552,7 @@ def _is_backend(argv_low: str) -> bool: ) # Pass 1: find orphaned backend ROOTS among the holders. - roots: list[int] = [] + roots: list[tuple[int, int]] = [] remaining: list[tuple[int, str]] = [] # (pid, argv_low) still to justify for pid, _name, cmdline in matches: argv = cmdline @@ -4570,6 +4570,19 @@ def _is_backend(argv_low: str) -> bool: continue try: proc = psutil.Process(int(pid)) + from gateway.status import get_process_start_time + + process_start_time = get_process_start_time(int(pid)) + if process_start_time is None: + return None + except psutil.NoSuchProcess: + # The candidate itself exited during classification; there is + # nothing left to reap and no identity to pass to taskkill. + continue + except Exception: + return None + + try: ppid = proc.ppid() parent = psutil.Process(ppid) if ppid else None if parent is not None and parent.is_running(): @@ -4588,12 +4601,12 @@ def _is_backend(argv_low: str) -> bool: pass # parent gone → orphan except Exception: return None - roots.append(int(pid)) + roots.append((int(pid), process_start_time)) # Pass 2: every non-backend holder must be a descendant of an accepted # orphan root — then it dies with the root's tree reap. Anything else # (operator REPL, stray script) keeps the refusal. - root_set = set(roots) + root_set = {pid for pid, _start_time in roots} for pid, _low in remaining: if not root_set: return None @@ -4723,7 +4736,9 @@ def _is_backend(argv_low: str) -> bool: return roots or None -def _stop_process_trees(pids: list[int]) -> None: +def _stop_process_trees( + pids: list[int] | list[tuple[int, int]], +) -> None: """Force-stop each PID with its full child tree (Windows). ``taskkill /T /F`` mirrors the Desktop's ``forceKillProcessTree`` and @@ -4731,12 +4746,38 @@ def _stop_process_trees(pids: list[int]) -> None: ``.hermes-runtime`` interpreter child alive and holding the install open (#70026). Best effort; never raises. """ - for pid in pids: + from gateway.status import get_process_start_time + from hermes_cli._subprocess_compat import pid_is_hermes, windows_hide_flags + + for entry in pids: + if isinstance(entry, tuple): + pid, expected_start_time = entry + else: + pid = int(entry) + expected_start_time = get_process_start_time(pid) try: + if expected_start_time is None: + logger.debug( + "Skipping taskkill of PID %s: process identity unavailable", + pid, + ) + continue + if not pid_is_hermes( + pid, + expected_start_time=expected_start_time, + ): + logger.debug( + "Skipping taskkill of non-Hermes or changed PID %s", + pid, + ) + continue subprocess.run( - ["taskkill", "/PID", str(int(pid)), "/T", "/F"], + ["taskkill", "/PID", str(pid), "/T", "/F"], check=False, - capture_output=True, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + creationflags=windows_hide_flags(), ) except Exception as exc: logger.debug("Could not stop process tree %s: %s", pid, exc) diff --git a/tests/hermes_cli/test_stale_pid_guard.py b/tests/hermes_cli/test_stale_pid_guard.py new file mode 100644 index 000000000000..e73de38bd42d --- /dev/null +++ b/tests/hermes_cli/test_stale_pid_guard.py @@ -0,0 +1,205 @@ +# -*- coding: utf-8 -*- +"""Regression tests for the fail-closed PID-ownership guard. + +Refs #90471 / #89614. The three patched Windows ``taskkill`` boundaries: + +- ``hermes_cli/_subprocess_compat.pid_is_hermes`` / ``kill_process_tree`` +- ``hermes_cli/dashboard_procs._kill_stale_dashboard_processes`` (win32) +- ``hermes_cli/update_cmd._stop_process_trees`` + +Acceptance from #90471: +1. missing / unreadable / non-matching identity fails closed -> no taskkill +2. a recycled or foreign PID control process remains untouched +3. probe failure or timeout is never converted into permission to kill +""" +import subprocess +import sys +from unittest import mock + +import pytest + +from hermes_cli import _subprocess_compat +from hermes_cli import dashboard_procs +from hermes_cli import update_cmd + + +def _probe_stdout(value: str) -> mock.Mock: + return mock.Mock(stdout=value) + + +class TestPidIsHermes: + """The shared identity probe must fail closed on every ambiguity.""" + + def test_non_windows_is_unconditional_pass(self): + # Non-Windows callers have no taskkill path at all. + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", False): + assert _subprocess_compat.pid_is_hermes(1234) 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 + assert _subprocess_compat.pid_is_hermes(0) is False + assert _subprocess_compat.pid_is_hermes("not-a-pid") is False + assert _subprocess_compat.pid_is_hermes(True) is False + + def test_probe_matches_hermes_like_process(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=123 + ), mock.patch.object( + _subprocess_compat, "_process_command_is_hermes", return_value=True + ): + assert _subprocess_compat.pid_is_hermes(1234) is True + + def test_probe_rejects_recycled_process_identity(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=456 + ), mock.patch.object( + _subprocess_compat, "_process_command_is_hermes", return_value=True + ): + assert _subprocess_compat.pid_is_hermes( + 1234, expected_start_time=123 + ) is False + + def test_probe_rejects_foreign_process(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=123 + ), mock.patch.object( + _subprocess_compat, "_process_command_is_hermes", return_value=False + ): + assert _subprocess_compat.pid_is_hermes(1234) is False + + def test_probe_blank_stdout_fails_closed(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=None + ): + assert _subprocess_compat.pid_is_hermes(1234) is False + + def test_probe_timeout_fails_closed(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", return_value=None + ): + assert _subprocess_compat.pid_is_hermes(1234) is False + + def test_probe_oserror_fails_closed(self): + with mock.patch.object(_subprocess_compat, "IS_WINDOWS", True), mock.patch.object( + _subprocess_compat, "_process_start_time", side_effect=OSError("broken pipe") + ): + assert _subprocess_compat.pid_is_hermes(1234) is False + + @pytest.mark.skipif(sys.platform != "win32", reason="real probe is windows-only") + def test_missing_pid_real_probe_fails_closed(self): + # A PID that cannot exist must never be judged Hermes-owned. + assert _subprocess_compat.pid_is_hermes(2**24) is False + + +class TestKillProcessTree: + """kill_process_tree must never taskkill a PID the probe rejects.""" + + def _proc(self, pid=4321): + return mock.Mock(pid=pid) + + def test_foreign_pid_never_taskkilled(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()) + run.assert_called_once() + argv = run.call_args.args[0] + assert argv[0] == "taskkill" + assert "/PID" in argv + assert str(4321) in argv + + +class TestStopProcessTrees: + """update_cmd._stop_process_trees guard behaviour.""" + + def test_foreign_pids_only_probed(self): + with mock.patch( + "gateway.status.get_process_start_time", return_value=123 + ), mock.patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=False + ), mock.patch.object(update_cmd.subprocess, "run") as run: + update_cmd._stop_process_trees([1111, 2222]) + run.assert_not_called() + + def test_hermes_pid_probed_then_taskkilled(self): + with mock.patch( + "gateway.status.get_process_start_time", return_value=123 + ), mock.patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=True + ), mock.patch.object( + update_cmd.subprocess, "run", return_value=mock.Mock(returncode=0) + ) as run: + update_cmd._stop_process_trees([1111]) + assert len(run.call_args_list) == 1 + assert run.call_args.args[0][0] == "taskkill" + + def test_probe_timeout_skips_taskkill(self): + with mock.patch( + "gateway.status.get_process_start_time", return_value=123 + ), mock.patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=False + ), mock.patch.object(update_cmd.subprocess, "run") as run: + update_cmd._stop_process_trees([1111, 2222]) # must not raise + run.assert_not_called() + + +class TestKillStaleDashboardProcesses: + """dashboard_procs win32 kill branch guard behaviour.""" + + def _fake_m(self, pids=(12345,)): + m = mock.Mock() + m._find_stale_dashboard_pids.return_value = list(pids) + return m + + def test_foreign_pid_reported_not_killed(self): + with mock.patch.object(dashboard_procs, "_m", return_value=self._fake_m()), mock.patch.object( + dashboard_procs.sys, "platform", "win32" + ), mock.patch( + "gateway.status.get_process_start_time", return_value=123 + ), mock.patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=False + ), mock.patch.object(dashboard_procs.subprocess, "run") as run: + result = dashboard_procs._kill_stale_dashboard_processes() + assert result["killed"] == [] + assert result["failed"] == [ + (12345, "not hermes-owned or process identity changed") + ] + run.assert_not_called() + + def test_hermes_pid_killed(self): + with mock.patch.object(dashboard_procs, "_m", return_value=self._fake_m()), mock.patch.object( + dashboard_procs.sys, "platform", "win32" + ), mock.patch( + "gateway.status.get_process_start_time", return_value=123 + ), mock.patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=True + ), mock.patch.object( + dashboard_procs.subprocess, "run", return_value=mock.Mock( + returncode=0, stderr="", stdout="" + ) + ) as run: + result = dashboard_procs._kill_stale_dashboard_processes() + taskkill_calls = [c for c in run.call_args_list if c.args[0][0] == "taskkill"] + assert len(taskkill_calls) == 1 + assert result["killed"] == [12345] + assert result["failed"] == [] diff --git a/tests/hermes_cli/test_update_orphan_backend_reap.py b/tests/hermes_cli/test_update_orphan_backend_reap.py index baf2399d5c0a..fa1a3da0885b 100644 --- a/tests/hermes_cli/test_update_orphan_backend_reap.py +++ b/tests/hermes_cli/test_update_orphan_backend_reap.py @@ -84,7 +84,7 @@ def test_orphan_backend_dead_parent_qualifies(): backend = _proc(200, _SERVE_ARGV, ppid=999) # 999 not in table → dead fake = _fake_psutil({200: backend}) with patch.dict(sys.modules, {"psutil": fake}): - assert cli_main._orphaned_desktop_backend_pids(_holders()) == [200] + assert cli_main._orphaned_desktop_backend_pids(_holders()) == [(200, 10000)] def test_backend_with_live_parent_keeps_refusal(): @@ -101,7 +101,7 @@ def test_recycled_parent_pid_counts_as_orphan(): backend = _proc(200, _SERVE_ARGV, ppid=50, create_time=100.0) fake = _fake_psutil({50: recycled, 200: backend}) with patch.dict(sys.modules, {"psutil": fake}): - assert cli_main._orphaned_desktop_backend_pids(_holders()) == [200] + assert cli_main._orphaned_desktop_backend_pids(_holders()) == [(200, 10000)] def test_non_backend_holder_keeps_refusal(): @@ -136,7 +136,7 @@ def test_orphan_root_plus_managed_runtime_descendant_qualifies(): fake = _fake_psutil({200: backend, 210: child}) with patch.dict(sys.modules, {"psutil": fake}): holders = _holders() + [(210, "python.exe", " ".join(child_argv))] - assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + assert cli_main._orphaned_desktop_backend_pids(holders) == [(200, 10000)] def test_descendant_of_grandchild_depth_qualifies(): @@ -150,7 +150,7 @@ def test_descendant_of_grandchild_depth_qualifies(): fake = _fake_psutil({200: backend, 210: mid, 220: grand}) with patch.dict(sys.modules, {"psutil": fake}): holders = _holders() + [(220, "python.exe", "python.exe leaf.py")] - assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + assert cli_main._orphaned_desktop_backend_pids(holders) == [(200, 10000)] def test_non_descendant_alongside_orphan_root_keeps_refusal(): @@ -172,7 +172,7 @@ def test_descendant_exited_between_scan_and_classify_is_skipped(): fake = _fake_psutil({200: backend}) # descendant 210 already gone with patch.dict(sys.modules, {"psutil": fake}): holders = _holders() + [(210, "python.exe", "python.exe worker.py")] - assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + assert cli_main._orphaned_desktop_backend_pids(holders) == [(200, 10000)] def test_holder_gone_between_scan_and_classify_is_skipped(): @@ -205,7 +205,9 @@ def _no_psutil(name, *args, **kwargs): def test_stop_process_trees_kills_full_tree(): from hermes_cli import update_cmd - with patch.object(update_cmd.subprocess, "run") as run: + with patch("gateway.status.get_process_start_time", return_value=123), patch( + "hermes_cli._subprocess_compat.pid_is_hermes", return_value=True + ), patch.object(update_cmd.subprocess, "run") as run: cli_main._stop_process_trees([111, 222]) calls = [c.args[0] for c in run.call_args_list] assert calls == [ diff --git a/tests/hermes_cli/test_update_stale_dashboard.py b/tests/hermes_cli/test_update_stale_dashboard.py index 1c8bb0f9d85d..22338524d999 100644 --- a/tests/hermes_cli/test_update_stale_dashboard.py +++ b/tests/hermes_cli/test_update_stale_dashboard.py @@ -108,11 +108,19 @@ def test_self_pid_excluded(self): assert 12345 in pids - def test_ps_timeout_returns_empty(self): + def _assert_ps_timeout_returns_empty(self): import subprocess as sp with patch("subprocess.run", side_effect=sp.TimeoutExpired("ps", 10)): assert _find_stale_dashboard_pids() == [] + @pytest.mark.linux_only + def test_ps_timeout_returns_empty_linux(self): + self._assert_ps_timeout_returns_empty() + + @pytest.mark.macos_only + def test_ps_timeout_returns_empty_macos(self): + self._assert_ps_timeout_returns_empty() + @@ -211,6 +219,8 @@ def fake_run(args, *a, **kw): with patch("hermes_cli.main._find_stale_dashboard_pids", return_value=[12345, 12346]), \ + patch("gateway.status.get_process_start_time", return_value=123), \ + patch("hermes_cli._subprocess_compat.pid_is_hermes", return_value=True), \ patch("subprocess.run", side_effect=fake_run) as mock_run: _kill_stale_dashboard_processes()