Skip to content

Commit e26e9d0

Browse files
committed
Address review round 2: checkpoint reject, thread-safe pid/ports, drop dead binding
- checkpoint(): a popen() Process owns the handle, so checkpoint (SIGSTOP + ptrace freeze) now rejects with _reject_if_popen() instead of reaching past the Process for a handle that is None on the sandbox — consistent with wait/pause/resume/kill. - Sandbox.pid / Sandbox.ports: delegate to the Process (Process.pid is lock-guarded + cached; new Process.ports() reads the handle under the lock and reports {} while a wait() holds it). Previously both did a raw FFI read of the Process's handle via _live_handle(), racing a concurrent wait() free. Extracted _read_port_mappings() shared by both. - Removed the now-unused sandlock_handle_kill ctypes binding: Process.kill signals the group by pid (killpg), so the FFI kill has no caller. Tests: checkpoint() rejects a popen Process; Sandbox.pid/ports delegate to the live Process and return None/{} after it is reaped.
1 parent 3ac2df6 commit e26e9d0

3 files changed

Lines changed: 62 additions & 21 deletions

File tree

python/src/sandlock/_sdk.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -333,11 +333,6 @@ def confine(policy: "PolicyDataclass") -> None:
333333
ctypes.POINTER(ctypes.c_int), # out_stderr_fd
334334
]
335335

336-
# SIGKILL the handle's process group without freeing it (so wait can still
337-
# collect the exit status). 0 on success, -1 on error.
338-
_lib.sandlock_handle_kill.restype = ctypes.c_int
339-
_lib.sandlock_handle_kill.argtypes = [_c_handle_p]
340-
341336
# Result
342337
_lib.sandlock_result_exit_code.restype = ctypes.c_int
343338
_lib.sandlock_result_exit_code.argtypes = [_c_result_p]

python/src/sandlock/sandbox.py

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -560,11 +560,15 @@ def __exit__(self, exc_type, exc_val, exc_tb):
560560
def pid(self) -> int | None:
561561
"""Child PID while running, None otherwise. Reflects a running child from
562562
either the sandbox lifecycle or a live :meth:`popen` :class:`Process`."""
563-
handle = self._live_handle()
564-
if handle is None:
563+
proc = self._popen_process()
564+
if proc is not None:
565+
# The Process owns the handle; its pid is lock-guarded and cached, so
566+
# reading it can't race a concurrent wait() freeing the handle.
567+
return proc.pid
568+
if self._handle is None:
565569
return None
566570
from ._sdk import _lib
567-
return _lib.sandlock_handle_pid(handle) or None
571+
return _lib.sandlock_handle_pid(self._handle) or None
568572

569573
@property
570574
def is_running(self) -> bool:
@@ -1207,19 +1211,14 @@ def ports(self) -> dict[int, int]:
12071211
or no ports have been remapped. Works for a running child from either
12081212
the lifecycle or a live :meth:`popen` :class:`Process`.
12091213
"""
1210-
handle = self._live_handle()
1211-
if handle is None:
1212-
return {}
1213-
from ._sdk import _lib
1214-
c_str = _lib.sandlock_handle_port_mappings(handle)
1215-
if not c_str:
1214+
proc = self._popen_process()
1215+
if proc is not None:
1216+
# The Process owns the handle; its ports() reads it under the lock so
1217+
# this can't race a concurrent wait() freeing it.
1218+
return proc.ports()
1219+
if self._handle is None:
12161220
return {}
1217-
try:
1218-
import json
1219-
raw = json.loads(c_str.decode())
1220-
return {int(k): v for k, v in raw.items()}
1221-
finally:
1222-
_lib.sandlock_string_free(c_str)
1221+
return _read_port_mappings(self._handle)
12231222

12241223
def checkpoint(
12251224
self,
@@ -1244,6 +1243,10 @@ def checkpoint(
12441243
"""
12451244
from ._sdk import _lib, Checkpoint
12461245

