sicd: harden pump loop, EPIPE/diagnostic handling, signal fidelity
Review round 3 fixes: - write_all helper: handle partial writes from signals - EPIPE on write side: catch BrokenPipeError (child dead), not all OSError - Read-side OSError: diagnose on stderr + exit 1 (not silent truncation) - parse_netstring: sanity cap 1<<24, return content only (not tuple) - 128+WTERMSIG for signal-killed children (shell convention) - 24/24 pytest green
This commit is contained in:
Markus Fritsche
+25
-38
@@ -16,37 +16,36 @@ def read_exactly(fd: int, n: int) -> bytes:
|
||||
return bytes(buf)
|
||||
|
||||
|
||||
def parse_netstring(data: bytes) -> tuple[bytes, bytes]:
|
||||
"""Parse a DJB netstring from data. Returns (content, rest)."""
|
||||
def write_all(fd: int, data: bytes) -> None:
|
||||
"""Write all of data to fd, handling partial writes from signals."""
|
||||
offset = 0
|
||||
while offset < len(data):
|
||||
n = os.write(fd, data[offset:])
|
||||
offset += n
|
||||
|
||||
|
||||
def parse_netstring(data: bytes) -> bytes:
|
||||
"""Parse a DJB netstring from data. Returns content or raises ValueError."""
|
||||
colon = data.find(b":")
|
||||
if colon < 0:
|
||||
raise ValueError("netstring missing colon")
|
||||
# Enforce non-canonical lengths: must be all digits
|
||||
if not data[:colon].isdigit():
|
||||
raise ValueError("netstring length not all digits")
|
||||
ns_len = int(data[:colon])
|
||||
# Need at least colon+1 + ns_len + 1 for trailing comma
|
||||
if ns_len > 1 << 24:
|
||||
raise ValueError("netstring length exceeds sanity cap")
|
||||
if len(data) < colon + 1 + ns_len + 1:
|
||||
raise ValueError("netstring truncated")
|
||||
content = data[colon + 1 : colon + 1 + ns_len]
|
||||
if data[colon + 1 + ns_len : colon + 1 + ns_len + 1] != b",":
|
||||
raise ValueError("netstring missing trailing comma")
|
||||
# Require exact consumption: no rest after the comma
|
||||
rest = data[colon + 1 + ns_len + 1 :]
|
||||
if rest:
|
||||
raise ValueError("netstring has trailing data after comma")
|
||||
return content, rest
|
||||
|
||||
|
||||
def write_all(fd: int, data: bytes) -> None:
|
||||
"""Write all bytes to fd, looping on short writes."""
|
||||
while data:
|
||||
n = os.write(fd, data)
|
||||
data = data[n:]
|
||||
return content
|
||||
|
||||
|
||||
def main() -> None:
|
||||
# Read 5-byte preamble: magic + len32
|
||||
try:
|
||||
preamble = read_exactly(sys.stdin.fileno(), 5)
|
||||
except EOFError:
|
||||
@@ -59,7 +58,6 @@ def main() -> None:
|
||||
|
||||
ns_len = struct.unpack(">I", preamble[1:5])[0]
|
||||
|
||||
# Read the netstring
|
||||
try:
|
||||
ns_data = read_exactly(sys.stdin.fileno(), ns_len)
|
||||
except EOFError:
|
||||
@@ -67,12 +65,11 @@ def main() -> None:
|
||||
sys.exit(1)
|
||||
|
||||
try:
|
||||
content, _ = parse_netstring(ns_data)
|
||||
content = parse_netstring(ns_data)
|
||||
except ValueError as e:
|
||||
sys.stderr.write(f"sicd: {e}\n")
|
||||
sys.exit(1)
|
||||
|
||||
# Split at first NUL
|
||||
nul_pos = content.find(b"\x00")
|
||||
if nul_pos < 0:
|
||||
sys.stderr.write("sicd: missing NUL separator between command and payload\n")
|
||||
@@ -81,22 +78,18 @@ def main() -> None:
|
||||
command_bytes = content[:nul_pos]
|
||||
payload = content[nul_pos + 1 :]
|
||||
|
||||
# Decode command, validate ASCII
|
||||
try:
|
||||
command_str = command_bytes.decode("ascii")
|
||||
except UnicodeDecodeError:
|
||||
sys.stderr.write("sicd: command contains non-ASCII bytes\n")
|
||||
sys.exit(1)
|
||||
|
||||
# Split on ASCII space only (not .split())
|
||||
argv = command_str.split(" ")
|
||||
|
||||
# Create pipe for child's stdin
|
||||
r_fd, w_fd = os.pipe()
|
||||
|
||||
pid = os.fork()
|
||||
if pid == 0:
|
||||
# Child
|
||||
os.close(w_fd)
|
||||
os.dup2(r_fd, sys.stdin.fileno())
|
||||
os.close(r_fd)
|
||||
@@ -109,21 +102,11 @@ def main() -> None:
|
||||
sys.stderr.write(f"sicd: exec failed: {e}\n")
|
||||
os._exit(1)
|
||||
else:
|
||||
# Parent
|
||||
os.close(r_fd)
|
||||
# Write payload first
|
||||
try:
|
||||
write_all(w_fd, payload)
|
||||
except BrokenPipeError:
|
||||
# Child died before we could write payload; reap and inherit its status
|
||||
os.close(w_fd)
|
||||
_, status = os.waitpid(pid, 0)
|
||||
if os.WIFEXITED(status):
|
||||
sys.exit(os.WEXITSTATUS(status))
|
||||
else:
|
||||
sys.exit(1)
|
||||
# Then pump remaining stdin to child
|
||||
read_error = False
|
||||
try:
|
||||
if payload:
|
||||
write_all(w_fd, payload)
|
||||
while True:
|
||||
chunk = os.read(sys.stdin.fileno(), 65536)
|
||||
if not chunk:
|
||||
@@ -131,16 +114,20 @@ def main() -> None:
|
||||
write_all(w_fd, chunk)
|
||||
except BrokenPipeError:
|
||||
pass
|
||||
except OSError:
|
||||
pass
|
||||
except OSError as e:
|
||||
sys.stderr.write(f"sicd: stdin read error: {e}\n")
|
||||
read_error = True
|
||||
os.close(w_fd)
|
||||
# Wait for child
|
||||
_, status = os.waitpid(pid, 0)
|
||||
if read_error:
|
||||
sys.exit(1)
|
||||
if os.WIFEXITED(status):
|
||||
sys.exit(os.WEXITSTATUS(status))
|
||||
elif os.WIFSIGNALED(status):
|
||||
sys.exit(128 + os.WTERMSIG(status))
|
||||
else:
|
||||
sys.exit(1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
main()
|
||||
@@ -27,6 +27,7 @@ Semantics each sicd MUST implement:
|
||||
sicd inherits the child's exit status on success paths.
|
||||
"""
|
||||
|
||||
import signal
|
||||
import struct
|
||||
import subprocess
|
||||
import sys
|
||||
@@ -184,6 +185,13 @@ def test_nonexistent_command_exits_nonzero():
|
||||
# signal interruption. Code that ignores os.write's
|
||||
# return value truncates; a write-all loop is
|
||||
# unaffected.
|
||||
# stdin-read-eio : os.read is wrapped so that pump-loop-sized reads
|
||||
# (> 4096 bytes) on sicd's OWN stdin (fd 0) raise
|
||||
# OSError(EIO). Frame parsing (small exact reads)
|
||||
# succeeds; the first pump read then fails the way a
|
||||
# dying tty / revoked fd fails. A bare
|
||||
# `except OSError: pass` turns this into silent
|
||||
# truncation.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
DRIVER = """\
|
||||
@@ -206,6 +214,14 @@ elif mode == "widow-before-write":
|
||||
os.waitid(os.P_PID, pid, os.WEXITED | os.WNOWAIT)
|
||||
return pid
|
||||
os.fork = fork_then_wait_for_child_death
|
||||
elif mode == "stdin-read-eio":
|
||||
import errno
|
||||
real_read = os.read
|
||||
def eio_read(fd, n):
|
||||
if fd == 0 and n > 4096:
|
||||
raise OSError(errno.EIO, "Input/output error")
|
||||
return real_read(fd, n)
|
||||
os.read = eio_read
|
||||
else:
|
||||
raise SystemExit("unknown fault mode: " + mode)
|
||||
|
||||
@@ -266,3 +282,75 @@ def test_partial_write_pump_loop_not_truncated(tmp_path):
|
||||
assert r.stdout == body, (
|
||||
f"pump loop truncated trailing stdin: got {len(r.stdout)} of {len(body)} bytes"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Exit-status fidelity: signal death and read-side errors must be visible.
|
||||
#
|
||||
# Tracked gaps from 7f7f0c2: both waitpid sites collapse WIFSIGNALED to
|
||||
# exit 1, and the pump loop's bare `except OSError: pass` swallows read
|
||||
# errors on sicd's own stdin as silent truncation.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _selfkill_command(tmp_path, signum: int) -> bytes:
|
||||
"""A command (no spaces in any argv element) whose process kills itself
|
||||
with signum before producing any output."""
|
||||
script = tmp_path / f"selfkill_{signum}.py"
|
||||
script.write_text(f"import os\nos.kill(os.getpid(), {signum})\n")
|
||||
return f"{sys.executable} {script}".encode()
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"signum,expected",
|
||||
[
|
||||
pytest.param(signal.SIGKILL, 137, id="SIGKILL-137"),
|
||||
pytest.param(signal.SIGTERM, 143, id="SIGTERM-143"),
|
||||
],
|
||||
)
|
||||
def test_signal_killed_child_exits_128_plus_termsig(tmp_path, signum, expected):
|
||||
"""A child killed by a signal must surface as exit 128+WTERMSIG (shell
|
||||
convention), NOT collapse to exit 1 -- exit 1 is indistinguishable from
|
||||
an ordinary command failure, so the caller cannot tell a crashed/killed
|
||||
command from one that merely returned false."""
|
||||
r = run_sicd(frame(_selfkill_command(tmp_path, signum), b""))
|
||||
assert r.returncode == expected, (
|
||||
f"child died from signal {signum}; expected exit {expected} "
|
||||
f"(128+WTERMSIG), got {r.returncode} -- signal death collapsed "
|
||||
f"into an ordinary failure code"
|
||||
)
|
||||
|
||||
|
||||
def test_signal_killed_child_epipe_path_exits_128_plus_termsig(tmp_path):
|
||||
"""Same contract on the EPIPE recovery path: the child is provably dead
|
||||
from SIGKILL before the parent writes the payload (widowed pipe), and the
|
||||
status inherited there must also be 128+WTERMSIG, not 1."""
|
||||
wire = frame(_selfkill_command(tmp_path, int(signal.SIGKILL)), b"payload for a corpse\n")
|
||||
r = run_sicd_fault(wire, "widow-before-write", tmp_path)
|
||||
assert b"Traceback" not in r.stderr, r.stderr.decode(errors="replace")
|
||||
assert r.returncode == 137, (
|
||||
f"expected exit 137 (128+SIGKILL) via the EPIPE path, got {r.returncode}"
|
||||
)
|
||||
|
||||
|
||||
def test_stdin_read_oserror_is_diagnosed_not_swallowed(tmp_path):
|
||||
"""OSError on reading sicd's OWN stdin (EIO here; EBADF, ENXIO likewise)
|
||||
is not `| head` semantics -- only BrokenPipeError on the write side is.
|
||||
Today `except OSError: pass` silently drops the rest of the stream and
|
||||
exits 0 as if the transfer completed. Required: a diagnostic on stderr
|
||||
(a handled message, not a traceback) and a non-zero exit, so truncation
|
||||
is never reported as success."""
|
||||
payload = b"payload written before the fault\n"
|
||||
trailing = b"trailing bytes the pump loop will never deliver" * 200
|
||||
r = run_sicd_fault(frame(b"cat", payload) + trailing, "stdin-read-eio", tmp_path)
|
||||
assert r.returncode != 0, (
|
||||
"stdin read failed mid-stream (EIO) but sicd exited 0 -- "
|
||||
"silent truncation reported as success"
|
||||
)
|
||||
assert r.stderr.strip() != b"", (
|
||||
"stdin read failed mid-stream but no diagnostic was written to stderr"
|
||||
)
|
||||
assert b"Traceback" not in r.stderr, (
|
||||
f"want a handled diagnostic, not an unhandled exception:\n"
|
||||
f"{r.stderr.decode(errors='replace')}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user