Skip to content

Commit bed191e

Browse files
authored
fix: repair TCP port-remapping slow path
Fixes #2. Four bugs in the TCP port-remapping slow path: 1. _allocate_real_port held socket blocked the child's bind — close immediately 2. os.pidfd_open not available in some Python builds — use _pidfd_open from _context 3. fixup_getsockname leaked the duplicated fd — move os.close into finally block 4. NotifSupervisor.stop() never called port_map.close() — add cleanup Also adds two slow-path tests (host-holds-port and concurrent sandboxes) and tightens /proc/net/tcp assertions from >= 1 to == 1. Signed-off-by: Vahagn <wanghang25@mails.tsinghua.edu.cn>
1 parent 03124be commit bed191e

File tree

4 files changed

+87
-22
lines changed

4 files changed

+87
-22
lines changed

src/sandlock/_context.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,15 +125,15 @@ def _notif_syscall_names(notif: "NotifPolicy") -> list[str]:
125125
from ._seccomp import _SYSCALL_NR
126126
names = []
127127

128-
# openat/open needed when features require path inspection.
128+
# openat/open only needed when features require path inspection
129129
needs_openat = (
130130
notif is not None and (
131131
notif.rules # path-based virtualization (e.g. /etc/hosts)
132-
or notif.isolate_pids # block /proc/<foreign_pid> access
133132
or notif.cow_enabled # COW filesystem redirects
134133
or notif.random_seed is not None # deterministic /dev/urandom
135134
or notif.time_start is not None # /proc/uptime, /proc/stat virtualization
136135
or notif.port_remap # /proc/net/* filtering
136+
or notif.isolate_pids # /proc/<pid> access control
137137
)
138138
)
139139
if needs_openat:

src/sandlock/_notif.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,9 @@ def stop(self) -> None:
528528
pass
529529
self._mem_fd = -1
530530
self._mem_fd_pid = -1
531+
if self._port_map is not None:
532+
self._port_map.close()
533+
self._port_map = None
531534

532535
def _check_disk_quota(self) -> None:
533536
"""Check if overlay upper dir exceeds disk quota."""

src/sandlock/_port_remap.py

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,6 @@ class PortMap:
4747
_lock: threading.Lock = field(default_factory=threading.Lock, repr=False)
4848
_virtual_to_real: dict[int, int] = field(default_factory=dict, repr=False)
4949
_real_to_virtual: dict[int, int] = field(default_factory=dict, repr=False)
50-
# Sockets held open to keep real ports reserved
51-
_held_sockets: list[socket.socket] = field(default_factory=list, repr=False)
5250
# Proxy state
5351
_proxy_threads: list[threading.Thread] = field(default_factory=list, repr=False)
5452
_proxy_sockets: list[socket.socket] = field(default_factory=list, repr=False)
@@ -96,7 +94,7 @@ def virtual_port(self, real: int) -> int | None:
9694
return self._real_to_virtual.get(real)
9795

9896
def close(self) -> None:
99-
"""Release all held sockets, stop proxies."""
97+
"""Stop proxies and release all state."""
10098
self._proxy_stop.set()
10199
for s in self._proxy_sockets:
102100
try:
@@ -106,12 +104,6 @@ def close(self) -> None:
106104
for t in self._proxy_threads:
107105
t.join(timeout=2.0)
108106
with self._lock:
109-
for s in self._held_sockets:
110-
try:
111-
s.close()
112-
except OSError:
113-
pass
114-
self._held_sockets.clear()
115107
self._virtual_to_real.clear()
116108
self._real_to_virtual.clear()
117109
self._proxy_sockets.clear()
@@ -138,17 +130,16 @@ def _try_reserve_port(self, port: int, family: int) -> int | None:
138130

139131
def _allocate_real_port(self, family: int) -> int | None:
140132
"""Bind a socket to port 0 to get a free port from the kernel."""
133+
af = socket.AF_INET6 if family == _AF_INET6 else socket.AF_INET
134+
addr = "::1" if af == socket.AF_INET6 else "127.0.0.1"
135+
s = socket.socket(af, socket.SOCK_STREAM)
141136
try:
142-
af = socket.AF_INET6 if family == _AF_INET6 else socket.AF_INET
143-
s = socket.socket(af, socket.SOCK_STREAM)
144-
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
145-
addr = "::1" if af == socket.AF_INET6 else "127.0.0.1"
146137
s.bind((addr, 0))
147-
real_port = s.getsockname()[1]
148-
self._held_sockets.append(s)
149-
return real_port
138+
return s.getsockname()[1]
150139
except OSError:
151140
return None
141+
finally:
142+
s.close()
152143

