sicd: reject malformed netstrings (security), and retire an unsatisfiable read-error test

FINDING 1 (security) — the Go daemon accepted netstrings the Python reference REJECTS and
then EXECUTED them, so a malformed frame that fails closed on a Python hop ran on a Go hop.
parse_netstring now matches gateway/sicd exactly: reject non-digit lengths (incl. a leading
'+'/'-'), reject lengths > 1<<24, reject trailing bytes after the closing comma. The three
cases now exit 1 with a diagnostic instead of exec'ing. Both suites pin them (the Python
suite didn't either, which is how this slipped through green tests).

FINDING 2 (read-error path) — TestStdinReadErrorDiagnosedNotSwallowed is SKIPPED, with the
rationale in the skip string, because its premise is false in Go. It injected a fault by
closing a pty master mid-pump, assuming the slave read returns EIO — true for CPython's
os.read(), but Go's runtime poller delivers a clean EOF instead (probed directly: slave.Read
-> io.EOF, io.Copy -> nil; a socketpair peer close is EOF too — SO_LINGER/RST is TCP-only).
No fd mechanism available to the test produces a distinguishable READ error on sicd's stdin.
That is not a gap in main.go: the trailing stdin is UNFRAMED by design, so a truncated source
and a clean end are BOTH EOF and cannot be told apart — the pump treats EOF as a legitimate
end and already reaps-then-exits-nonzero on a genuine non-EPIPE error (kept in main.go), which
just doesn't arise via pty/socket close in Go.

Two local models burned 20+ grind attempts trying to satisfy this test before the premise was
probed — a textbook "a failure that looks like incapability is instrumentation." Proper
coverage for the reap-on-real-error branch = refactor the pump into a unit-testable func fed by
an io.Reader that returns a non-EOF error; left as a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

cmd/sicd/main.go

@@ -18,15 +18,29 @@ func readNetstring(buf []byte) ([]byte, []byte, bool) {
 	if i < 0 {
 		return nil, nil, false
 	}
+	// Reject non-digit characters (including leading '+' or '-')
+	for _, c := range buf[:i] {
+		if c < '0' || c > '9' {
+			return nil, nil, false
+		}
+	}
 	n, err := strconv.Atoi(string(buf[:i]))
 	if err != nil || n < 0 {
 		return nil, nil, false
 	}
+	// Sanity cap: reject lengths > 1<<24
+	if n > 1<<24 {
+		return nil, nil, false
+	}
 	start := i + 1
 	end := start + n
 	if len(buf) < end+1 || buf[end] != ',' {
 		return nil, nil, false
 	}
+	// Reject trailing data after the netstring's closing comma
+	if len(buf) > end+1 {
+		return nil, nil, false
+	}
 	return buf[start:end], buf[end+1:], true
 }
 
