From 438e9b56069b584c7a51c76994c705a9042c0c91 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 12:27:54 -0400 Subject: [PATCH 1/4] feat(interp): add global 10 MiB stdout cap to Runner.Run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrap the runner's stdout writer with a limitWriter at the start of each Run() call. Bytes beyond maxStdoutBytes (10 MiB) are silently discarded during execution so builtins never see a mid-stream write error, but the exceeded flag is checked after the script finishes and Run() returns ErrOutputLimitExceeded — a stable, exported sentinel — so callers get a well-defined error rather than a silent truncation. Also adds TestGlobalStdoutCapReturnsError to verify the cap fires, the error is returned with the correct value, partial output up to the limit is still delivered, and output below the cap is not suppressed. Co-Authored-By: Claude Sonnet 4.6 --- interp/api.go | 15 +++++++++ interp/runner_expand.go | 19 +++++++++-- interp/tests/cmdsubst_hardening_test.go | 43 +++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/interp/api.go b/interp/api.go index ce817918..e2a0b572 100644 --- a/interp/api.go +++ b/interp/api.go @@ -421,6 +421,11 @@ func (r *Runner) Reset() { r.didReset = true } +// ErrOutputLimitExceeded is returned by Run when a script produces more stdout +// than maxStdoutBytes. Partial output up to the limit is still delivered to the +// caller's writer. Use errors.Is to check for this condition. +var ErrOutputLimitExceeded = fmt.Errorf("stdout limit exceeded: script produced more than %d MiB of output", maxStdoutBytes/(1024*1024)) + // ExitStatus is a non-zero status code resulting from running a shell node. type ExitStatus uint8 @@ -458,6 +463,12 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { return r.exit.err } } + // Wrap stdout with a cap. Bytes beyond maxStdoutBytes are silently + // discarded so that builtins never see a write error mid-execution, but + // the exceeded flag is set so Run() can surface a well-defined error to + // the caller after the script finishes. + stdoutCap := &limitWriter{w: r.stdout, limit: maxStdoutBytes} + r.stdout = stdoutCap r.startTime = time.Now() r.globReadDirCount = &atomic.Int64{} r.fillExpandConfig(ctx) @@ -478,6 +489,10 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { default: return fmt.Errorf("node can only be File, Stmt, or Command: %T", node) } + // If the script exceeded the stdout cap, report it regardless of exit code. + if stdoutCap.exceeded { + return ErrOutputLimitExceeded + } // Return the first of: a fatal error, a non-fatal handler error, or the exit code. if err := r.exit.err; err != nil { return err diff --git a/interp/runner_expand.go b/interp/runner_expand.go index 2307a72d..d8c4a249 100644 --- a/interp/runner_expand.go +++ b/interp/runner_expand.go @@ -49,6 +49,12 @@ func (r *Runner) updateExpandOpts() { // commands that produce unbounded output. const maxCmdSubstOutput = 1 << 20 // 1 MiB +// maxStdoutBytes is the maximum number of bytes a script can write to stdout +// before further output is silently discarded. This caps total script output +// to prevent memory exhaustion from runaway commands (e.g. infinite loops +// writing to stdout). +const maxStdoutBytes = 10 * 1024 * 1024 // 10 MiB + // MaxGlobReadDirCalls is the maximum number of ReadDirForGlob invocations // allowed per Run() call. This prevents memory exhaustion from scripts that // trigger an excessive number of glob expansions (e.g. millions of unquoted @@ -130,14 +136,20 @@ func catShortcutArg(stmt *syntax.Stmt) *syntax.Word { } // limitWriter wraps a writer and stops writing after limit bytes. +// When the limit is exceeded, exceeded is set to true and further writes +// are silently discarded so that callers do not see spurious short-write +// errors mid-execution. The exceeded flag can be checked after execution +// to surface the event as an error. type limitWriter struct { - w io.Writer - limit int64 - n int64 + w io.Writer + limit int64 + n int64 + exceeded bool } func (lw *limitWriter) Write(p []byte) (int, error) { if lw.n >= lw.limit { + lw.exceeded = true return len(p), nil // silently discard excess } remaining := lw.limit - lw.n @@ -146,6 +158,7 @@ func (lw *limitWriter) Write(p []byte) (int, error) { return int(remaining), err } lw.n = lw.limit + lw.exceeded = true return len(p), nil // report all bytes consumed to avoid short-write errors } n, err := lw.w.Write(p) diff --git a/interp/tests/cmdsubst_hardening_test.go b/interp/tests/cmdsubst_hardening_test.go index 8392dd83..1c08888f 100644 --- a/interp/tests/cmdsubst_hardening_test.go +++ b/interp/tests/cmdsubst_hardening_test.go @@ -6,6 +6,7 @@ package tests_test import ( + "bytes" "context" "fmt" "os" @@ -16,10 +17,52 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "mvdan.cc/sh/v3/syntax" + + "github.com/DataDog/rshell/internal/interpoption" + "github.com/DataDog/rshell/interp" ) // --- Memory limits: output capping --- +// TestGlobalStdoutCapReturnsError verifies that Run returns ErrOutputLimitExceeded +// when a script exceeds the 10 MiB stdout cap. The script runs to completion +// but partial output (up to the limit) is still delivered, and the caller +// receives a well-defined error rather than a silent truncation. +func TestGlobalStdoutCapReturnsError(t *testing.T) { + dir := t.TempDir() + + // Create a file of exactly 1 MiB. + content := strings.Repeat("A", 1<<20) + require.NoError(t, os.WriteFile(filepath.Join(dir, "mb.txt"), []byte(content), 0644)) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + // cat the file 11 times — produces 11 MiB, exceeding the 10 MiB cap. + script := `for i in 1 2 3 4 5 6 7 8 9 10 11; do cat mb.txt; done` + var outBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(nil, &outBuf, nil), + interp.AllowedPaths([]string{dir}), + interpoption.AllowAllCommands().(interp.RunnerOption), + ) + require.NoError(t, err) + defer runner.Close() + runner.Dir = dir + + prog, err := syntax.NewParser().Parse(strings.NewReader(script), "test") + require.NoError(t, err) + + runErr := runner.Run(ctx, prog) + assert.ErrorIs(t, runErr, interp.ErrOutputLimitExceeded, + "Run must return ErrOutputLimitExceeded when stdout cap is exceeded") + // Output up to the cap must still be delivered. + assert.LessOrEqual(t, outBuf.Len(), 10*1024*1024, + "stdout must not exceed 10 MiB; got %d bytes", outBuf.Len()) + assert.Greater(t, outBuf.Len(), 0, "expected non-empty stdout before cap") +} + func TestCmdSubstOutputCapped(t *testing.T) { // Generate output exceeding 1 MiB inside command substitution. // The output should be truncated, not cause OOM. From 9f6b8e22a5d8f7971bebfd95e5624393fe912ba2 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 12:45:08 -0400 Subject: [PATCH 2/4] fix(interp): address stdout cap review findings - Restore r.stdout after each Run() via defer so repeated calls do not double-wrap the writer and silently inherit the previous call's byte count - Move ErrOutputLimitExceeded check after r.exit.err so fatal execution errors take precedence over the stdout cap error - Change ErrOutputLimitExceeded from fmt.Errorf to errors.New (no wrapping) - Add TestGlobalStdoutCapMultipleRuns to cover the double-wrap regression - Add TestGlobalStdoutCapPrecedenceOverExitCode to document error ordering Co-Authored-By: Claude Sonnet 4.6 --- interp/api.go | 20 +++--- interp/tests/cmdsubst_hardening_test.go | 88 +++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/interp/api.go b/interp/api.go index e2a0b572..09bc95e3 100644 --- a/interp/api.go +++ b/interp/api.go @@ -424,7 +424,7 @@ func (r *Runner) Reset() { // ErrOutputLimitExceeded is returned by Run when a script produces more stdout // than maxStdoutBytes. Partial output up to the limit is still delivered to the // caller's writer. Use errors.Is to check for this condition. -var ErrOutputLimitExceeded = fmt.Errorf("stdout limit exceeded: script produced more than %d MiB of output", maxStdoutBytes/(1024*1024)) +var ErrOutputLimitExceeded = errors.New("stdout limit exceeded: script produced more than 10 MiB of output") // ExitStatus is a non-zero status code resulting from running a shell node. type ExitStatus uint8 @@ -466,9 +466,12 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { // Wrap stdout with a cap. Bytes beyond maxStdoutBytes are silently // discarded so that builtins never see a write error mid-execution, but // the exceeded flag is set so Run() can surface a well-defined error to - // the caller after the script finishes. - stdoutCap := &limitWriter{w: r.stdout, limit: maxStdoutBytes} + // the caller after the script finishes. Restore r.stdout on return so + // that repeated Run() calls without Reset() do not double-wrap the writer. + prevStdout := r.stdout + stdoutCap := &limitWriter{w: prevStdout, limit: maxStdoutBytes} r.stdout = stdoutCap + defer func() { r.stdout = prevStdout }() r.startTime = time.Now() r.globReadDirCount = &atomic.Int64{} r.fillExpandConfig(ctx) @@ -489,14 +492,15 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { default: return fmt.Errorf("node can only be File, Stmt, or Command: %T", node) } - // If the script exceeded the stdout cap, report it regardless of exit code. - if stdoutCap.exceeded { - return ErrOutputLimitExceeded - } - // Return the first of: a fatal error, a non-fatal handler error, or the exit code. + // Return the first of: a fatal/handler error, stdout cap exceeded, or the exit code. + // Fatal errors take precedence over ErrOutputLimitExceeded so that cancellation + // and handler failures are not masked when the cap is also hit. if err := r.exit.err; err != nil { return err } + if stdoutCap.exceeded { + return ErrOutputLimitExceeded + } if code := r.exit.code; code != 0 { return ExitStatus(code) } diff --git a/interp/tests/cmdsubst_hardening_test.go b/interp/tests/cmdsubst_hardening_test.go index 1c08888f..6108727b 100644 --- a/interp/tests/cmdsubst_hardening_test.go +++ b/interp/tests/cmdsubst_hardening_test.go @@ -8,6 +8,7 @@ package tests_test import ( "bytes" "context" + "errors" "fmt" "os" "path/filepath" @@ -63,6 +64,93 @@ func TestGlobalStdoutCapReturnsError(t *testing.T) { assert.Greater(t, outBuf.Len(), 0, "expected non-empty stdout before cap") } +// TestGlobalStdoutCapMultipleRuns verifies that repeated Run() calls on the +// same Runner without Reset() do not double-wrap the stdout writer. The first +// call must not leave r.stdout pointing at the limitWriter, so the second call +// starts with a fresh 10 MiB budget rather than inheriting the first call's +// byte counter. +func TestGlobalStdoutCapMultipleRuns(t *testing.T) { + dir := t.TempDir() + content := strings.Repeat("A", 1<<20) // 1 MiB + require.NoError(t, os.WriteFile(filepath.Join(dir, "mb.txt"), []byte(content), 0644)) + + var outBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(nil, &outBuf, nil), + interp.AllowedPaths([]string{dir}), + interpoption.AllowAllCommands().(interp.RunnerOption), + ) + require.NoError(t, err) + defer runner.Close() + runner.Dir = dir + + parse := func(script string) *syntax.File { + t.Helper() + prog, parseErr := syntax.NewParser().Parse(strings.NewReader(script), "test") + require.NoError(t, parseErr) + return prog + } + + ctx := context.Background() + + // First call: write 9 MiB — just under the cap. Must succeed. + outBuf.Reset() + ctx1, cancel1 := context.WithTimeout(ctx, 10*time.Second) + defer cancel1() + err = runner.Run(ctx1, parse(`for i in 1 2 3 4 5 6 7 8 9; do cat mb.txt; done`)) + assert.NoError(t, err, "first run (9 MiB) must not exceed cap") + assert.Equal(t, 9<<20, outBuf.Len(), "first run must deliver exactly 9 MiB") + + // Second call: write another 9 MiB. If r.stdout was not restored, the + // wrapped limitWriter from call 1 already has 9 MiB counted and would + // silently drop all output here — returning no error. A fresh budget means + // this call also succeeds with 9 MiB delivered. + outBuf.Reset() + ctx2, cancel2 := context.WithTimeout(ctx, 10*time.Second) + defer cancel2() + err = runner.Run(ctx2, parse(`for i in 1 2 3 4 5 6 7 8 9; do cat mb.txt; done`)) + assert.NoError(t, err, "second run (9 MiB) must not exceed cap") + assert.Equal(t, 9<<20, outBuf.Len(), "second run must deliver exactly 9 MiB (fresh cap)") +} + +// TestGlobalStdoutCapPrecedenceOverExitCode verifies that ErrOutputLimitExceeded +// takes precedence over a non-zero exit code when both occur in the same Run() +// call. Fatal handler errors (r.exit.err) still take precedence over the cap +// per the ordering in Run(), but that path is not easily triggerable from a +// script-level test. +func TestGlobalStdoutCapPrecedenceOverExitCode(t *testing.T) { + dir := t.TempDir() + content := strings.Repeat("A", 1<<20) // 1 MiB + require.NoError(t, os.WriteFile(filepath.Join(dir, "mb.txt"), []byte(content), 0644)) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + var outBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(nil, &outBuf, nil), + interp.AllowedPaths([]string{dir}), + interpoption.AllowAllCommands().(interp.RunnerOption), + ) + require.NoError(t, err) + defer runner.Close() + runner.Dir = dir + + // Exceed the cap then exit non-zero. ErrOutputLimitExceeded must be returned, + // not ExitStatus(1). + prog, parseErr := syntax.NewParser().Parse(strings.NewReader( + `for i in 1 2 3 4 5 6 7 8 9 10 11; do cat mb.txt; done; exit 1`, + ), "test") + require.NoError(t, parseErr) + + runErr := runner.Run(ctx, prog) + assert.ErrorIs(t, runErr, interp.ErrOutputLimitExceeded, + "ErrOutputLimitExceeded must take precedence over a non-zero exit code") + var es interp.ExitStatus + assert.False(t, errors.As(runErr, &es), + "must not return ExitStatus when stdout cap was exceeded") +} + func TestCmdSubstOutputCapped(t *testing.T) { // Generate output exceeding 1 MiB inside command substitution. // The output should be truncated, not cause OOM. From dd6b6ccb22ab8ae8c090b0cc8e87f9e114e8dc50 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 13:20:34 -0400 Subject: [PATCH 3/4] fix(allowedsymbols): add errors.New to interpAllowedSymbols interp/api.go uses errors.New to define the ErrOutputLimitExceeded sentinel, but errors.New was missing from the interp allowlist, causing TestInterpAllowedSymbols and TestVerificationInterpCleanPass to fail. Co-Authored-By: Claude Sonnet 4.6 --- allowedsymbols/symbols_interp.go | 1 + 1 file changed, 1 insertion(+) diff --git a/allowedsymbols/symbols_interp.go b/allowedsymbols/symbols_interp.go index 5960185b..810e9bdf 100644 --- a/allowedsymbols/symbols_interp.go +++ b/allowedsymbols/symbols_interp.go @@ -24,6 +24,7 @@ var interpAllowedSymbols = []string{ "context.WithTimeout", // 🟢 derives a context with a deadline; needed for execution timeout support. "context.WithValue", // 🟢 derives a context carrying a key-value pair; pure function. "errors.As", // 🟢 error type assertion; pure function, no I/O. + "errors.New", // 🟢 creates a sentinel error value; pure function, no I/O. "fmt.Errorf", // 🟢 formatted error creation; pure function, no I/O. "fmt.Fprintf", // 🟠 formatted write to an io.Writer; delegates to Write, no filesystem access. "fmt.Fprintln", // 🟠 writes to an io.Writer with newline; delegates to Write, no filesystem access. From 2151ff4cf70367ee3b554ed65e732316841002fb Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 13:54:18 -0400 Subject: [PATCH 4/4] fix(interp): make limitWriter thread-safe; derive error message from const MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues raised in code review: 1. limitWriter was not safe for concurrent use — the byte counter (n) and exceeded flag were unprotected plain fields. Since background shell jobs can write to r.stdout concurrently, limitWriter must protect its state. Added sync.Mutex; Write() holds it for the full duration so n and exceeded are always consistent. Added isExceeded() so callers can also read the flag safely. 2. ErrOutputLimitExceeded hardcoded "10 MiB" in the error string. If maxStdoutBytes ever changes the message becomes stale. Switch to fmt.Sprintf so the message is derived from the constant at init time. Co-Authored-By: Claude Sonnet 4.6 --- interp/api.go | 7 +++++-- interp/runner_expand.go | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/interp/api.go b/interp/api.go index 09bc95e3..94e10489 100644 --- a/interp/api.go +++ b/interp/api.go @@ -424,7 +424,10 @@ func (r *Runner) Reset() { // ErrOutputLimitExceeded is returned by Run when a script produces more stdout // than maxStdoutBytes. Partial output up to the limit is still delivered to the // caller's writer. Use errors.Is to check for this condition. -var ErrOutputLimitExceeded = errors.New("stdout limit exceeded: script produced more than 10 MiB of output") +var ErrOutputLimitExceeded = errors.New(fmt.Sprintf( + "stdout limit exceeded: script produced more than %d MiB of output", + maxStdoutBytes/(1024*1024), +)) // ExitStatus is a non-zero status code resulting from running a shell node. type ExitStatus uint8 @@ -498,7 +501,7 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { if err := r.exit.err; err != nil { return err } - if stdoutCap.exceeded { + if stdoutCap.isExceeded() { return ErrOutputLimitExceeded } if code := r.exit.code; code != 0 { diff --git a/interp/runner_expand.go b/interp/runner_expand.go index d8c4a249..42252e19 100644 --- a/interp/runner_expand.go +++ b/interp/runner_expand.go @@ -14,6 +14,7 @@ import ( "io/fs" "os" "strings" + "sync" "mvdan.cc/sh/v3/expand" "mvdan.cc/sh/v3/syntax" @@ -139,8 +140,13 @@ func catShortcutArg(stmt *syntax.Stmt) *syntax.Word { // When the limit is exceeded, exceeded is set to true and further writes // are silently discarded so that callers do not see spurious short-write // errors mid-execution. The exceeded flag can be checked after execution -// to surface the event as an error. +// via isExceeded to surface the event as an error. +// +// limitWriter is safe for concurrent use: the mutex serialises writes so +// that the byte counter and exceeded flag are always consistent, even when +// background goroutines write to the same writer concurrently. type limitWriter struct { + mu sync.Mutex w io.Writer limit int64 n int64 @@ -148,6 +154,8 @@ type limitWriter struct { } func (lw *limitWriter) Write(p []byte) (int, error) { + lw.mu.Lock() + defer lw.mu.Unlock() if lw.n >= lw.limit { lw.exceeded = true return len(p), nil // silently discard excess @@ -166,6 +174,14 @@ func (lw *limitWriter) Write(p []byte) (int, error) { return n, err } +// isExceeded reports whether any write has exceeded the byte limit. +// It is safe to call concurrently with Write. +func (lw *limitWriter) isExceeded() bool { + lw.mu.Lock() + defer lw.mu.Unlock() + return lw.exceeded +} + func (r *Runner) expandErr(err error) { if err == nil { return