1246+
# A popen() Process owns its handle; checkpoint (SIGSTOP + ptrace freeze)
1247+
# would fight the streaming Process for it, so reject rather than reach
1248+
# past it — consistent with wait/pause/resume/kill.
1249+
self._reject_if_popen()
12471250
if self._handle is None:
12481251
raise RuntimeError("sandbox is not running (use start() or run() first)")
12491252
ptr = _lib.sandlock_handle_checkpoint(self._handle)
@@ -1255,6 +1258,22 @@ def checkpoint(
12551258
return cp
12561259

12571260

1261+
def _read_port_mappings(handle) -> dict[int, int]:
1262+
"""Decode {virtual: real} port mappings for a live handle. Shared by
1263+
``Sandbox.ports`` and ``Process.ports``; the caller must hold whatever lock
1264+
guards ``handle`` while invoking this."""
1265+
from ._sdk import _lib
1266+
c_str = _lib.sandlock_handle_port_mappings(handle)
1267+
if not c_str:
1268+
return {}
1269+
try:
1270+
import json
1271+
raw = json.loads(c_str.decode())
1272+
return {int(k): v for k, v in raw.items()}
1273+
finally:
1274+
_lib.sandlock_string_free(c_str)
1275+
1276+
12581277
def _encode(s: str) -> bytes:
12591278
"""Encode a string to UTF-8 bytes, rejecting NUL bytes."""
12601279
if isinstance(s, str):
@@ -1394,6 +1413,15 @@ def pid(self) -> int | None:
13941413
with self._lock:
13951414
return self._pid if self._handle is not None and self._pid > 0 else None
13961415

1416+
def ports(self) -> dict[int, int]:
1417+
"""Current virtual→real port mappings while running, else ``{}`` (needs
1418+
``port_remap``). Reads the handle under the lock, and reports ``{}``
1419+
while a :meth:`wait` holds it, so it can't race the wait's free."""
1420+
with self._lock:
1421+
if self._handle is None or self._waiting:
1422+
return {}
1423+
return _read_port_mappings(self._handle)
1424+
13971425
def kill(self) -> None:
13981426
"""SIGKILL the child's entire process group by pid (like the sandbox and
13991427
the Go binding), so a kill from another thread is safe while :meth:`wait`

python/tests/test_popen.py

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,8 @@ def test_sandbox_lifecycle_methods_reject_popen_process():
277277
sandbox = _policy()
278278
proc = sandbox.popen(["sleep", "100"])
279279
try:
280-
for op in (sandbox.wait, sandbox.kill, sandbox.pause, sandbox.resume):
280+
for op in (sandbox.wait, sandbox.kill, sandbox.pause, sandbox.resume,
281+
sandbox.checkpoint):
281282
with pytest.raises(RuntimeError):
282283
op()
283284
# The Process still works after the rejected sandbox-level calls.
@@ -287,6 +288,23 @@ def test_sandbox_lifecycle_methods_reject_popen_process():
287288
proc.wait()
288289

289290

291+
def test_sandbox_pid_and_ports_delegate_to_popen_process():
292+
# Sandbox.pid/ports must read the popen child through the Process (whose
293+
# accessors are lock-guarded), not touch the Process's handle directly —
294+
# otherwise a concurrent wait() free would race the raw FFI read.
295+
sandbox = _policy()
296+
proc = sandbox.popen(["sleep", "100"])
297+
try:
298+
assert proc.pid is not None
299+
assert sandbox.pid == proc.pid
300+
assert sandbox.ports() == proc.ports() # both {} without port_remap
301+
finally:
302+
proc.kill()
303+
proc.wait()
304+
assert sandbox.pid is None
305+
assert sandbox.ports() == {}
306+
307+
290308
def test_wait_timeout_zero_does_not_wait_forever():
291309
# timeout=0 (and sub-millisecond values) must not collapse to "wait forever";
292310
# they return promptly with a non-success (killed) result. Watchdog so a

0 commit comments

Comments
 (0)