diff --git a/routes/shell_routes.py b/routes/shell_routes.py index 6953748c4..6a1c0f583 100644 --- a/routes/shell_routes.py +++ b/routes/shell_routes.py @@ -695,7 +695,7 @@ def _session_pgid(pid: int) -> int | None: return pgid -def _signal_session(pgid: int | None, pid: int, sig: int) -> bool: +def _signal_session(pgid: int | None, pid: int | None, sig: int) -> bool: """Send ``sig`` to the whole process group, or to the lone process. Returns whether anything was signalled, so a caller can tell "the session @@ -721,6 +721,27 @@ def _session_alive(pgid: int | None, pid: int) -> bool: return pid_alive(pid) +def _bind_pty_spawn_identity(proc) -> None: + """Freeze the PTY leader's identity and its session group at spawn. + + Called immediately after the spawn, while the pid is known to be the child + just created: we hold it unreaped, so the slot cannot have been reissued. + The group is recorded only when it is the leader's own (``setsid`` + applied: pgid == pid) and the identity still verifies after reading it. + Teardown works from this record alone and never re-derives ownership + from ``proc.pid``, which outlives the process it named. + """ + pid = getattr(proc, "pid", None) + if not pid: + return + identity = process_lifecycle.ProcessIdentity.capture(pid) + pgid = _session_pgid(pid) + if pgid != pid or identity.verdict() != process_lifecycle.OWNED: + pgid = None # No safe session group: teardown reaches the child alone. + proc._ody_pty_identity = process_lifecycle.ProcessIdentity( + pid=identity.pid, start_token=identity.start_token, pgid=pgid) + + async def _terminate_pty_session(proc) -> bool: """Kill the PTY child and every process in the session it leads. @@ -732,6 +753,18 @@ async def _terminate_pty_session(proc) -> bool: outlives the grace period, and return whether the session is actually gone so the caller can say so rather than assume it. + Ownership is the identity frozen at spawn (:func:`_bind_pty_spawn_identity`), + re-verified before every signal: + + * leader OWNED and still leading the recorded group → signal the group; + * leader GONE (exited and reaped) → the recorded group only, never the + pid: a group id is not reissued while the group lives, so a present + group with no process in its leader's slot is still ours; + * leader FOREIGN → the pid was reissued, which proves our group's + lifetime had already ended; nothing of ours is left to signal; + * leader UNVERIFIABLE, or no spawn identity at all → nothing is + signalled and the session is not reported gone. + The ladder itself is :func:`src.process_lifecycle.escalate_async`; the leader is reaped through ``proc.wait()`` inside each window, otherwise its own zombie keeps the group alive and the probe can never come back clean. @@ -739,15 +772,41 @@ async def _terminate_pty_session(proc) -> bool: pid = getattr(proc, "pid", None) if pid is None: return True - pgid = _session_pgid(pid) + frozen = getattr(proc, "_ody_pty_identity", None) + if frozen is None: + logger.warning("PTY teardown for pid %s has no spawn identity; not signalling", pid) + return False + pgid = frozen.pgid + + def _gone() -> bool: + verdict = frozen.verdict() + if verdict == process_lifecycle.FOREIGN: + return True + if verdict == process_lifecycle.UNVERIFIABLE: + return False + if pgid is not None: + return not _session_alive(pgid, frozen.pid) + return verdict == process_lifecycle.GONE or process_lifecycle.is_zombie(frozen.pid) + + def _send(sig) -> bool: + verdict = frozen.verdict() + if verdict == process_lifecycle.OWNED: + if pgid is not None and process_lifecycle.pgid_of(frozen.pid) == pgid: + return _signal_session(pgid, frozen.pid, sig) + # Child-only: no safe group, or the leader no longer leads it. + return _signal_session(None, frozen.pid, sig) + if verdict == process_lifecycle.GONE and pgid is not None: + # Never fall back to the pid: it names no process of ours now. + return _signal_session(pgid, None, sig) + return False async def _reap_leader(): if proc.returncode is None: await proc.wait() result = await process_lifecycle.escalate_async( - lambda: not _session_alive(pgid, pid), - lambda sig: _signal_session(pgid, pid, sig), + _gone, + _send, steps=tuple((sig, PTY_KILL_GRACE) for sig in PTY_KILL_ESCALATION), wait=_reap_leader, poll_s=PTY_KILL_POLL_INTERVAL, @@ -801,6 +860,7 @@ async def _generate_pty(cmd: str, timeout: int, request: Request): cwd=str(Path.home()), preexec_fn=os.setsid, ) + _bind_pty_spawn_identity(proc) os.close(slave_fd) # parent doesn't need the slave side deadline = (loop.time() + timeout) if timeout else None diff --git a/tests/test_shell_routes.py b/tests/test_shell_routes.py index e763560ed..e71ec1aeb 100644 --- a/tests/test_shell_routes.py +++ b/tests/test_shell_routes.py @@ -119,13 +119,31 @@ pty_session = pytest.mark.skipif( async def _spawn_pty_style_session(script: str): - """Spawn `script` the way _generate_pty does: its own session via setsid.""" - return await asyncio.create_subprocess_shell( + """Spawn `script` the way _generate_pty does: its own session via setsid, + with the leader's identity and group bound at spawn.""" + import routes.shell_routes as shell_routes + + proc = await asyncio.create_subprocess_shell( script, stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.DEVNULL, preexec_fn=os.setsid, ) + shell_routes._bind_pty_spawn_identity(proc) + return proc + + +def _bound_fake_leader(monkeypatch, pid=4242): + """A fake PTY leader whose spawn identity verifies and still leads its group.""" + from src import process_lifecycle, process_ownership + + real_getpgid = os.getpgid + monkeypatch.setattr(process_ownership, "verify", lambda p, token: process_ownership.OWNED) + monkeypatch.setattr(os, "getpgid", lambda p: pid if p == pid else real_getpgid(p)) + return SimpleNamespace( + pid=pid, returncode=0, wait=None, + _ody_pty_identity=process_lifecycle.ProcessIdentity(pid, "spawn-token", pgid=pid), + ) def _stubborn_child(pid_file: Path, ignore: tuple[str, ...]) -> str: @@ -281,9 +299,8 @@ async def test_terminate_pty_session_reports_a_session_it_could_not_kill( monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) monkeypatch.setattr(shell_routes, "_signal_session", lambda *_: True) monkeypatch.setattr(shell_routes, "_session_alive", lambda *_: True) - monkeypatch.setattr(shell_routes, "_session_pgid", lambda _: 4242) - proc = SimpleNamespace(pid=4242, returncode=0, wait=None) + proc = _bound_fake_leader(monkeypatch) assert await shell_routes._terminate_pty_session(proc) is False @@ -293,7 +310,6 @@ async def test_terminate_pty_session_escalates_before_giving_up(monkeypatch): sent = [] monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) - monkeypatch.setattr(shell_routes, "_session_pgid", lambda _: 4242) monkeypatch.setattr(shell_routes, "_session_alive", lambda *_: True) monkeypatch.setattr( shell_routes, @@ -301,7 +317,7 @@ async def test_terminate_pty_session_escalates_before_giving_up(monkeypatch): lambda pgid, pid, sig: sent.append(sig) or True, ) - proc = SimpleNamespace(pid=4242, returncode=0, wait=None) + proc = _bound_fake_leader(monkeypatch) await shell_routes._terminate_pty_session(proc) assert sent == [signal.SIGTERM, signal.SIGKILL] @@ -343,20 +359,60 @@ async def test_terminate_pty_session_never_signals_the_servers_own_group(monkeyp """If setsid did not apply, the child's group is ours: reach the child alone.""" import routes.shell_routes as shell_routes + from src import process_ownership + own = os.getpgid(0) sent = [] monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) monkeypatch.setattr(shell_routes.process_lifecycle, "pgid_of", lambda _pid: own) - monkeypatch.setattr(os, "killpg", lambda pgid, sig: sent.append(("group", pgid, sig))) - monkeypatch.setattr(os, "kill", lambda pid, sig: sent.append(("pid", pid, sig))) + monkeypatch.setattr(process_ownership, "start_token", lambda pid: "the-leader") proc = SimpleNamespace(pid=987654, returncode=0, wait=None) assert shell_routes._session_pgid(proc.pid) is None + shell_routes._bind_pty_spawn_identity(proc) + assert proc._ody_pty_identity.pgid is None # no safe session group recorded + + monkeypatch.setattr(os, "killpg", lambda pgid, sig: sent.append(("group", pgid, sig))) + monkeypatch.setattr(os, "kill", lambda pid, sig: sent.append(("pid", pid, sig))) await shell_routes._terminate_pty_session(proc) assert sent and all(kind == "pid" and target == 987654 for kind, target, _ in sent), sent +@pytest.mark.skipif(os.name == "nt", reason="POSIX process groups") +async def test_terminate_pty_session_never_signals_a_reused_leader_pid(monkeypatch): + """Leader spawned and bound → reaped → pid reissued → teardown signals nothing. + + The replacement is the worst case: an unrelated session leader, so both + its pid and its process group carry the number our leader had. + """ + import routes.shell_routes as shell_routes + from src import process_ownership + + pid = 987650 + real_getpgid = os.getpgid + occupant = {"token": "leader-token"} + monkeypatch.setattr(process_ownership, "start_token", + lambda p: occupant["token"] if int(p) == pid else None) + monkeypatch.setattr(os, "getpgid", lambda p: pid if p == pid else real_getpgid(p)) + + proc = SimpleNamespace(pid=pid, returncode=None, wait=None) + shell_routes._bind_pty_spawn_identity(proc) # spawn time: the leader we just created + assert proc._ody_pty_identity.pgid == pid + + # The leader exits and is reaped; the kernel reissues its pid to a stranger. + proc.returncode = 0 + occupant["token"] = "replacement-token" + signalled = [] + monkeypatch.setattr(os, "killpg", lambda g, sig: sig and signalled.append(("group", g, sig))) + monkeypatch.setattr(os, "kill", lambda p, sig: sig and signalled.append(("pid", p, sig))) + monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) + + await shell_routes._terminate_pty_session(proc) + + assert signalled == [], f"teardown signalled the replacement: {signalled}" + + def test_session_alive_treats_a_refused_probe_as_alive(monkeypatch): """EPERM says the group exists but we may not signal it, not that it died. @@ -391,11 +447,10 @@ async def test_terminate_pty_session_reports_a_group_it_may_not_signal(monkeypat raise PermissionError(errno.EPERM, "Operation not permitted") monkeypatch.setattr(shell_routes, "PTY_KILL_GRACE", 0.01) - monkeypatch.setattr(shell_routes, "_session_pgid", lambda _: 4242) + proc = _bound_fake_leader(monkeypatch) monkeypatch.setattr(shell_routes.os, "killpg", refuse) monkeypatch.setattr(shell_routes.os, "kill", refuse) - proc = SimpleNamespace(pid=4242, returncode=0, wait=None) assert await shell_routes._terminate_pty_session(proc) is False