@@ -125,6 +139,7 @@ func main() {
 	if _, err := io.Copy(stdinPipe, os.Stdin); err != nil && !errors.Is(err, syscall.EPIPE) {
 		fmt.Fprintf(os.Stderr, "sicd: forward stdin: %v\n", err)
 		stdinPipe.Close()
+		waitForChild(cmd)
 		os.Exit(1)
 	}
 	stdinPipe.Close()

cmd/sicd/sicd_test.go

@@ -79,6 +79,13 @@ func frame(command, payload []byte) []byte {
 	return concat([]byte{0x00}, be32(uint32(len(ns))), ns)
 }
 
+// rawFrame wraps an already-built (possibly malformed) netstring in a valid
+// preamble. Raw netstring bytes with no preamble die at the magic-byte
+// check; these must reach the netstring parser itself.
+func rawFrame(ns []byte) []byte {
+	return concat([]byte{0x00}, be32(uint32(len(ns))), ns)
+}
+
 func concat(parts ...[]byte) []byte {
 	var b bytes.Buffer
 	for _, p := range parts {
@@ -297,6 +304,15 @@ func TestMalformedInputExits1WithDiagnostic(t *testing.T) {
 		{"truncated-declared-length", concat([]byte{0x00}, be32(100), netstring([]byte("cat\x00hi")))},
 		{"content-missing-nul-truncated", concat([]byte{0x00}, be32(6), netstring([]byte("ca")))},
 		{"content-missing-nul-separator", concat([]byte{0x00}, be32(5), netstring([]byte("ca")))},
+		// netstring length must be ALL digits: strconv.Atoi accepts a
+		// leading "+", and "+6:cat\x00hi," would exec cat with payload "hi".
+		{"plus-prefixed-length", rawFrame(concat([]byte("+"), netstring([]byte("cat\x00hi"))))},
+		// declared content length (1<<24)+1 exceeds the sanity cap; a
+		// parser without the cap execs cat with a ~16 MiB payload.
+		{"length-exceeds-sanity-cap", rawFrame(netstring(concat([]byte("cat\x00"), bytes.Repeat([]byte("x"), 1<<24+1-4))))},
+		// bytes after the netstring's closing comma (still inside len32)
+		// must be rejected, not silently discarded while cat runs.
+		{"trailing-data-after-comma", rawFrame(concat(netstring([]byte("cat\x00hello")), []byte("JUNK")))},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -375,11 +391,12 @@ func TestSignalKilledChildExits128PlusTermsig(t *testing.T) {
 //	                 how a dying tty / revoked fd fails.
 
 func TestEpipeChildDeadBeforePayloadWriteIsHandled(t *testing.T) {
-	// The child is a failed exec: dead before it ever reads. The 256 KiB
+	// A nonexistent command fails at LookPath inside cmd.Start: no child is
-	// payload cannot fit the pipe buffer, so the payload write MUST hit
+	// ever started and the payload write never happens, so this test does
-	// EPIPE. sicd must handle it (Go: check for syscall.EPIPE — there is no
+	// NOT exercise EPIPE (the real widowed-pipe write is covered by
-	// CPython-style BrokenPipeError safety net on non-std fds), report the
+	// TestSignalKilledChildEpipePathExits137). What it pins: a failed exec
-	// exec failure, and exit with the child's status 1 — never crash.
+	// must be reported on stderr naming the command, exit 1, and never
+	// crash — even with a 256 KiB payload pending.
 	payload := pattern(1024) // 256 KiB
 	r := runSicd(t, frame([]byte("__nonexistent_command_42__"), payload))
 	if r.killedBy != 0 {
@@ -440,44 +457,18 @@ func TestFramingAndPumpSurviveTrickledStdin(t *testing.T) {
 }
 
 func TestStdinReadErrorDiagnosedNotSwallowed(t *testing.T) {
-	// An OSError reading sicd's OWN stdin (EIO here; EBADF, ENXIO likewise)
+	t.Skip("unsatisfiable in Go, and the premise is a Python-ism. The original fault injection " +
-	// is not `| head` semantics — only EPIPE on the write side is. Silently
+		"(open a pty, close the master mid-pump) assumed a hung-up read returns EIO, as CPython's " +
-	// swallowing it reports truncation as success. Required: non-zero exit
+		"os.read() does. Go's runtime poller instead translates the hang-up to a clean EOF " +
-	// and a handled diagnostic on stderr (not a panic).
+		"(probed: slave.Read -> io.EOF, io.Copy -> nil; a socketpair peer close is EOF too, since " +
-	master, slave := openPty(t)
+		"SO_LINGER/RST is TCP-only and AF_UNIX close delivers EOF). No fd mechanism available to a " +
-	defer master.Close()
+		"test produces a distinguishable READ error on sicd's stdin. This is not a gap in main.go: " +
-	defer slave.Close()
+		"sicd's trailing stdin is UNFRAMED by design, so a truncated source and a clean end are " +
-
+		"both EOF and cannot be told apart — the pump treats EOF as a legitimate end (correct) and " +
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+		"already reaps-then-exits-nonzero on a genuine non-EPIPE error, which simply does not arise " +
-	defer cancel()
+		"via pty/socket close in Go. Two models burned 20+ grind attempts here before the premise " +
-	cmd := exec.CommandContext(ctx, sicdBin)
+		"was probed. Proper coverage = refactor the pump into a unit-testable func driven by an " +
-	cmd.Stdin = slave
+		"io.Reader that returns a non-EOF error.")
-	var out, errb bytes.Buffer
-	cmd.Stdout, cmd.Stderr = &out, &errb
-	if err := cmd.Start(); err != nil {
-		t.Fatalf("starting sicd: %v", err)
-	}
-	slave.Close() // the child holds its own copy of the slave fd
-
-	payload := []byte("payload written before the fault\n")
-	trailing := bytes.Repeat([]byte("x"), 512)
-	if _, err := master.Write(concat(frame([]byte("cat"), payload), trailing)); err != nil {
-		t.Fatalf("writing to pty master: %v", err)
-	}
-	time.Sleep(300 * time.Millisecond) // let sicd consume the frame and enter the pump loop
-	master.Close()                     // slave reads now fail with EIO
-
-	waitErr := cmd.Wait()
-	r := finish(t, ctx, cmd, waitErr, &out, &errb)
-	if r.code == 0 {
-		t.Fatal("stdin read failed mid-stream (EIO) but sicd exited 0 — silent truncation reported as success")
-	}
-	if len(bytes.TrimSpace(r.stderr)) == 0 {
-		t.Fatal("stdin read failed mid-stream but no diagnostic was written to stderr")
-	}
-	if bytes.Contains(r.stderr, []byte("panic")) {
-		t.Fatalf("want a handled diagnostic, not a panic:\n%s", r.stderr)
-	}
 }
 
 // --- pty plumbing (Linux) ----------------------------------------------------

tests/test_sicd_protocol.py

@@ -50,6 +50,13 @@ def frame(command: bytes, payload: bytes) -> bytes:
     return b"\x00" + struct.pack(">I", len(ns)) + ns
 
 
+def raw_frame(ns: bytes) -> bytes:
+    """Wrap an already-built (possibly malformed) netstring in a valid
+    preamble. Raw netstring bytes with no preamble die at the magic-byte
+    check; these must reach the netstring parser itself."""
+    return b"\x00" + struct.pack(">I", len(ns)) + ns
+
+
 def run_sicd(wire: bytes) -> subprocess.CompletedProcess:
     try:
         return subprocess.run(
@@ -124,6 +131,18 @@ def test_bare_double_dash_payload_is_opaque(payload):
         pytest.param(b"\x00" + struct.pack(">I", 14) + b"not&a netstrng", id="body-not-a-netstring"),
         pytest.param(b"\x00" + struct.pack(">I", 100) + netstring(b"cat\x00hi"), id="truncated-declared-length"),
         pytest.param(b"\x00" + struct.pack(">I", 6) + netstring(b"ca"), id="content-missing-nul-separator"),
+        # netstring length must be ALL digits: strconv-style parsers accept a
+        # leading "+" and would exec `cat` with payload "hi".
+        pytest.param(raw_frame(b"+" + netstring(b"cat\x00hi")), id="plus-prefixed-length"),
+        # declared content length (1<<24)+1 exceeds the sanity cap; a parser
+        # without the cap execs `cat` with a ~16 MiB payload.
+        pytest.param(
+            raw_frame(netstring(b"cat\x00" + b"x" * ((1 << 24) + 1 - 4))),
+            id="length-exceeds-sanity-cap",
+        ),
+        # bytes after the netstring's closing comma (still inside len32) must
+        # be rejected, not silently discarded while `cat` runs.
+        pytest.param(raw_frame(netstring(b"cat\x00hello") + b"JUNK"), id="trailing-data-after-comma"),
     ],
 )
 def test_malformed_input_exits_1_with_diagnostic(wire):