diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index 9680117..f43044b 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -13,35 +13,35 @@ import ( "syscall" ) -func readNetstring(buf []byte) ([]byte, []byte, bool) { +func readNetstring(buf []byte) ([]byte, bool) { i := bytes.IndexByte(buf, ':') if i < 0 { - return nil, nil, false + return nil, false } // Reject non-digit characters (including leading '+' or '-') for _, c := range buf[:i] { if c < '0' || c > '9' { - return nil, nil, false + return nil, false } } n, err := strconv.Atoi(string(buf[:i])) if err != nil || n < 0 { - return nil, nil, false + return nil, false } // Sanity cap: reject lengths > 1<<24 if n > 1<<24 { - return nil, nil, false + return nil, false } start := i + 1 end := start + n if len(buf) < end+1 || buf[end] != ',' { - return nil, nil, false + return nil, false } // Reject trailing data after the netstring's closing comma if len(buf) > end+1 { - return nil, nil, false + return nil, false } - return buf[start:end], buf[end+1:], true + return buf[start:end], true } func waitForChild(cmd *exec.Cmd) int { @@ -84,7 +84,7 @@ func main() { os.Exit(1) } - content, _, ok := readNetstring(nsBuf) + content, ok := readNetstring(nsBuf) if !ok { fmt.Fprintf(os.Stderr, "sicd: invalid netstring\n") os.Exit(1) @@ -132,19 +132,27 @@ func main() { os.Exit(waitForChild(cmd)) } - // Pump remaining stdin to child. EPIPE on the write side means the - // child stopped reading (| head semantics) — that is not an error. - // Any other error (EIO, etc.) means sicd's own stdin failed mid-stream - // and the transfer was truncated — exit non-zero with a diagnostic. - 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() + os.Exit(pumpStdin(stdinPipe, os.Stdin, func() int { return waitForChild(cmd) }, os.Stderr)) +} - os.Exit(waitForChild(cmd)) +// pumpStdin copies src (sicd's own stdin) into the child's stdin (dst), closes dst so the +// child sees EOF, then reaps. EPIPE from the WRITE side means the child stopped reading +// (| head semantics) — not an error, so the child's own status is returned. Any OTHER error +// (a genuine read/write failure) means the transfer was truncated: report a diagnostic, reap +// to avoid a zombie, and return non-zero REGARDLESS of the child's status — a truncated +// transfer must never be reported as success. Extracted so a mock erroring io.Reader can +// cover this branch: that is exactly how the Python reference tests it (monkeypatching +// os.read to raise OSError), since no real fd on this platform yields a read error on stdin +// (a pty slave and a socket peer both see a clean kernel EOF on hangup, in Go AND CPython). +func pumpStdin(dst io.WriteCloser, src io.Reader, reap func() int, errOut io.Writer) int { + _, err := io.Copy(dst, src) + dst.Close() + if err != nil && !errors.Is(err, syscall.EPIPE) { + fmt.Fprintf(errOut, "sicd: forward stdin: %v\n", err) + reap() + return 1 + } + return reap() } func writeAll(w io.Writer, data []byte) error { diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index 39f32fd..42df498 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -457,18 +457,13 @@ func TestFramingAndPumpSurviveTrickledStdin(t *testing.T) { } func TestStdinReadErrorDiagnosedNotSwallowed(t *testing.T) { - t.Skip("unsatisfiable in Go, and the premise is a Python-ism. The original fault injection " + - "(open a pty, close the master mid-pump) assumed a hung-up read returns EIO, as CPython's " + - "os.read() does. Go's runtime poller instead translates the hang-up to a clean EOF " + - "(probed: slave.Read -> io.EOF, io.Copy -> nil; a socketpair peer close is EOF too, since " + - "SO_LINGER/RST is TCP-only and AF_UNIX close delivers EOF). No fd mechanism available to a " + - "test produces a distinguishable READ error on sicd's stdin. This is not a gap in main.go: " + - "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 " + - "already reaps-then-exits-nonzero on a genuine non-EPIPE error, which simply does not arise " + - "via pty/socket close in Go. Two models burned 20+ grind attempts here before the premise " + - "was probed. Proper coverage = refactor the pump into a unit-testable func driven by an " + - "io.Reader that returns a non-EOF error.") + t.Skip("no real fd yields a read error on sicd's stdin on this platform, so this " + + "integration-style injection is unsatisfiable — a pty slave and a socket peer both see a " + + "clean KERNEL EOF on hangup (probed: raw syscall.Read -> n=0, errno=0), identically in Go " + + "and CPython; EIO-after-hangup is master-side, not slave-side. The Python reference does " + + "NOT use a pty here — it monkeypatches os.read to raise OSError(EIO). The Go equivalent is " + + "a mock erroring io.Reader, which now covers the reap-then-exit-nonzero branch as a UNIT " + + "test (TestPumpStdin*). This black-box test is kept only as a placeholder for that contract.") } // --- pty plumbing (Linux) ---------------------------------------------------- @@ -518,3 +513,81 @@ func openPty(t *testing.T) (master, slave *os.File) { } return master, slave } + +// erroringReader yields data once, then err — the Go translation of the Python reference's +// fault injection (monkeypatching os.read to raise OSError). Lets us cover pumpStdin's +// read-error branch without an fd, since no real fd produces a read error on stdin here. +type erroringReader struct { + data []byte + err error + done bool +} + +func (r *erroringReader) Read(p []byte) (int, error) { + if !r.done && len(r.data) > 0 { + n := copy(p, r.data) + r.data = r.data[n:] + if len(r.data) == 0 { + r.done = true + } + return n, nil + } + return 0, r.err +} + +type errWriteCloser struct { + buf bytes.Buffer + writeErr error // if set, Write returns it (e.g. EPIPE from a widowed pipe) + closed bool +} + +func (w *errWriteCloser) Write(p []byte) (int, error) { + if w.writeErr != nil { + return 0, w.writeErr + } + return w.buf.Write(p) +} +func (w *errWriteCloser) Close() error { w.closed = true; return nil } + +func TestPumpStdinReapsAndExitsNonzeroOnReadError(t *testing.T) { + src := &erroringReader{data: []byte("partial payload"), err: syscall.EIO} + dst := &errWriteCloser{} + reaped := false + var errOut bytes.Buffer + code := pumpStdin(dst, src, func() int { reaped = true; return 0 }, &errOut) + if code != 1 { + t.Fatalf("a genuine read error must exit non-zero regardless of child status, got %d", code) + } + if !reaped { + t.Fatal("the child must still be reaped on a read error (no zombie)") + } + if !dst.closed { + t.Fatal("the child's stdin must be closed so it sees EOF") + } + if !bytes.Contains(errOut.Bytes(), []byte("forward stdin")) { + t.Fatalf("a diagnostic must be written, got: %q", errOut.String()) + } +} + +func TestPumpStdinEpipeIsHeadSemanticsReturnsChildStatus(t *testing.T) { + // EPIPE from the write side = the child stopped reading (`| head`) — NOT an error. + src := &erroringReader{data: []byte("x"), err: io.EOF} // clean src; the write side EPIPEs + dst := &errWriteCloser{writeErr: syscall.EPIPE} + var errOut bytes.Buffer + code := pumpStdin(dst, src, func() int { return 42 }, &errOut) + if code != 42 { + t.Fatalf("EPIPE is head-semantics; must return the child's own status 42, got %d", code) + } + if errOut.Len() != 0 { + t.Fatalf("EPIPE must not emit a diagnostic, got: %q", errOut.String()) + } +} + +func TestPumpStdinCleanEofReturnsChildStatus(t *testing.T) { + src := bytes.NewReader([]byte("all of the stdin, cleanly")) + dst := &errWriteCloser{} + code := pumpStdin(dst, src, func() int { return 7 }, io.Discard) + if code != 7 { + t.Fatalf("a clean EOF is a legitimate end; must return the child's status 7, got %d", code) + } +}