From bd08d4307ab8c1521c318365fa34afaa4122cea3 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Sat, 31 Oct 2026 17:46:40 -0400 Subject: [PATCH] fix(interp): report write errors on builtin stdout instead of exiting 0 CallContext.Out/Outf drop the error from the underlying write, and the dispatcher only looked at the builtin's own return code. So echo, printf, sed, sort, grep, cut, wc and the other builtins that print through Out exited 0 on ENOSPC/EIO/EFBIG and left a truncated file. Wrap the builtin's stdout in a WriteGuard that remembers the first write error and fails every later write with it, so builtins that check their writes stop early. When the builtin otherwise succeeded, the dispatcher prints one `: write error: ` line and sets the status to 1, as bash does. Builtins that already failed and said why (cat, head, uniq) are left alone, and a broken pipe stays silent. The find -exec / xargs child dispatch gets the same treatment. Found by Jacob Hayes with patina (https://github.com/JacobHayes/patina). Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- builtins/builtins.go | 46 +++++++++++++++ interp/builtin_write_error_test.go | 95 ++++++++++++++++++++++++++++++ interp/runner_exec.go | 21 ++++++- 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 interp/builtin_write_error_test.go diff --git a/builtins/builtins.go b/builtins/builtins.go index e21bd67d9..445397593 100644 --- a/builtins/builtins.go +++ b/builtins/builtins.go @@ -468,6 +468,52 @@ type CallContext struct { Systemd *SystemdServices } +// WriteGuard wraps a builtin's stdout and remembers the first write error. +// Builtins print through CallContext.Out/Outf, which have no error return, so +// without a guard a failed write (ENOSPC, EIO, EFBIG on a redirect target) +// would be dropped and the command would exit 0 over a truncated file. After +// the first failure the guard stops touching the destination and keeps +// returning that error, so builtins that do check their writes stop early and +// the rest cannot keep writing to a broken target. +type WriteGuard struct { + w io.Writer + err error +} + +// NewWriteGuard returns a WriteGuard that writes to w. +func NewWriteGuard(w io.Writer) *WriteGuard { return &WriteGuard{w: w} } + +// Write implements io.Writer. +func (g *WriteGuard) Write(p []byte) (int, error) { + if g.err != nil { + return 0, g.err + } + n, err := g.w.Write(p) + if err != nil { + g.err = err + } + return n, err +} + +// Err returns the first write error, or nil. +func (g *WriteGuard) Err() error { return g.err } + +// WriteErrorMessage formats a recorded write error the way bash reports a +// failed builtin write: "echo: write error: no space left on device". It +// returns "" when there is nothing to report: no error, or a broken pipe, +// which bash's builtins die from silently (SIGPIPE) rather than print. +func WriteErrorMessage(name string, err error) string { + if err == nil || IsBrokenPipe(err) { + return "" + } + var errno syscall.Errno + msg := err.Error() + if errors.As(err, &errno) { + msg = errno.Error() + } + return name + ": write error: " + msg + "\n" +} + // Out writes a string to stdout. func (c *CallContext) Out(s string) { io.WriteString(c.Stdout, s) diff --git a/interp/builtin_write_error_test.go b/interp/builtin_write_error_test.go new file mode 100644 index 000000000..739e9c23a --- /dev/null +++ b/interp/builtin_write_error_test.go @@ -0,0 +1,95 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package interp_test + +import ( + "bytes" + "context" + "errors" + "io" + "strings" + "syscall" + "testing" + + "mvdan.cc/sh/v3/syntax" + + "github.com/DataDog/rshell/internal/interpoption" + "github.com/DataDog/rshell/interp" +) + +// failingWriter models a redirect target that fails every write (ENOSPC, +// EIO, or EFBIG past RLIMIT_FSIZE). +type failingWriter struct{ err error } + +func (w failingWriter) Write([]byte) (int, error) { return 0, w.err } + +func runWithStdout(t *testing.T, script string, stdout io.Writer) (stderr string, code int) { + t.Helper() + prog, err := syntax.NewParser().Parse(strings.NewReader(script), "") + if err != nil { + t.Fatal(err) + } + var errBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(strings.NewReader("b\na\nb\n"), stdout, &errBuf), + interpoption.AllowAllCommands().(interp.RunnerOption), + ) + if err != nil { + t.Fatal(err) + } + defer runner.Close() + runErr := runner.Run(context.Background(), prog) + var es interp.ExitStatus + if runErr != nil && !errors.As(runErr, &es) { + t.Fatalf("unexpected error: %v", runErr) + } + return errBuf.String(), int(es) +} + +// A builtin whose stdout write fails must exit 1 and say so once, like +// bash's "echo: write error: No space left on device". +func TestBuiltinWriteErrorReported(t *testing.T) { + scripts := map[string]string{ + "echo": "echo hello", + "printf": "printf 'hello\\n'", + "sed": "sed p", + "sort": "sort", + "grep": "grep a", + "wc": "wc -l", + "cut": "cut -c1", + "cat": "cat", + "head": "head", + "uniq": "uniq", + } + for name, script := range scripts { + t.Run(name, func(t *testing.T) { + stderr, code := runWithStdout(t, script, failingWriter{syscall.ENOSPC}) + if code != 1 { + t.Errorf("exit code = %d, want 1 (stderr %q)", code, stderr) + } + // Builtins that already checked their writes (cat, head, uniq) + // word the message themselves; either way the line appears once. + if n := strings.Count(stderr, "\n"); n != 1 { + t.Errorf("stderr has %d lines, want one: %q", n, stderr) + } + }) + } +} + +func TestBuiltinWriteErrorFormat(t *testing.T) { + stderr, _ := runWithStdout(t, "echo hello", failingWriter{syscall.ENOSPC}) + if want := "echo: write error: no space left on device\n"; stderr != want { + t.Errorf("stderr = %q, want %q", stderr, want) + } +} + +// A broken pipe is not an error bash prints for a builtin. +func TestBuiltinWriteErrorBrokenPipeSilent(t *testing.T) { + stderr, _ := runWithStdout(t, "echo hello", failingWriter{syscall.EPIPE}) + if stderr != "" { + t.Errorf("stderr = %q, want empty", stderr) + } +} diff --git a/interp/runner_exec.go b/interp/runner_exec.go index 92e0fab16..0201e3d05 100644 --- a/interp/runner_exec.go +++ b/interp/runner_exec.go @@ -918,8 +918,9 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string, setup r.errf("%s", msg) return 1, nil } + childOut := builtins.NewWriteGuard(r.stdout) child := &builtins.CallContext{ - Stdout: r.stdout, + Stdout: childOut, Stderr: r.stderr, WorkDir: func() string { return dir }, Env: envEach, @@ -1061,13 +1062,20 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string, setup child.Stdin = r.stdin } result := cmdFn(ctx, child, cmdArgs) + if result.Code == 0 { + if msg := builtins.WriteErrorMessage(cmdName, childOut.Err()); msg != "" { + r.errf("%s", msg) + result.Code = 1 + } + } return result.Code, nil } runCmd := func(ctx context.Context, dir string, cmdName string, cmdArgs []string) (uint8, error) { return runCmdWithStdin(ctx, dir, cmdName, cmdArgs, nil) } + callOut := builtins.NewWriteGuard(r.stdout) call := &builtins.CallContext{ - Stdout: r.stdout, + Stdout: callOut, Stderr: r.stderr, InLoop: r.inLoop, LastExitCode: r.lastExit.code, @@ -1215,6 +1223,15 @@ func (r *Runner) call(ctx context.Context, pos syntax.Pos, args []string, setup } else { result = fn(ctx, call, args[1:]) } + // Builtins print through CallContext.Out, which cannot fail; report a + // failed write (ENOSPC, EIO, EFBIG) here once, unless the builtin + // already failed and said why. + if result.Code == 0 { + if msg := builtins.WriteErrorMessage(name, callOut.Err()); msg != "" { + r.errf("%s", msg) + result.Code = 1 + } + } r.exit.code = result.Code r.exit.exiting = result.Exiting r.breakEnclosing = result.BreakN