diff --git a/gateway/sicd b/gateway/sicd index f435697..744b343 100755 --- a/gateway/sicd +++ b/gateway/sicd @@ -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() \ No newline at end of file diff --git a/tests/test_sicd_protocol.py b/tests/test_sicd_protocol.py index 28166c0..3789c30 100644 --- a/tests/test_sicd_protocol.py +++ b/tests/test_sicd_protocol.py @@ -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')}" + )