From 26232f590e6d7824095802943b2521306d94cabc Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 13:26:45 -0400 Subject: [PATCH 1/5] feat(allowedsymbols): add structural AST rules for Scanner.Buffer and OpenFile.Close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new structural rules enforced on every file that passes through checkAllowedSymbols, plus as standalone go/analysis analyzers: Rule 1 (ScannerBufferAnalyzer): every bufio.NewScanner result must have .Buffer() called in the same function scope. Without it the scanner uses a 64 KiB fixed buffer and silently fails on longer lines, a reliability and DoS risk for builtins handling arbitrary input. Rule 2 (OpenFileCloseAnalyzer): every callCtx.OpenFile result must be closed via defer or explicit Close() in the same function scope. The checker understands hand-off assignments (rc = f; defer rc.Close()) and return ownership transfer (return f, nil) to avoid false positives. Also fixes two genuine violations caught by the new rules: - builtins/internal/procinfo/procinfo_linux.go: add .Buffer() to two scanners reading /proc files. Documents both rules in allowedsymbols/README.md §Structural Rules with compliant and violation examples. Co-Authored-By: Claude Sonnet 4.6 --- allowedsymbols/README.md | 62 +++++ allowedsymbols/structural.go | 232 +++++++++++++++++ allowedsymbols/structural_test.go | 253 +++++++++++++++++++ allowedsymbols/symbols_test.go | 4 + builtins/internal/procinfo/procinfo_linux.go | 2 + 5 files changed, 553 insertions(+) create mode 100644 allowedsymbols/structural.go create mode 100644 allowedsymbols/structural_test.go diff --git a/allowedsymbols/README.md b/allowedsymbols/README.md index ccca2139..12b0a846 100644 --- a/allowedsymbols/README.md +++ b/allowedsymbols/README.md @@ -56,6 +56,68 @@ The tests in this package use Go's `go/parser` and `go/ast` to walk source files Verification tests additionally inject banned imports or unlisted symbols into a temporary copy of the repo and assert the checker catches them. +## Structural Rules + +In addition to symbol-level allowlist checking, the package enforces **structural rules** — code patterns that must (or must not) appear together in the same function scope. These are checked by `checkFileScannerBuffer` and `checkFileOpenFileClose` in `structural.go` and are applied automatically to every file that passes through `checkAllowedSymbols`. + +Both rules are also exposed as standalone `go/analysis` analyzers (`ScannerBufferAnalyzer`, `OpenFileCloseAnalyzer`) that can be registered with `go vet` or gopls. + +### Rule 1 — `bufio.NewScanner` must call `.Buffer()` + +**Why:** `bufio.Scanner` has a fixed default buffer of 64 KiB. Any line longer than that causes `Scanner.Scan()` to return `false` and `Scanner.Err()` to return `bufio.ErrTooLong`. In a shell that must handle arbitrary user input this is a reliability and DoS risk — a single long line silently truncates or aborts processing. + +**What is checked:** Every variable assigned from `bufio.NewScanner(...)` must have `.Buffer(...)` called on it within the same function body. Nested function literals (`func() { ... }`) are treated as independent scopes. + +**Compliant:** +```go +sc := bufio.NewScanner(r) +sc.Buffer(make([]byte, 4096), maxLineBytes) +for sc.Scan() { ... } +``` + +**Violation:** +```go +sc := bufio.NewScanner(r) // flagged: no sc.Buffer() call +for sc.Scan() { ... } +``` + +### Rule 2 — `callCtx.OpenFile` results must be closed + +**Why:** Every open file descriptor consumes a kernel resource. Over repeated script executions, unclosed handles exhaust the process file-descriptor limit and cause all subsequent I/O to fail. + +**What is checked:** Every variable assigned from a `.OpenFile(...)` call (any receiver — the check matches the method name, not the receiver type) must have `.Close()` called on it — directly or via `defer` — within the same function body. The checker also tracks **hand-off** assignments: if `f` is reassigned to `rc` and `rc.Close()` is called, `f` is considered closed. + +**Compliant — direct close:** +```go +f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0) +if err != nil { return err } +defer f.Close() +``` + +**Compliant — hand-off pattern:** +```go +f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0) +if err != nil { return err } +rc = f // hand off to rc +defer rc.Close() // closes f transitively +``` + +**Compliant — return ownership transfer:** +```go +func openHelper(callCtx cc) (io.ReadCloser, error) { + f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0) + if err != nil { return nil, err } + return f, nil // caller is responsible for closing +} +``` + +**Violation:** +```go +f, err := callCtx.OpenFile(ctx, path, os.O_RDONLY, 0) +if err != nil { return err } +_ = f // flagged: f is never closed +``` + ## Adding a New Symbol 1. Add a line to the appropriate allowlist (and the per-command sublist if it's a builtin). diff --git a/allowedsymbols/structural.go b/allowedsymbols/structural.go new file mode 100644 index 00000000..99b5276c --- /dev/null +++ b/allowedsymbols/structural.go @@ -0,0 +1,232 @@ +// 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 allowedsymbols + +import ( + "go/ast" + "go/token" + + "golang.org/x/tools/go/analysis" +) + +// ScannerBufferAnalyzer checks that every bufio.NewScanner call in the +// analyzed package has a corresponding .Buffer() call on the returned value +// within the same function scope. Without Buffer(), the scanner uses a fixed +// 64 KiB internal buffer and fails on lines longer than that — a reliability +// and DoS risk for builtins that must handle arbitrary input. +var ScannerBufferAnalyzer = &analysis.Analyzer{ + Name: "scannerbuffer", + Doc: "checks that bufio.NewScanner results have Buffer() called to set a bounded read buffer", + Run: runScannerBuffer, +} + +// OpenFileCloseAnalyzer checks that every callCtx.OpenFile call result that +// is assigned to a variable has a corresponding .Close() call (direct or via +// defer) within the same function scope. Unclosed file handles exhaust file +// descriptors over repeated script executions. +var OpenFileCloseAnalyzer = &analysis.Analyzer{ + Name: "openfileclose", + Doc: "checks that callCtx.OpenFile results are always closed within the same function", + Run: runOpenFileClose, +} + +func runScannerBuffer(pass *analysis.Pass) (any, error) { + for _, f := range pass.Files { + checkFileScannerBuffer(f, func(pos token.Pos, format string, args ...any) { + pass.Reportf(pos, format, args...) + }) + } + return nil, nil +} + +func runOpenFileClose(pass *analysis.Pass) (any, error) { + for _, f := range pass.Files { + checkFileOpenFileClose(f, func(pos token.Pos, format string, args ...any) { + pass.Reportf(pos, format, args...) + }) + } + return nil, nil +} + +// checkFileScannerBuffer enforces the Scanner.Buffer() rule on a single file. +// It is also called directly by the test harness in symbols_test.go. +func checkFileScannerBuffer(f *ast.File, report func(pos token.Pos, format string, args ...any)) { + forEachFuncBody(f, func(body *ast.BlockStmt) { + type scannerVar struct { + pos token.Pos + name string + } + var scanners []scannerVar + buffered := make(map[string]bool) + + inspectBody(body, func(n ast.Node) { + switch node := n.(type) { + case *ast.AssignStmt: + // Detect: x := bufio.NewScanner(...) + for i, rhs := range node.Rhs { + if !isCall(rhs, "bufio", "NewScanner") { + continue + } + if i < len(node.Lhs) { + if id, ok := node.Lhs[i].(*ast.Ident); ok && id.Name != "_" { + scanners = append(scanners, scannerVar{pos: rhs.Pos(), name: id.Name}) + } + } + } + case *ast.CallExpr: + // Detect: x.Buffer(...) + if sel, ok := node.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Buffer" { + if id, ok := sel.X.(*ast.Ident); ok { + buffered[id.Name] = true + } + } + } + }) + + for _, sc := range scanners { + if !buffered[sc.name] { + report(sc.pos, + "bufio.NewScanner result %q must have .Buffer() called to cap the maximum line size (see allowedsymbols/README.md §Structural Rules)", + sc.name) + } + } + }) +} + +// checkFileOpenFileClose enforces the OpenFile-must-be-closed rule on a +// single file. It is also called directly by the test harness. +// +// The rule accounts for the common hand-off pattern: +// +// f, err := callCtx.OpenFile(...) +// rc = f // hand off to rc +// defer rc.Close() // closes f transitively +func checkFileOpenFileClose(f *ast.File, report func(pos token.Pos, format string, args ...any)) { + forEachFuncBody(f, func(body *ast.BlockStmt) { + type openVar struct { + pos token.Pos + name string + } + var opens []openVar + closed := make(map[string]bool) + // handOff maps a "holder" variable name to the original variable it was + // assigned from. Closing the holder counts as closing the original. + handOff := make(map[string]string) // holder → original + // returned tracks variables that appear in a return statement. Returning + // a file handle transfers ownership to the caller, so no Close() is + // required in the current scope. + returned := make(map[string]bool) + + inspectBody(body, func(n ast.Node) { + switch node := n.(type) { + case *ast.AssignStmt: + for i, rhs := range node.Rhs { + // Detect: f, err := .OpenFile(...) + if call, ok := rhs.(*ast.CallExpr); ok { + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "OpenFile" { + if i < len(node.Lhs) { + if id, ok := node.Lhs[i].(*ast.Ident); ok && id.Name != "_" { + opens = append(opens, openVar{pos: rhs.Pos(), name: id.Name}) + } + } + } + } + // Detect: rc = f (hand-off from OpenFile result to another var). + // Works for both := and = assignments. + if rhsId, ok := rhs.(*ast.Ident); ok && i < len(node.Lhs) { + if lhsId, ok := node.Lhs[i].(*ast.Ident); ok && lhsId.Name != "_" { + handOff[lhsId.Name] = rhsId.Name + } + } + } + case *ast.ReturnStmt: + // Detect: return f, nil — caller takes ownership, no Close needed here. + for _, result := range node.Results { + if id, ok := result.(*ast.Ident); ok { + returned[id.Name] = true + } + } + case *ast.CallExpr: + // Detect: f.Close() or defer f.Close() (the defer wrapper is + // stripped by inspectBody before the CallExpr reaches here). + if sel, ok := node.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Close" { + if id, ok := sel.X.(*ast.Ident); ok { + closed[id.Name] = true + } + } + } + }) + + for _, ov := range opens { + if !isClosedTransitive(ov.name, closed, handOff) && !returned[ov.name] { + report(ov.pos, + "OpenFile result %q must be closed via defer or explicit Close() call (see allowedsymbols/README.md §Structural Rules)", + ov.name) + } + } + }) +} + +// isClosedTransitive returns true if name is closed directly or if a variable +// that name was handed off to is closed. +func isClosedTransitive(name string, closed map[string]bool, handOff map[string]string) bool { + if closed[name] { + return true + } + for holder, orig := range handOff { + if orig == name && closed[holder] { + return true + } + } + return false +} + +// forEachFuncBody calls fn for every function body in f — both top-level +// FuncDecl bodies and FuncLit bodies — so each scope is checked independently. +func forEachFuncBody(f *ast.File, fn func(*ast.BlockStmt)) { + ast.Inspect(f, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.FuncDecl: + if node.Body != nil { + fn(node.Body) + } + case *ast.FuncLit: + fn(node.Body) + } + return true + }) +} + +// inspectBody walks body without recursing into nested FuncLit nodes (which +// are treated as independent scopes by forEachFuncBody). fn is called for +// every non-FuncLit node encountered. +func inspectBody(body *ast.BlockStmt, fn func(ast.Node)) { + ast.Inspect(body, func(n ast.Node) bool { + if n == nil { + return false + } + if _, ok := n.(*ast.FuncLit); ok { + return false // handled as its own scope by forEachFuncBody + } + fn(n) + return true + }) +} + +// isCall returns true if expr is a call to pkg.Name (using the local package +// alias name, e.g. "bufio" for import "bufio"). +func isCall(expr ast.Expr, localPkg, name string) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + id, ok := sel.X.(*ast.Ident) + return ok && id.Name == localPkg && sel.Sel.Name == name +} diff --git a/allowedsymbols/structural_test.go b/allowedsymbols/structural_test.go new file mode 100644 index 00000000..40d87cb2 --- /dev/null +++ b/allowedsymbols/structural_test.go @@ -0,0 +1,253 @@ +// 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 allowedsymbols + +import ( + "go/parser" + "go/token" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// parseSnippet parses a minimal Go source file containing the given body +// inserted into a function named "f". The returned file can be passed to +// checkFileScannerBuffer or checkFileOpenFileClose. +func parseSnippet(t *testing.T, body string) (*token.FileSet, interface{ Pos() token.Pos }) { + t.Helper() + src := `package p + +import ( + "bufio" + "context" + "os" +) + +type fakeCtx struct{} +func (fakeCtx) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { + return nil, nil +} + +func run(callCtx fakeCtx) { +` + body + ` +} +` + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "snippet.go", src, 0) + require.NoError(t, err) + return fset, f +} + +// collectViolations runs fn against a parsed snippet and returns all reported +// violation messages. +func collectViolations(t *testing.T, body string, fn func(f interface{ Pos() token.Pos }, report func(token.Pos, string, ...any))) []string { + t.Helper() + _, f := parseSnippet(t, body) + + type astFile interface { + Pos() token.Pos + } + + var msgs []string + fn(f, func(_ token.Pos, format string, args ...any) { + msg := format + if len(args) > 0 { + msg = format // simplified: just collect format for test assertions + } + msgs = append(msgs, msg) + }) + return msgs +} + +// --- ScannerBuffer rule tests --- + +func TestScannerBufferClean(t *testing.T) { + // Scanner with Buffer() called — must not be flagged. + fset := token.NewFileSet() + src := `package p +import "bufio" +func run() { + sc := bufio.NewScanner(nil) + sc.Buffer(make([]byte, 4096), 1<<20) + _ = sc.Scan() +} +` + f, err := parser.ParseFile(fset, "clean.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileScannerBuffer(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Empty(t, violations, "Scanner with Buffer() should not be flagged") +} + +func TestScannerBufferMissing(t *testing.T) { + // Scanner without Buffer() — must be flagged. + fset := token.NewFileSet() + src := `package p +import "bufio" +func run() { + sc := bufio.NewScanner(nil) + _ = sc.Scan() +} +` + f, err := parser.ParseFile(fset, "missing.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileScannerBuffer(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Len(t, violations, 1, "Scanner without Buffer() must be flagged") +} + +func TestScannerBufferInsideFuncLit(t *testing.T) { + // Scanner inside a func literal without Buffer() — must be flagged as its own scope. + fset := token.NewFileSet() + src := `package p +import "bufio" +func run() { + fn := func() { + sc := bufio.NewScanner(nil) + _ = sc.Scan() + } + fn() +} +` + f, err := parser.ParseFile(fset, "funlit.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileScannerBuffer(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Len(t, violations, 1, "Scanner inside func literal without Buffer() must be flagged") +} + +func TestScannerBufferOuterCleanInnerMissing(t *testing.T) { + // Outer function has Buffer(), inner func literal does not — only inner flagged. + fset := token.NewFileSet() + src := `package p +import "bufio" +func run() { + sc := bufio.NewScanner(nil) + sc.Buffer(make([]byte, 4096), 1<<20) + _ = sc.Scan() + + inner := func() { + sc2 := bufio.NewScanner(nil) + _ = sc2.Scan() + } + inner() +} +` + f, err := parser.ParseFile(fset, "mixed.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileScannerBuffer(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Len(t, violations, 1, "only the inner scanner without Buffer() must be flagged") +} + +// --- OpenFileClose rule tests --- + +func TestOpenFileCloseClean(t *testing.T) { + // File opened and closed via defer — must not be flagged. + fset := token.NewFileSet() + src := `package p +import ("context"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return } + defer f.Close() +} +` + f, err := parser.ParseFile(fset, "clean.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Empty(t, violations, "OpenFile with defer f.Close() should not be flagged") +} + +func TestOpenFileCloseHandOff(t *testing.T) { + // File opened, handed off to rc, then rc closed — must not be flagged. + fset := token.NewFileSet() + src := `package p +import ("context"; "io"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc) { + var rc io.ReadCloser + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return } + rc = f + defer rc.Close() +} +` + f, err := parser.ParseFile(fset, "handoff.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Empty(t, violations, "OpenFile handed off to rc and rc closed should not be flagged") +} + +func TestOpenFileCloseMissing(t *testing.T) { + // File opened but never closed — must be flagged. + fset := token.NewFileSet() + src := `package p +import ("context"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return } + _ = f +} +` + f, err := parser.ParseFile(fset, "missing.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Len(t, violations, 1, "OpenFile result not closed must be flagged") +} + +func TestOpenFileCloseReturnTransfer(t *testing.T) { + // File opened then returned to caller — must not be flagged. + fset := token.NewFileSet() + src := `package p +import ("context"; "os"; "io") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func open(callCtx cc) (io.ReadCloser, error) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return nil, err } + return f, nil +} +` + f, err := parser.ParseFile(fset, "return.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Empty(t, violations, "OpenFile result returned to caller should not be flagged") +} diff --git a/allowedsymbols/symbols_test.go b/allowedsymbols/symbols_test.go index 3a1c6342..8407b532 100644 --- a/allowedsymbols/symbols_test.go +++ b/allowedsymbols/symbols_test.go @@ -108,6 +108,10 @@ func checkAllowedSymbols(t *testing.T, cfg allowedSymbolsConfig) { reporter := fileLineReporter(fset, rel, reportErr) localToPath := checkFileImports(f, allowedPkgs, cfg.ExemptImport, reporter) checkFileSelectors(f, localToPath, allowedSyms, usedSymbols, reporter) + + // Structural rules — applied to every checked file. + checkFileScannerBuffer(f, reporter) + checkFileOpenFileClose(f, reporter) } if checked < cfg.MinFiles { diff --git a/builtins/internal/procinfo/procinfo_linux.go b/builtins/internal/procinfo/procinfo_linux.go index 76c61e80..78dc3fde 100644 --- a/builtins/internal/procinfo/procinfo_linux.go +++ b/builtins/internal/procinfo/procinfo_linux.go @@ -247,6 +247,7 @@ func readUID(procPath string, pid int) string { return "?" } scanner := bufio.NewScanner(bytes.NewReader(data)) + scanner.Buffer(make([]byte, 4096), 1<<20) for scanner.Scan() { line := scanner.Text() if strings.HasPrefix(line, "Uid:") { @@ -286,6 +287,7 @@ func procBootTime(procPath string) (int64, error) { } defer f.Close() scanner := bufio.NewScanner(f) + scanner.Buffer(make([]byte, 4096), 1<<20) for scanner.Scan() { line := scanner.Text() if strings.HasPrefix(line, "btime ") { From ce773ba7bddaf8fa0430509ff1ebccf7dd75bff4 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 13:40:22 -0400 Subject: [PATCH 2/5] refactor: rename allowedsymbols/ package and workflow to analysis/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renames the package directory, Go package name, and GitHub Actions workflow from "allowedsymbols" to "analysis" to better reflect that it now enforces both symbol-level import restrictions and structural code patterns (not just allowed symbols). - allowedsymbols/ → analysis/ (package analysis) - .github/workflows/allowed-symbols.yml → analysis.yml - Analyzer Name field: "allowedsymbols" → "analysis" - All internal references updated throughout The GitHub label "verified/allowed_symbols" is unchanged — it is a GitHub-side configuration that cannot be renamed via code. Co-Authored-By: Claude Sonnet 4.6 --- .../{allowed-symbols.yml => analysis.yml} | 14 +++++++------- {allowedsymbols => analysis}/README.md | 4 ++-- {allowedsymbols => analysis}/analyzer.go | 8 ++++---- {allowedsymbols => analysis}/analyzer_test.go | 6 +++--- {allowedsymbols => analysis}/structural.go | 6 +++--- {allowedsymbols => analysis}/structural_test.go | 2 +- .../symbols_allowedpaths.go | 2 +- .../symbols_allowedpaths_test.go | 2 +- .../symbols_allowedpaths_verification_test.go | 2 +- {allowedsymbols => analysis}/symbols_builtins.go | 2 +- .../symbols_builtins_test.go | 2 +- .../symbols_builtins_verification_test.go | 2 +- {allowedsymbols => analysis}/symbols_common.go | 2 +- {allowedsymbols => analysis}/symbols_internal.go | 2 +- {allowedsymbols => analysis}/symbols_interp.go | 2 +- .../symbols_interp_test.go | 2 +- .../symbols_interp_verification_test.go | 2 +- {allowedsymbols => analysis}/symbols_test.go | 4 ++-- .../symbols_verification_test.go | 4 ++-- .../testdata/src/bannedimport/bannedimport.go | 0 .../src/disallowedimport/disallowedimport.go | 0 .../testdata/src/good/good.go | 0 .../testdata/src/unusedsymbol/unusedsymbol.go | 0 builtins/internal/winnet/types.go | 2 +- 24 files changed, 36 insertions(+), 36 deletions(-) rename .github/workflows/{allowed-symbols.yml => analysis.yml} (79%) rename {allowedsymbols => analysis}/README.md (98%) rename {allowedsymbols => analysis}/analyzer.go (96%) rename {allowedsymbols => analysis}/analyzer_test.go (95%) rename {allowedsymbols => analysis}/structural.go (97%) rename {allowedsymbols => analysis}/structural_test.go (99%) rename {allowedsymbols => analysis}/symbols_allowedpaths.go (99%) rename {allowedsymbols => analysis}/symbols_allowedpaths_test.go (98%) rename {allowedsymbols => analysis}/symbols_allowedpaths_verification_test.go (98%) rename {allowedsymbols => analysis}/symbols_builtins.go (99%) rename {allowedsymbols => analysis}/symbols_builtins_test.go (99%) rename {allowedsymbols => analysis}/symbols_builtins_verification_test.go (99%) rename {allowedsymbols => analysis}/symbols_common.go (98%) rename {allowedsymbols => analysis}/symbols_internal.go (99%) rename {allowedsymbols => analysis}/symbols_interp.go (99%) rename {allowedsymbols => analysis}/symbols_interp_test.go (98%) rename {allowedsymbols => analysis}/symbols_interp_verification_test.go (99%) rename {allowedsymbols => analysis}/symbols_test.go (99%) rename {allowedsymbols => analysis}/symbols_verification_test.go (98%) rename {allowedsymbols => analysis}/testdata/src/bannedimport/bannedimport.go (100%) rename {allowedsymbols => analysis}/testdata/src/disallowedimport/disallowedimport.go (100%) rename {allowedsymbols => analysis}/testdata/src/good/good.go (100%) rename {allowedsymbols => analysis}/testdata/src/unusedsymbol/unusedsymbol.go (100%) diff --git a/.github/workflows/allowed-symbols.yml b/.github/workflows/analysis.yml similarity index 79% rename from .github/workflows/allowed-symbols.yml rename to .github/workflows/analysis.yml index 9a341035..56b61018 100644 --- a/.github/workflows/allowed-symbols.yml +++ b/.github/workflows/analysis.yml @@ -1,4 +1,4 @@ -name: Allowed Symbols Verification +name: Static Analysis Verification # Use pull_request_target so the workflow definition always comes from the # base branch (main). This prevents a PR from editing the workflow file to @@ -15,7 +15,7 @@ permissions: jobs: check-allowed-symbols: - name: Allowed Symbols Label Check + name: Static Analysis Label Check runs-on: ubuntu-latest timeout-minutes: 10 steps: @@ -28,17 +28,17 @@ jobs: - name: Fetch PR head run: git fetch origin "refs/pull/${{ github.event.pull_request.number }}/head" - - name: Check for allowedsymbols changes + - name: Check for analysis changes id: changes run: | - changed=$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}" -- 'allowedsymbols/') + changed=$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}" -- 'analysis/') if [ -n "$changed" ]; then echo "modified=true" >> "$GITHUB_OUTPUT" - echo "Changed allowedsymbols files:" + echo "Changed analysis files:" echo "$changed" else echo "modified=false" >> "$GITHUB_OUTPUT" - echo "No allowedsymbols files changed." + echo "No analysis files changed." fi - name: Require verified/allowed_symbols label @@ -49,6 +49,6 @@ jobs: if echo "$PR_LABELS" | jq -e 'index("verified/allowed_symbols")' > /dev/null 2>&1; then echo "Label 'verified/allowed_symbols' is present. Check passed." else - echo "::error::Files in allowedsymbols/ have been modified. A human must review the changes and add the 'verified/allowed_symbols' label to this PR before it can be merged." + echo "::error::Files in analysis/ have been modified. A human must review the changes and add the 'verified/allowed_symbols' label to this PR before it can be merged." exit 1 fi diff --git a/allowedsymbols/README.md b/analysis/README.md similarity index 98% rename from allowedsymbols/README.md rename to analysis/README.md index 12b0a846..baa207ff 100644 --- a/allowedsymbols/README.md +++ b/analysis/README.md @@ -1,4 +1,4 @@ -# allowedsymbols +# analysis Static enforcement of symbol-level import restrictions for the rshell interpreter. @@ -122,4 +122,4 @@ _ = f // flagged: f is never closed 1. Add a line to the appropriate allowlist (and the per-command sublist if it's a builtin). 2. Prefix the comment with the correct safety emoji. -3. Run `go test ./allowedsymbols/` to verify the entry is valid and used. +3. Run `go test ./analysis/` to verify the entry is valid and used. diff --git a/allowedsymbols/analyzer.go b/analysis/analyzer.go similarity index 96% rename from allowedsymbols/analyzer.go rename to analysis/analyzer.go index 6afb25ea..bda6a9fb 100644 --- a/allowedsymbols/analyzer.go +++ b/analysis/analyzer.go @@ -3,14 +3,14 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -// Package allowedsymbols provides a go/analysis.Analyzer that enforces +// Package analysis provides a go/analysis.Analyzer that enforces // symbol-level import restrictions on Go source files. // // The analyzer checks that every imported symbol is in a given allowlist, that // no permanently banned packages are imported, and that every symbol in the // allowlist is actually used. It reports violations with file:line:col // diagnostics. -package allowedsymbols +package analysis import ( "fmt" @@ -42,7 +42,7 @@ type AnalyzerConfig struct { func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer { for _, entry := range cfg.Symbols { if strings.LastIndexByte(entry, '.') <= 0 { - panic(fmt.Sprintf("allowedsymbols.NewAnalyzer: malformed allowlist entry (no dot): %q", entry)) + panic(fmt.Sprintf("analysis.NewAnalyzer: malformed allowlist entry (no dot): %q", entry)) } } @@ -74,7 +74,7 @@ func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer { } return &analysis.Analyzer{ - Name: "allowedsymbols", + Name: "analysis", Doc: "enforces symbol-level import restrictions via an allowlist", Run: run, } diff --git a/allowedsymbols/analyzer_test.go b/analysis/analyzer_test.go similarity index 95% rename from allowedsymbols/analyzer_test.go rename to analysis/analyzer_test.go index e192822c..08110014 100644 --- a/allowedsymbols/analyzer_test.go +++ b/analysis/analyzer_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "testing" @@ -18,8 +18,8 @@ func TestNewAnalyzer_Metadata(t *testing.T) { Symbols: []string{"fmt.Println"}, ListName: "test", }) - if a.Name != "allowedsymbols" { - t.Errorf("Name = %q, want \"allowedsymbols\"", a.Name) + if a.Name != "analysis" { + t.Errorf("Name = %q, want \"analysis\"", a.Name) } if len(a.Requires) != 0 { t.Errorf("Requires has %d entries, want 0", len(a.Requires)) diff --git a/allowedsymbols/structural.go b/analysis/structural.go similarity index 97% rename from allowedsymbols/structural.go rename to analysis/structural.go index 99b5276c..1aacefb9 100644 --- a/allowedsymbols/structural.go +++ b/analysis/structural.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "go/ast" @@ -89,7 +89,7 @@ func checkFileScannerBuffer(f *ast.File, report func(pos token.Pos, format strin for _, sc := range scanners { if !buffered[sc.name] { report(sc.pos, - "bufio.NewScanner result %q must have .Buffer() called to cap the maximum line size (see allowedsymbols/README.md §Structural Rules)", + "bufio.NewScanner result %q must have .Buffer() called to cap the maximum line size (see analysis/README.md §Structural Rules)", sc.name) } } @@ -163,7 +163,7 @@ func checkFileOpenFileClose(f *ast.File, report func(pos token.Pos, format strin for _, ov := range opens { if !isClosedTransitive(ov.name, closed, handOff) && !returned[ov.name] { report(ov.pos, - "OpenFile result %q must be closed via defer or explicit Close() call (see allowedsymbols/README.md §Structural Rules)", + "OpenFile result %q must be closed via defer or explicit Close() call (see analysis/README.md §Structural Rules)", ov.name) } } diff --git a/allowedsymbols/structural_test.go b/analysis/structural_test.go similarity index 99% rename from allowedsymbols/structural_test.go rename to analysis/structural_test.go index 40d87cb2..4f33318f 100644 --- a/allowedsymbols/structural_test.go +++ b/analysis/structural_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "go/parser" diff --git a/allowedsymbols/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go similarity index 99% rename from allowedsymbols/symbols_allowedpaths.go rename to analysis/symbols_allowedpaths.go index cfd69932..aac1dd5a 100644 --- a/allowedsymbols/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis // allowedpathsAllowedSymbols lists every "importpath.Symbol" that may be used // by non-test Go files in allowedpaths/. Each entry must be in diff --git a/allowedsymbols/symbols_allowedpaths_test.go b/analysis/symbols_allowedpaths_test.go similarity index 98% rename from allowedsymbols/symbols_allowedpaths_test.go rename to analysis/symbols_allowedpaths_test.go index 68efa415..bf552fd2 100644 --- a/allowedsymbols/symbols_allowedpaths_test.go +++ b/analysis/symbols_allowedpaths_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "strings" diff --git a/allowedsymbols/symbols_allowedpaths_verification_test.go b/analysis/symbols_allowedpaths_verification_test.go similarity index 98% rename from allowedsymbols/symbols_allowedpaths_verification_test.go rename to analysis/symbols_allowedpaths_verification_test.go index fa6591c0..e994e0d1 100644 --- a/allowedsymbols/symbols_allowedpaths_verification_test.go +++ b/analysis/symbols_allowedpaths_verification_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "path/filepath" diff --git a/allowedsymbols/symbols_builtins.go b/analysis/symbols_builtins.go similarity index 99% rename from allowedsymbols/symbols_builtins.go rename to analysis/symbols_builtins.go index 707125db..bd063a52 100644 --- a/allowedsymbols/symbols_builtins.go +++ b/analysis/symbols_builtins.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis // builtinAllowedSymbols lists every "importpath.Symbol" that may be used by // command implementation files in builtins/. Each entry must be in diff --git a/allowedsymbols/symbols_builtins_test.go b/analysis/symbols_builtins_test.go similarity index 99% rename from allowedsymbols/symbols_builtins_test.go rename to analysis/symbols_builtins_test.go index 3ff58e5c..574cf0e1 100644 --- a/allowedsymbols/symbols_builtins_test.go +++ b/analysis/symbols_builtins_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "strings" diff --git a/allowedsymbols/symbols_builtins_verification_test.go b/analysis/symbols_builtins_verification_test.go similarity index 99% rename from allowedsymbols/symbols_builtins_verification_test.go rename to analysis/symbols_builtins_verification_test.go index cdf8e090..a56f1a0d 100644 --- a/allowedsymbols/symbols_builtins_verification_test.go +++ b/analysis/symbols_builtins_verification_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "path/filepath" diff --git a/allowedsymbols/symbols_common.go b/analysis/symbols_common.go similarity index 98% rename from allowedsymbols/symbols_common.go rename to analysis/symbols_common.go index 2fde4e0b..3fe80204 100644 --- a/allowedsymbols/symbols_common.go +++ b/analysis/symbols_common.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis // permanentlyBanned lists packages that may never be imported, // regardless of what symbols they export. diff --git a/allowedsymbols/symbols_internal.go b/analysis/symbols_internal.go similarity index 99% rename from allowedsymbols/symbols_internal.go rename to analysis/symbols_internal.go index 9237cc24..0b73ca0a 100644 --- a/allowedsymbols/symbols_internal.go +++ b/analysis/symbols_internal.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis // internalPerPackageSymbols maps each builtins/internal/ name to the // symbols it is allowed to use. Every symbol listed here must also appear in diff --git a/allowedsymbols/symbols_interp.go b/analysis/symbols_interp.go similarity index 99% rename from allowedsymbols/symbols_interp.go rename to analysis/symbols_interp.go index 5960185b..9bc1973a 100644 --- a/allowedsymbols/symbols_interp.go +++ b/analysis/symbols_interp.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis // interpAllowedSymbols lists every "importpath.Symbol" that may be used by // non-test Go files in interp/. Each entry must be in "importpath.Symbol" diff --git a/allowedsymbols/symbols_interp_test.go b/analysis/symbols_interp_test.go similarity index 98% rename from allowedsymbols/symbols_interp_test.go rename to analysis/symbols_interp_test.go index f5190043..49c72f94 100644 --- a/allowedsymbols/symbols_interp_test.go +++ b/analysis/symbols_interp_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "strings" diff --git a/allowedsymbols/symbols_interp_verification_test.go b/analysis/symbols_interp_verification_test.go similarity index 99% rename from allowedsymbols/symbols_interp_verification_test.go rename to analysis/symbols_interp_verification_test.go index 8b0152c3..84a5e5cf 100644 --- a/allowedsymbols/symbols_interp_verification_test.go +++ b/analysis/symbols_interp_verification_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "path/filepath" diff --git a/allowedsymbols/symbols_test.go b/analysis/symbols_test.go similarity index 99% rename from allowedsymbols/symbols_test.go rename to analysis/symbols_test.go index 8407b532..6fb8789a 100644 --- a/allowedsymbols/symbols_test.go +++ b/analysis/symbols_test.go @@ -3,7 +3,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2026-present Datadog, Inc. -package allowedsymbols +package analysis import ( "fmt" @@ -78,7 +78,7 @@ func checkAllowedSymbols(t *testing.T, cfg allowedSymbolsConfig) { if cfg.RepoRootOverride != "" { root = cfg.RepoRootOverride } else { - // This package lives in allowedsymbols/, so the repo root is one level up. + // This package lives in analysis/, so the repo root is one level up. dir, err := os.Getwd() if err != nil { t.Fatal(err) diff --git a/allowedsymbols/symbols_verification_test.go b/analysis/symbols_verification_test.go similarity index 98% rename from allowedsymbols/symbols_verification_test.go rename to analysis/symbols_verification_test.go index 72bf9476..8ac0c7a7 100644 --- a/allowedsymbols/symbols_verification_test.go +++ b/analysis/symbols_verification_test.go @@ -11,7 +11,7 @@ // repetitive plumbing (copying trees, rewriting Go source, locating files) so // each per-config test file stays focused on the specific violation it tests. -package allowedsymbols +package analysis import ( "io" @@ -64,7 +64,7 @@ func copyFile(src, dst string) error { } // repoRoot returns the repo root by going one level up from the test's working -// directory (allowedsymbols/). +// directory (analysis/). func repoRoot(t *testing.T) string { t.Helper() dir, err := os.Getwd() diff --git a/allowedsymbols/testdata/src/bannedimport/bannedimport.go b/analysis/testdata/src/bannedimport/bannedimport.go similarity index 100% rename from allowedsymbols/testdata/src/bannedimport/bannedimport.go rename to analysis/testdata/src/bannedimport/bannedimport.go diff --git a/allowedsymbols/testdata/src/disallowedimport/disallowedimport.go b/analysis/testdata/src/disallowedimport/disallowedimport.go similarity index 100% rename from allowedsymbols/testdata/src/disallowedimport/disallowedimport.go rename to analysis/testdata/src/disallowedimport/disallowedimport.go diff --git a/allowedsymbols/testdata/src/good/good.go b/analysis/testdata/src/good/good.go similarity index 100% rename from allowedsymbols/testdata/src/good/good.go rename to analysis/testdata/src/good/good.go diff --git a/allowedsymbols/testdata/src/unusedsymbol/unusedsymbol.go b/analysis/testdata/src/unusedsymbol/unusedsymbol.go similarity index 100% rename from allowedsymbols/testdata/src/unusedsymbol/unusedsymbol.go rename to analysis/testdata/src/unusedsymbol/unusedsymbol.go diff --git a/builtins/internal/winnet/types.go b/builtins/internal/winnet/types.go index 175159bc..40c9d5ac 100644 --- a/builtins/internal/winnet/types.go +++ b/builtins/internal/winnet/types.go @@ -5,7 +5,7 @@ // Package winnet provides socket enumeration for Windows via iphlpapi.dll. // It is placed in builtins/internal/ to isolate the unsafe.Pointer DLL call -// from the allowedsymbols checker, which cannot evaluate build tags. +// from the analysis checker, which cannot evaluate build tags. package winnet // SocketEntry holds the parsed fields for one Windows socket. From c19d369882e8a57f45a52b463d8222a59cffd4a8 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 13:41:45 -0400 Subject: [PATCH 3/5] chore(analysis): accept verified/analysis or verified/allowed_symbols label Adds the new verified/analysis label as the canonical gate label, while keeping verified/allowed_symbols accepted for backwards compatibility with in-flight PRs. Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/analysis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/analysis.yml b/.github/workflows/analysis.yml index 56b61018..c4e34c70 100644 --- a/.github/workflows/analysis.yml +++ b/.github/workflows/analysis.yml @@ -46,9 +46,9 @@ jobs: env: PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} run: | - if echo "$PR_LABELS" | jq -e 'index("verified/allowed_symbols")' > /dev/null 2>&1; then - echo "Label 'verified/allowed_symbols' is present. Check passed." + if echo "$PR_LABELS" | jq -e 'index("verified/analysis") // index("verified/allowed_symbols")' > /dev/null 2>&1; then + echo "Label present. Check passed." else - echo "::error::Files in analysis/ have been modified. A human must review the changes and add the 'verified/allowed_symbols' label to this PR before it can be merged." + echo "::error::Files in analysis/ have been modified. A human must review the changes and add the 'verified/analysis' label to this PR before it can be merged." exit 1 fi From 48001ae4ff1f498b9a7941bcd8da34ca072d78f8 Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Fri, 27 Mar 2026 15:18:36 -0400 Subject: [PATCH 4/5] fix(analysis): address code review findings from PR #162 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2: make isClosedTransitive truly transitive via BFS walk so that multi-hop hand-off chains (f → a → b → Close()) are correctly detected instead of only checking one level deep. P1/P2: document path-insensitivity limitations with comments in structural.go and dedicated Known-Limitation tests in structural_test.go: - returned-map false negative: a variable returned on any branch exempts it globally (path-sensitive CFG analysis would be needed to fix) - variable name reuse false negative: re-using the same name for two successive OpenFile results can cause one leak to go undetected P2: update NewAnalyzer godoc to note that ScannerBufferAnalyzer and OpenFileCloseAnalyzer must also be registered for full coverage. P3: remove dead collectViolations helper (silently dropped format args); add TestOpenFileCloseChainedHandOff to exercise the now-fixed BFS walk. Co-Authored-By: Claude Sonnet 4.6 --- analysis/analyzer.go | 6 ++ analysis/structural.go | 45 ++++++++++---- analysis/structural_test.go | 113 +++++++++++++++++++++++++++++------- 3 files changed, 133 insertions(+), 31 deletions(-) diff --git a/analysis/analyzer.go b/analysis/analyzer.go index bda6a9fb..c4bbbfc8 100644 --- a/analysis/analyzer.go +++ b/analysis/analyzer.go @@ -39,6 +39,12 @@ type AnalyzerConfig struct { // // NewAnalyzer panics if any entry in cfg.Symbols is malformed (no dot // separator), matching the behaviour of the test-harness variant. +// +// NOTE: This analyzer only enforces symbol-level allowlist restrictions. For +// full static analysis coverage, callers should also register +// ScannerBufferAnalyzer and OpenFileCloseAnalyzer alongside this one. The +// test-harness path (checkAllowedSymbols) already applies all three checks +// automatically. func NewAnalyzer(cfg AnalyzerConfig) *analysis.Analyzer { for _, entry := range cfg.Symbols { if strings.LastIndexByte(entry, '.') <= 0 { diff --git a/analysis/structural.go b/analysis/structural.go index 1aacefb9..3444fd81 100644 --- a/analysis/structural.go +++ b/analysis/structural.go @@ -110,14 +110,25 @@ func checkFileOpenFileClose(f *ast.File, report func(pos token.Pos, format strin pos token.Pos name string } + // NOTE: opens is keyed on variable name as a string. If the same name is + // reused for two successive OpenFile calls without closing the first, a + // single Close() call will satisfy both entries and the second leak will + // not be flagged. Use distinct variable names for successive file opens + // in the same scope to ensure reliable detection. var opens []openVar closed := make(map[string]bool) // handOff maps a "holder" variable name to the original variable it was // assigned from. Closing the holder counts as closing the original. handOff := make(map[string]string) // holder → original - // returned tracks variables that appear in a return statement. Returning - // a file handle transfers ownership to the caller, so no Close() is - // required in the current scope. + // returned tracks variables that appear in any return statement. + // Returning a file handle transfers ownership to the caller, so no + // Close() is required in the current scope. + // + // NOTE: This check is path-insensitive. If a variable appears in a + // return statement on one branch (e.g. the happy path) but is leaked on + // another branch (e.g. a subsequent early-return), this checker will NOT + // flag it. Full path-sensitive analysis requires CFG-based data flow, + // which is beyond the scope of this AST-only checker. returned := make(map[string]bool) inspectBody(body, func(n ast.Node) { @@ -170,16 +181,30 @@ func checkFileOpenFileClose(f *ast.File, report func(pos token.Pos, format strin }) } -// isClosedTransitive returns true if name is closed directly or if a variable -// that name was handed off to is closed. +// isClosedTransitive returns true if name is closed directly or transitively +// through a chain of hand-off assignments. It performs a breadth-first search +// over the handOff graph to handle chains of arbitrary depth, e.g.: +// +// f → a → b → Close() (handOff["a"]="f", handOff["b"]="a", closed["b"]=true) func isClosedTransitive(name string, closed map[string]bool, handOff map[string]string) bool { - if closed[name] { - return true - } - for holder, orig := range handOff { - if orig == name && closed[holder] { + visited := make(map[string]bool) + queue := []string{name} + for len(queue) > 0 { + cur := queue[0] + queue = queue[1:] + if visited[cur] { + continue + } + visited[cur] = true + if closed[cur] { return true } + // Enqueue all variables that were assigned from cur. + for holder, orig := range handOff { + if orig == cur && !visited[holder] { + queue = append(queue, holder) + } + } } return false } diff --git a/analysis/structural_test.go b/analysis/structural_test.go index 4f33318f..f79f3014 100644 --- a/analysis/structural_test.go +++ b/analysis/structural_test.go @@ -42,27 +42,6 @@ func run(callCtx fakeCtx) { return fset, f } -// collectViolations runs fn against a parsed snippet and returns all reported -// violation messages. -func collectViolations(t *testing.T, body string, fn func(f interface{ Pos() token.Pos }, report func(token.Pos, string, ...any))) []string { - t.Helper() - _, f := parseSnippet(t, body) - - type astFile interface { - Pos() token.Pos - } - - var msgs []string - fn(f, func(_ token.Pos, format string, args ...any) { - msg := format - if len(args) > 0 { - msg = format // simplified: just collect format for test assertions - } - msgs = append(msgs, msg) - }) - return msgs -} - // --- ScannerBuffer rule tests --- func TestScannerBufferClean(t *testing.T) { @@ -251,3 +230,95 @@ func open(callCtx cc) (io.ReadCloser, error) { }) assert.Empty(t, violations, "OpenFile result returned to caller should not be flagged") } + +func TestOpenFileCloseChainedHandOff(t *testing.T) { + // f → a → b → Close(): two-hop chain must be detected. + fset := token.NewFileSet() + src := `package p +import ("context"; "io"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return } + var a io.ReadCloser + a = f + var b io.ReadCloser + b = a + defer b.Close() +} +` + f, err := parser.ParseFile(fset, "chain.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + assert.Empty(t, violations, "two-hop hand-off chain closed at end must not be flagged") +} + +// TestOpenFileCloseKnownLimitationMultiBranchReturn documents a known +// path-insensitivity limitation: if a variable appears in a return statement +// on any branch, the checker treats the whole variable as "returned" and will +// not flag it even if another branch leaks it. Fixing this requires CFG-based +// data-flow analysis beyond the scope of this AST-only checker. +func TestOpenFileCloseKnownLimitationMultiBranchReturn(t *testing.T) { + fset := token.NewFileSet() + src := `package p +import ("context"; "io"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc, cond bool) (io.ReadCloser, error) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return nil, err } + if cond { + return f, nil // f is returned here + } + // f is leaked here (no close, no return) — NOT caught due to path-insensitivity + return nil, nil +} +` + f, err := parser.ParseFile(fset, "multibranch.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + // Known false negative: the checker sees "f" in a return statement and + // exempts it globally. This test documents the gap rather than asserting + // correct behaviour. + assert.Empty(t, violations, "known limitation: path-insensitive return exemption produces false negative") +} + +// TestOpenFileCloseKnownLimitationNameReuse documents a known limitation: +// if the same variable name is reused for two successive OpenFile results, +// a single Close() satisfies both entries in the checker. +func TestOpenFileCloseKnownLimitationNameReuse(t *testing.T) { + fset := token.NewFileSet() + src := `package p +import ("context"; "os") +type cc struct{} +func (cc) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { return nil, nil } +func run(callCtx cc) { + f, err := callCtx.OpenFile(context.Background(), "x", os.O_RDONLY, 0) + if err != nil { return } + defer f.Close() + // Re-using f for a second OpenFile — the second handle is never closed. + f, err = callCtx.OpenFile(context.Background(), "y", os.O_RDONLY, 0) + if err != nil { return } + _ = f +} +` + f, err := parser.ParseFile(fset, "reuse.go", src, 0) + require.NoError(t, err) + + var violations []string + checkFileOpenFileClose(f, func(_ token.Pos, format string, _ ...any) { + violations = append(violations, format) + }) + // Known false negative: closed["f"]=true from the first defer satisfies + // both opens entries. Use distinct variable names in production code. + assert.Empty(t, violations, "known limitation: name reuse produces false negative") +} From 1954beb2768f33d0bd4088c66da5e2d99b40ad9a Mon Sep 17 00:00:00 2001 From: Travis Thieman Date: Mon, 30 Mar 2026 10:16:44 -0400 Subject: [PATCH 5/5] chore(analysis): remove dead parseSnippet helper; rename workflow job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unused `parseSnippet` function from structural_test.go - Rename workflow job `check-allowed-symbols` → `check-analysis` and step "Require verified/allowed_symbols label" → "Require verified/analysis label" to match the allowedsymbols/ → analysis/ package rename Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/analysis.yml | 4 ++-- analysis/structural_test.go | 28 ---------------------------- 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/.github/workflows/analysis.yml b/.github/workflows/analysis.yml index c4e34c70..c0f0241e 100644 --- a/.github/workflows/analysis.yml +++ b/.github/workflows/analysis.yml @@ -14,7 +14,7 @@ permissions: pull-requests: read jobs: - check-allowed-symbols: + check-analysis: name: Static Analysis Label Check runs-on: ubuntu-latest timeout-minutes: 10 @@ -41,7 +41,7 @@ jobs: echo "No analysis files changed." fi - - name: Require verified/allowed_symbols label + - name: Require verified/analysis label if: steps.changes.outputs.modified == 'true' env: PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} diff --git a/analysis/structural_test.go b/analysis/structural_test.go index f79f3014..69758e16 100644 --- a/analysis/structural_test.go +++ b/analysis/structural_test.go @@ -14,34 +14,6 @@ import ( "github.com/stretchr/testify/require" ) -// parseSnippet parses a minimal Go source file containing the given body -// inserted into a function named "f". The returned file can be passed to -// checkFileScannerBuffer or checkFileOpenFileClose. -func parseSnippet(t *testing.T, body string) (*token.FileSet, interface{ Pos() token.Pos }) { - t.Helper() - src := `package p - -import ( - "bufio" - "context" - "os" -) - -type fakeCtx struct{} -func (fakeCtx) OpenFile(ctx context.Context, path string, flags int, mode os.FileMode) (interface{ Read([]byte)(int,error); Close() error }, error) { - return nil, nil -} - -func run(callCtx fakeCtx) { -` + body + ` -} -` - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, "snippet.go", src, 0) - require.NoError(t, err) - return fset, f -} - // --- ScannerBuffer rule tests --- func TestScannerBufferClean(t *testing.T) {