From f7698a6494d9228735e25bff23f2df9a99838c80 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman <91019033+matt-dz@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:45:53 -0400 Subject: [PATCH] fix(interp): handle brace-generated invalid glob patterns without panicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fuzzing the test builtin (FuzzTestNesting) found that a script whose brace expansion synthesizes an invalid glob pattern crashes expansion. For example `{a..A}` expands over the ASCII range that includes '[' and ']', so `test {a..A}1-{a..A}0{a..A}` produces a field like `[1-0]` — an invalid character-class range. mvdan.cc/sh's glob matcher (internal.ExtendedPatternMatcher) compiles the derived pattern with regexp.MustCompile, which panics rather than returning an error. The panic unwound through field expansion to Run's recovery and surfaced as the generic "internal error" sentinel (a non-ExitStatus error), failing callers that expect a normal exit code. Field expansion now recovers that specific panic (identified by the "regexp: Compile(" prefix of regexp.MustCompile's panic value) and converts it into a clean expansion error; any other panic is re-raised so genuine bugs still surface. The same guard wraps the two FieldsSeq loops in the static checker. Valid globbing and no-match literal behavior are unchanged. Tests: TestBraceGeneratedInvalidGlobDoesNotPanic and TestValidGlobStillExpands (interp), plus the crashing input added as a FuzzTestNesting seed. Full go test -race ./... passes. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 --- builtins/tests/testcmd/testcmd_fuzz_test.go | 4 + interp/check.go | 44 +++++++-- interp/glob_pattern_panic_test.go | 100 ++++++++++++++++++++ interp/runner_expand.go | 35 ++++++- 4 files changed, 174 insertions(+), 9 deletions(-) create mode 100644 interp/glob_pattern_panic_test.go diff --git a/builtins/tests/testcmd/testcmd_fuzz_test.go b/builtins/tests/testcmd/testcmd_fuzz_test.go index 2c3062399..f409ab90b 100644 --- a/builtins/tests/testcmd/testcmd_fuzz_test.go +++ b/builtins/tests/testcmd/testcmd_fuzz_test.go @@ -299,6 +299,10 @@ func FuzzTestNesting(f *testing.F) { f.Add("1 -eq 2 -o 2 -eq 2 -o 3 -eq 4") // Mixed -a and -o f.Add("1 -eq 1 -o 1 -eq 2 -a 2 -eq 2") + // Brace sequence that expands over the ASCII '['..']' range and so forms an + // invalid glob character class ([1-0]); previously panicked mvdan's glob + // matcher during field expansion. Regression for that crash. + f.Add("{a..A}1-{a..A}0{a..A}") // Shared base dir avoids per-iteration t.TempDir() cleanup overhead. baseDir := f.TempDir() diff --git a/interp/check.go b/interp/check.go index e5eb188e4..828354286 100644 --- a/interp/check.go +++ b/interp/check.go @@ -505,6 +505,31 @@ func (c *scriptChecker) forgetCommandEffects(name string, state *checkState) { } } +// forEachCheckField iterates the fields produced by expanding word, calling +// yield for each (field, err) until yield returns false or expansion ends. It +// returns false only if mvdan.cc/sh's glob matcher panicked on a pattern it +// could not compile (see isGlobPatternPanic) — e.g. an invalid class range a +// brace expansion synthesized — so the caller can mark the result +// indeterminate rather than let the panic reach the checker. Any other panic is +// re-raised. +func forEachCheckField(cfg *expand.Config, word *syntax.Word, yield func(string, error) bool) (ok bool) { + ok = true + defer func() { + if rec := recover(); rec != nil { + if !isGlobPatternPanic(rec) { + panic(rec) + } + ok = false + } + }() + for field, err := range expand.FieldsSeq(cfg, word) { + if !yield(field, err) { + return + } + } + return +} + func (c *scriptChecker) call(stmt *syntax.Stmt, call *syntax.CallExpr, state *checkState, depth int) { entry := c.add(stmt.Pos()) if entry == nil { @@ -518,18 +543,21 @@ func (c *scriptChecker) call(stmt *syntax.Stmt, call *syntax.CallExpr, state *ch complete = false break } - for field, err := range expand.FieldsSeq(state.expansionConfig(), word) { + if !forEachCheckField(state.expansionConfig(), word, func(field string, err error) bool { if err != nil { complete = false - break + return false } bytes += len(field) if len(fields) >= MaxExpandedArgumentsPerCommand+1 || bytes > MaxExpandedBytesPerCommand { entry.issue(CheckLimitExceeded, "command expansion exceeds argument or byte limit") complete = false - break + return false } fields = append(fields, field) + return true + }) { + complete = false } if !complete { break @@ -656,15 +684,15 @@ func (c *scriptChecker) redirects(entry *CommandCheck, redirects []*syntax.Redir known := state.boundWord(redirect.Word, false, MaxExpandedBytesPerCommand) var paths []string if known { - for path, err := range expand.FieldsSeq(state.expansionConfig(), redirect.Word) { + if !forEachCheckField(state.expansionConfig(), redirect.Word, func(path string, err error) bool { if err != nil || len(path) > MaxExpandedBytesPerCommand { known = false - break + return false } paths = append(paths, path) - if len(paths) > 1 { - break - } + return len(paths) <= 1 + }) { + known = false } } if !known { diff --git a/interp/glob_pattern_panic_test.go b/interp/glob_pattern_panic_test.go new file mode 100644 index 000000000..4c4e80485 --- /dev/null +++ b/interp/glob_pattern_panic_test.go @@ -0,0 +1,100 @@ +// 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 + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestBraceGeneratedInvalidGlobDoesNotPanic is a regression test for a crash +// found by fuzzing the test builtin (FuzzTestNesting). Brace expansion can +// synthesize a glob pattern that mvdan.cc/sh's matcher cannot compile — for +// example `{a..A}` expands over the ASCII range that includes '[' and ']', so +// `test {a..A}1-{a..A}0{a..A}` produces a field like `[1-0]`, an invalid +// character-class range. mvdan's ExtendedPatternMatcher calls regexp.MustCompile +// and panics on it; before the fix that panic reached Run's recovery and +// surfaced as the generic "internal error" sentinel (a non-ExitStatus error). +// Field expansion now converts it into a clean expansion failure. +func TestBraceGeneratedInvalidGlobDoesNotPanic(t *testing.T) { + dir := t.TempDir() + scripts := map[string]string{ + "brace_sequence_forms_invalid_class": "test {a..A}1-{a..A}0{a..A}", + "brace_builds_invalid_class": "echo {a..A}1-{a..A}0", + "direct_invalid_class": "echo [1-0]", + "reversed_class": "echo [z-a]", + } + for name, script := range scripts { + t.Run(name, func(t *testing.T) { + var out, errb bytes.Buffer + r, err := New(StdIO(nil, &out, &errb), allowAllCommandsOpt(), AllowedPaths([]string{dir})) + require.NoError(t, err) + defer r.Close() + r.Dir = dir + + prog, err := ParseScript(script, "") + require.NoError(t, err) + + rerr := r.Run(context.Background(), prog) + // The run must terminate with a normal exit status, never the + // recovered-panic sentinel or any other non-ExitStatus error. + if rerr != nil { + var es ExitStatus + if !errors.As(rerr, &es) { + t.Fatalf("expected a clean exit, got non-exit error %q (stderr=%q)", rerr, errb.String()) + } + } + if strings.Contains(errb.String(), "internal panic") { + t.Fatalf("interpreter panicked instead of handling the pattern: %s", errb.String()) + } + }) + } +} + +// TestValidGlobStillExpands guards against the invalid-pattern fix regressing +// ordinary, valid globbing. +func TestValidGlobStillExpands(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{"a.txt", "b.txt"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + } + for _, tc := range []struct { + script string + want string + }{ + {"echo *.txt", "a.txt b.txt\n"}, + {"echo [ab].txt", "a.txt b.txt\n"}, + {"echo [a-z].txt", "a.txt b.txt\n"}, + {"echo nomatch*.zzz", "nomatch*.zzz\n"}, // no match: word stays literal + } { + var out, errb bytes.Buffer + r, err := New(StdIO(nil, &out, &errb), allowAllCommandsOpt(), AllowedPaths([]string{dir})) + require.NoError(t, err) + r.Dir = dir + prog, err := ParseScript(tc.script, "") + require.NoError(t, err) + if err := r.Run(context.Background(), prog); err != nil { + var es ExitStatus + if !errors.As(err, &es) { + r.Close() + t.Fatalf("%q: unexpected error: %v", tc.script, err) + } + } + r.Close() + if out.String() != tc.want { + t.Errorf("%q: got %q, want %q", tc.script, out.String(), tc.want) + } + } +} diff --git a/interp/runner_expand.go b/interp/runner_expand.go index ffe1e2a4b..109a1e263 100644 --- a/interp/runner_expand.go +++ b/interp/runner_expand.go @@ -307,7 +307,40 @@ func (c *fieldCollector) setCommandPrefixFields(n int) bool { return true } -func (c *fieldCollector) add(words ...*syntax.Word) bool { +// isGlobPatternPanic reports whether rec is the panic raised by +// mvdan.cc/sh/v3's glob matcher (internal.ExtendedPatternMatcher, which calls +// regexp.MustCompile) on a pattern it produced but cannot compile — most +// notably an invalid character-class range such as `[1-0]`. Brace expansion can +// synthesize such a pattern from innocent-looking input: `{a..A}` expands over +// the ASCII range that includes `[`, `\` and `]`, so a field like `[1-0]` can +// appear and reach pathname expansion. mvdan panics instead of returning an +// error, so field expansion must recognise and convert it; any other panic is +// re-raised so genuine interpreter bugs still surface. +func isGlobPatternPanic(rec any) bool { + // regexp.MustCompile (reached via mvdan's internal.ExtendedPatternMatcher) + // panics with a string beginning "regexp: Compile(". + var s string + switch v := rec.(type) { + case string: + s = v + case error: + s = v.Error() + default: + return false + } + return strings.HasPrefix(s, "regexp: Compile(") +} + +func (c *fieldCollector) add(words ...*syntax.Word) (ok bool) { + defer func() { + if rec := recover(); rec != nil { + if !isGlobPatternPanic(rec) { + panic(rec) + } + c.r.expandErr(fmt.Errorf("invalid glob pattern in expansion")) + ok = false + } + }() for _, word := range words { if c.r.stop(c.r.ectx) { return false