From 7f7f0c2ecb554d5b36e39105fb12f3904d20ac3b Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 00:39:58 +0200 Subject: [PATCH] sicd: handle EPIPE and short writes at both write sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_all() loops until the full buffer is transferred — os.write's return value was ignored at the payload write and in the pump loop, so a short write silently truncated the stream. EPIPE at the payload write (child already dead) now reaps the child and inherits its status instead of dying with a BrokenPipeError traceback; EPIPE in the pump loop falls through to the same waitpid, which is standard `cmd | head` semantics. Tests use deterministic fault injection rather than racing signals: a fork wrapper that waits with waitid(WNOWAIT) so the child is provably dead but still reapable before the parent writes, and an os.write cap of 7 bytes per call as the stand-in for a signal-interrupted partial write. Reviewed three times. Known gaps, tracked, NOT fixed here: signal-killed children still collapse to exit 1 (no 128+WTERMSIG), and the pump loop's `except OSError: pass` is broader than it should be. Co-Authored-By: Claude Opus 4.8 --- gateway/sicd | 22 +++++++- tests/test_sicd_protocol.py | 101 ++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/gateway/sicd b/gateway/sicd index 53f7e6b..f435697 100755 --- a/gateway/sicd +++ b/gateway/sicd @@ -38,6 +38,13 @@ def parse_netstring(data: bytes) -> tuple[bytes, bytes]: 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:] + + def main() -> None: # Read 5-byte preamble: magic + len32 try: @@ -105,14 +112,25 @@ def main() -> None: # Parent os.close(r_fd) # Write payload first - os.write(w_fd, payload) + 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 try: while True: chunk = os.read(sys.stdin.fileno(), 65536) if not chunk: break - os.write(w_fd, chunk) + write_all(w_fd, chunk) + except BrokenPipeError: + pass except OSError: pass os.close(w_fd) diff --git a/tests/test_sicd_protocol.py b/tests/test_sicd_protocol.py index 55c3af8..28166c0 100644 --- a/tests/test_sicd_protocol.py +++ b/tests/test_sicd_protocol.py @@ -165,3 +165,104 @@ def test_nonexistent_command_exits_nonzero(): """Exec of a command that does not exist must not hang or exit 0.""" r = run_sicd(frame(b"__nonexistent_command_42__", b"")) assert r.returncode != 0, f"expected non-zero exit, got {r.returncode}" + + +# --------------------------------------------------------------------------- +# Fault injection: deterministic EPIPE and short-write coverage. +# +# sicd runs under a tiny driver that patches os primitives *before* executing +# gateway/sicd, instead of racing signals against the pump loop: +# +# widow-before-write : os.fork is wrapped so the parent blocks (waitid with +# WNOWAIT -- the child stays reapable for the later +# waitpid) until the child is dead. Exec'ing a +# nonexistent command therefore guarantees the pipe is +# widowed before the payload write: the EPIPE case, +# made deterministic. +# short-write : os.write is wrapped to transfer at most 7 bytes per +# call -- the kernel's partial-write behaviour on +# signal interruption. Code that ignores os.write's +# return value truncates; a write-all loop is +# unaffected. +# --------------------------------------------------------------------------- + +DRIVER = """\ +import os +import sys + +sicd_path = sys.argv[1] +mode = sys.argv[2] + +if mode == "short-write": + real_write = os.write + def short_write(fd, data): + return real_write(fd, bytes(data)[:7]) + os.write = short_write +elif mode == "widow-before-write": + real_fork = os.fork + def fork_then_wait_for_child_death(*args): + pid = real_fork(*args) + if pid: + os.waitid(os.P_PID, pid, os.WEXITED | os.WNOWAIT) + return pid + os.fork = fork_then_wait_for_child_death +else: + raise SystemExit("unknown fault mode: " + mode) + +sys.argv = [sicd_path] +code = compile(open(sicd_path, "rb").read(), sicd_path, "exec") +exec(code, {"__name__": "__main__", "__file__": sicd_path}) +""" + + +def run_sicd_fault(wire: bytes, mode: str, tmp_path) -> subprocess.CompletedProcess: + driver = tmp_path / "fault_driver.py" + driver.write_text(DRIVER) + try: + return subprocess.run( + [sys.executable, str(driver), SICD, mode], + input=wire, + capture_output=True, + timeout=TIMEOUT, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"sicd hung under fault mode {mode!r}") + + +def test_epipe_child_dead_before_payload_write_no_traceback(tmp_path): + """Child is guaranteed dead (nonexistent command; the wrapped fork waits + for the corpse) before the parent writes the payload: sicd must handle + EPIPE and exit with the child's status -- never dump a BrokenPipeError + traceback on stderr.""" + wire = frame(b"__nonexistent_command_42__", b"payload for a corpse\n") + r = run_sicd_fault(wire, "widow-before-write", tmp_path) + assert b"Traceback" not in r.stderr, ( + f"unhandled exception instead of clean EPIPE handling:\n" + f"{r.stderr.decode(errors='replace')}" + ) + assert b"BrokenPipeError" not in r.stderr + assert r.returncode == 1, f"expected the child's exit status 1, got {r.returncode}" + + +def test_partial_write_payload_not_truncated(tmp_path): + """os.write may transfer fewer bytes than requested; the payload write + must loop on its return value. With every write capped at 7 bytes, cat + must still receive the full 37-byte payload, not just the first chunk.""" + payload = b"ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789\n" + r = run_sicd_fault(frame(b"cat", payload), "short-write", tmp_path) + assert r.returncode == 0, f"expected exit 0, got {r.returncode}, stderr={r.stderr!r}" + assert r.stdout == payload, ( + f"payload truncated by ignored os.write return value: " + f"got {len(r.stdout)} of {len(payload)} bytes" + ) + + +def test_partial_write_pump_loop_not_truncated(tmp_path): + """The same 7-byte write cap applied to the stdin pump loop: an 8 KiB + trailing body must reach the child intact, not 7 bytes per read chunk.""" + body = bytes(range(256)) * 32 # 8 KiB + r = run_sicd_fault(frame(b"cat", b"") + body, "short-write", tmp_path) + assert r.returncode == 0, f"expected exit 0, got {r.returncode}, stderr={r.stderr!r}" + assert r.stdout == body, ( + f"pump loop truncated trailing stdin: got {len(r.stdout)} of {len(body)} bytes" + )