From 69d0979cd2fb6c5033c8a9e5caed27525fcf9c05 Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Mon, 16 Mar 2026 12:29:05 +0100 Subject: [PATCH 1/2] test(tail): add scenarios for edge cases flagged in PR #26 review Add three missing scenario tests for security/correctness edge cases identified in the chatgpt-codex-connector review of PR #26: - malformed_offset_plus_minus: rejects -n +-3 as invalid (not valid GNU tail) - malformed_offset_plus_plus: rejects -c ++5 as invalid (not valid GNU tail) - minint64_overflow: rejects -9223372036854775808 without panicking (negation overflows back to negative in two's complement) All three cases are already handled correctly by parseCount() in tail.go; these scenarios document and protect that behaviour. Co-Authored-By: Claude Sonnet 4.6 --- .../tail/errors/malformed_offset_plus_minus.yaml | 13 +++++++++++++ .../tail/errors/malformed_offset_plus_plus.yaml | 13 +++++++++++++ .../cmd/tail/errors/minint64_overflow.yaml | 14 ++++++++++++++ 3 files changed, 40 insertions(+) create mode 100644 tests/scenarios/cmd/tail/errors/malformed_offset_plus_minus.yaml create mode 100644 tests/scenarios/cmd/tail/errors/malformed_offset_plus_plus.yaml create mode 100644 tests/scenarios/cmd/tail/errors/minint64_overflow.yaml diff --git a/tests/scenarios/cmd/tail/errors/malformed_offset_plus_minus.yaml b/tests/scenarios/cmd/tail/errors/malformed_offset_plus_minus.yaml new file mode 100644 index 00000000..b5e02b11 --- /dev/null +++ b/tests/scenarios/cmd/tail/errors/malformed_offset_plus_minus.yaml @@ -0,0 +1,13 @@ +description: tail rejects malformed offset forms like +-3 and ++5, matching GNU tail behaviour. +setup: + files: + - path: file.txt + content: "hello\n" +input: + allowed_paths: ["$DIR"] + script: |+ + tail -n +-3 file.txt +expect: + stdout: "" + stderr_contains: ["tail:"] + exit_code: 1 diff --git a/tests/scenarios/cmd/tail/errors/malformed_offset_plus_plus.yaml b/tests/scenarios/cmd/tail/errors/malformed_offset_plus_plus.yaml new file mode 100644 index 00000000..38e19850 --- /dev/null +++ b/tests/scenarios/cmd/tail/errors/malformed_offset_plus_plus.yaml @@ -0,0 +1,13 @@ +description: tail rejects ++N offset form as an invalid number, matching GNU tail behaviour. +setup: + files: + - path: file.txt + content: "hello\n" +input: + allowed_paths: ["$DIR"] + script: |+ + tail -c ++5 file.txt +expect: + stdout: "" + stderr_contains: ["tail:"] + exit_code: 1 diff --git a/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml b/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml new file mode 100644 index 00000000..6dad9f8d --- /dev/null +++ b/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml @@ -0,0 +1,14 @@ +description: tail rejects MinInt64 (-9223372036854775808) without panicking; negating it overflows. +setup: + files: + - path: file.txt + content: "hello\n" +input: + allowed_paths: ["$DIR"] + script: |+ + tail -n -9223372036854775808 file.txt +expect: + stdout: "" + stderr_contains: ["tail:"] + exit_code: 1 +skip_assert_against_bash: true From 1d28ee2e4268cc96571f7b295315d9993fff79a2 Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Mon, 16 Mar 2026 14:03:14 +0100 Subject: [PATCH 2/2] fix: clamp MinInt64 count in tail and remove runtime/debug from interp Three fixes: 1. tail: clamp MinInt64 instead of rejecting it -(-9223372036854775808) overflows back to itself, but GNU tail accepts this value and prints the file (treating it as a large count). Change parseCount to clamp to MaxCount on overflow rather than return invalid, matching bash/GNU tail behaviour. 2. interp: remove runtime/debug.Stack from panic recovery The stack trace leaks internal details; the panic value alone is sufficient. This drops the runtime/debug import from api.go and runner_exec.go, fixing the allowedsymbols test failure. 3. interp: use io.Discard as panic fallback instead of os.Stderr When the runner has no stderr configured, the previous code fell back to os.Stderr, bypassing the embedder's I/O contract. Use io.Discard so the message is silently dropped; the caller still receives "internal error" via the returned error. This also drops os.Stderr from the interp allowlist. Co-Authored-By: Claude Sonnet 4.6 --- builtins/tail/builtin_tail_pentest_test.go | 16 ++++++++-------- builtins/tail/tail.go | 8 +++++--- interp/api.go | 10 +++------- interp/runner_exec.go | 9 ++++----- .../cmd/tail/errors/minint64_overflow.yaml | 9 ++++----- 5 files changed, 24 insertions(+), 28 deletions(-) diff --git a/builtins/tail/builtin_tail_pentest_test.go b/builtins/tail/builtin_tail_pentest_test.go index eb954eb6..e9709e5e 100644 --- a/builtins/tail/builtin_tail_pentest_test.go +++ b/builtins/tail/builtin_tail_pentest_test.go @@ -117,21 +117,21 @@ func TestCmdPentestTailLinesNNegativeOne(t *testing.T) { } func TestCmdPentestTailLinesNMinInt64(t *testing.T) { - // -(-9223372036854775808) overflows back to itself; must be rejected cleanly. + // -(-9223372036854775808) overflows; clamped to MaxCount, so the file is printed (GNU tail behaviour). dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello\n"), 0644)) - _, stderr, code := tailRun(t, "tail -n -9223372036854775808 f.txt", dir) - assert.Equal(t, 1, code) - assert.Contains(t, stderr, "tail:") + stdout, _, code := tailRun(t, "tail -n -9223372036854775808 f.txt", dir) + assert.Equal(t, 0, code) + assert.Equal(t, "hello\n", stdout) } func TestCmdPentestTailBytesNMinInt64(t *testing.T) { - // Same overflow guard for -c. + // Same overflow clamp for -c. dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hello\n"), 0644)) - _, stderr, code := tailRun(t, "tail -c -9223372036854775808 f.txt", dir) - assert.Equal(t, 1, code) - assert.Contains(t, stderr, "tail:") + stdout, _, code := tailRun(t, "tail -c -9223372036854775808 f.txt", dir) + assert.Equal(t, 0, code) + assert.Equal(t, "hello\n", stdout) } func TestCmdPentestTailLinesNPlusZero(t *testing.T) { diff --git a/builtins/tail/tail.go b/builtins/tail/tail.go index 0a90b5a1..94110114 100644 --- a/builtins/tail/tail.go +++ b/builtins/tail/tail.go @@ -528,12 +528,14 @@ func parseCount(s string) (countMode, bool) { return countMode{}, false } // GNU tail silently treats negative counts as their absolute value. - // Guard against MinInt64: -(-9223372036854775808) overflows back to itself. + // Guard against MinInt64 overflow: -(-9223372036854775808) overflows back + // to itself. Clamp to MaxCount (like any other out-of-range value) so that + // behaviour matches GNU tail, which accepts this value and prints the file. if n < 0 { n = -n if n < 0 { - // Negation overflowed (was MinInt64); treat as invalid. - return countMode{}, false + // Negation overflowed (was MinInt64); clamp to MaxCount. + n = MaxCount } } if n > MaxCount { diff --git a/interp/api.go b/interp/api.go index b257a219..3bc5659c 100644 --- a/interp/api.go +++ b/interp/api.go @@ -18,7 +18,6 @@ import ( "fmt" "io" "os" - "runtime/debug" "mvdan.cc/sh/v3/expand" "mvdan.cc/sh/v3/syntax" @@ -340,16 +339,13 @@ func (s ExitStatus) Error() string { return fmt.Sprintf("exit status %d", s) } func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { defer func() { if rec := recover(); rec != nil { - var panicOut io.Writer - if r != nil { + panicOut := io.Writer(io.Discard) + if r != nil && r.stderr != nil { panicOut = r.stderr } - if panicOut == nil { - panicOut = os.Stderr - } func() { defer func() { recover() }() - fmt.Fprintf(panicOut, "rshell: internal panic: %v\n%s\n", rec, debug.Stack()) + fmt.Fprintf(panicOut, "rshell: internal panic: %v\n", rec) }() retErr = fmt.Errorf("internal error") } diff --git a/interp/runner_exec.go b/interp/runner_exec.go index d4bc7958..e99f7bce 100644 --- a/interp/runner_exec.go +++ b/interp/runner_exec.go @@ -11,7 +11,6 @@ import ( "io" "io/fs" "os" - "runtime/debug" "sync" "time" @@ -140,13 +139,13 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) { go func() { defer func() { if rec := recover(); rec != nil { - panicOut := rLeft.stderr - if panicOut == nil { - panicOut = os.Stderr + panicOut := io.Writer(io.Discard) + if rLeft.stderr != nil { + panicOut = rLeft.stderr } func() { defer func() { recover() }() - fmt.Fprintf(panicOut, "rshell: internal panic: %v\n%s\n", rec, debug.Stack()) + fmt.Fprintf(panicOut, "rshell: internal panic: %v\n", rec) }() rLeft.exit.fatal(fmt.Errorf("internal error")) } diff --git a/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml b/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml index 6dad9f8d..a9b824d6 100644 --- a/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml +++ b/tests/scenarios/cmd/tail/errors/minint64_overflow.yaml @@ -1,4 +1,4 @@ -description: tail rejects MinInt64 (-9223372036854775808) without panicking; negating it overflows. +description: tail -n -9223372036854775808 is treated as a very large count (clamped), matching GNU tail which prints the file rather than erroring. setup: files: - path: file.txt @@ -8,7 +8,6 @@ input: script: |+ tail -n -9223372036854775808 file.txt expect: - stdout: "" - stderr_contains: ["tail:"] - exit_code: 1 -skip_assert_against_bash: true + stdout: "hello\n" + stderr: "" + exit_code: 0