153144
def _start_proxy(self, virtual: int, real: int, family: int) -> None:
154145
"""Start a TCP proxy: listen on virtual port, forward to real port."""
@@ -363,8 +354,9 @@ def fixup_getsockname(pid: int, sockaddr_addr: int, addrlen_addr: int,
363354
from ._procfs import write_bytes
364355

365356
# Duplicate the child's socket fd via pidfd_getfd syscall
357+
from ._context import _pidfd_open
366358
try:
367-
pidfd = os.pidfd_open(pid)
359+
pidfd = _pidfd_open(pid)
368360
except OSError:
369361
return False
370362

@@ -387,8 +379,8 @@ def fixup_getsockname(pid: int, sockaddr_addr: int, addrlen_addr: int,
387379
family = s.family
388380
finally:
389381
s.detach()
382+
os.close(local_fd)
390383
except OSError:
391-
os.close(local_fd)
392384
return False
393385

394386
if family not in (socket.AF_INET, socket.AF_INET6):

tests/test_sandbox.py

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
"""Tests for sandlock.sandbox.Sandbox."""
33

44
import os
5+
import socket
56
import sys
67
from unittest.mock import patch, MagicMock
78

@@ -322,7 +323,7 @@ def test_proc_net_tcp_shows_own_port_only(self):
322323
result = Sandbox(policy).run(["python3", "-c", code])
323324

324325
assert result.success
325-
assert int(result.stdout.strip()) >= 1
326+
assert int(result.stdout.strip()) == 1
326327

327328
def test_proc_net_tcp_hides_host_ports(self):
328329
"""/proc/net/tcp hides host ports (e.g. sshd on port 22)."""
@@ -372,7 +373,7 @@ def test_proc_net_tcp6_filtered(self):
372373
result = Sandbox(policy).run(["python3", "-c", code])
373374

374375
assert result.success
375-
assert int(result.stdout.strip()) >= 1
376+
assert int(result.stdout.strip()) == 1
376377

377378
def test_tcp_sendmsg_2mb_with_port_remap(self):
378379
"""TCP sendmsg() with 2 MB payload works correctly under port remap."""
@@ -443,6 +444,75 @@ def test_tcp_sendmsg_2mb_with_port_remap(self):
443444
assert data["data_ok"] is True
444445

445446

447+
def test_slow_path_host_holds_virtual_port(self):
448+
"""Slow path: host process holds TCP virtual port, sandbox must remap."""
449+
import socket as _socket
450+
code = (
451+
"import socket; "
452+
"s = socket.socket(socket.AF_INET, socket.SOCK_STREAM); "
453+
"s.bind(('127.0.0.1', 8080)); "
454+
"print(s.getsockname()[1]); "
455+
"s.close()"
456+
)
457+
policy = Policy(port_remap=True)
458+
459+
holder = _socket.socket(_socket.AF_INET, _socket.SOCK_STREAM)
460+
holder.setsockopt(_socket.SOL_SOCKET, _socket.SO_REUSEADDR, 1)
461+
holder.bind(("127.0.0.1", 8080))
462+
try:
463+
result = Sandbox(policy).run(["python3", "-c", code])
464+
finally:
465+
holder.close()
466+
467+
assert result.success, f"Sandbox failed: {result.stderr}"
468+
assert result.stdout.strip() == b"8080"
469+
470+
def test_slow_path_two_concurrent_sandboxes(self):
471+
"""Slow path: two concurrent sandboxes both bind the same virtual TCP port."""
472+
import threading
473+
code_hold = (
474+
"import socket, time; "
475+
"s = socket.socket(socket.AF_INET, socket.SOCK_STREAM); "
476+
"s.bind(('127.0.0.1', 8080)); "
477+
"print(s.getsockname()[1], flush=True); "
478+
"time.sleep(3); "
479+
"s.close()"
480+
)
481+
code_fast = (
482+
"import socket; "
483+
"s = socket.socket(socket.AF_INET, socket.SOCK_STREAM); "
484+
"s.bind(('127.0.0.1', 8080)); "
485+
"print(s.getsockname()[1]); "
486+
"s.close()"
487+
)
488+
policy = Policy(port_remap=True)
489+
results = [None, None]
490+
491+
def run(i, code):
492+
results[i] = Sandbox(policy).run(["python3", "-c", code])
493+
494+
t1 = threading.Thread(target=run, args=(0, code_hold))
495+
t1.start()
496+
import time
497+
for _ in range(50): # wait for sandbox 1 to bind port 8080
498+
probe = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
499+
try:
500+
probe.bind(("127.0.0.1", 8080))
501+
probe.close()
502+
time.sleep(0.1)
503+
except OSError:
504+
break # sandbox 1 is ready
505+
t2 = threading.Thread(target=run, args=(1, code_fast))
506+
t2.start()
507+
t1.join()
508+
t2.join()
509+
510+
r1, r2 = results
511+
assert r1.success, f"Sandbox 1 failed: {r1.stderr}"
512+
assert r2.success, f"Sandbox 2 failed: {r2.stderr}"
513+
assert r1.stdout.strip() == b"8080"
514+
assert r2.stdout.strip() == b"8080"
515+
446516
class TestCpuThrottle:
447517
"""Test SIGSTOP/SIGCONT CPU throttling."""
448518

0 commit comments

Comments
 (0)