fix(windows): compose the taskkill identity guards into one fail-closed class fix
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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
+32
-1
@@ -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
|
||||
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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)
|
||||
@@ -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
|
||||
|
||||
+33
-3
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user