feat(allowedsymbols): add structural AST rules for Scanner.Buffer and OpenFile.Close

[thieman]

Summary

• Adds two structural AST rules enforced on every file checked by checkAllowedSymbols, plus as standalone go/analysis analyzers (ScannerBufferAnalyzer, OpenFileCloseAnalyzer)
• Rule 1: every bufio.NewScanner result must have .Buffer() called in the same function scope — prevents silent truncation on lines > 64 KiB
• Rule 2: every .OpenFile(...) result must be closed via defer or explicit .Close() — prevents fd exhaustion over repeated script executions; understands hand-off (rc = f; defer rc.Close()) and return-ownership-transfer (return f, nil) patterns
• Fixes two genuine violations caught by the new rules in builtins/internal/procinfo/procinfo_linux.go
• Documents both rules in allowedsymbols/README.md §Structural Rules

Test plan

• go test ./allowedsymbols/ — all unit tests pass including new TestScannerBuffer* and TestOpenFileClose* tests
• go test ./interp/... — no regressions
• Verify false-positive cases: hand-off pattern, return ownership transfer
• CI green

🤖 Generated with Claude Code

[thieman]

Security-Focused Code Review — PR #162

Reviewer == PR author; using COMMENT event per review policy.

This PR adds two structural AST rules (checkFileScannerBuffer, checkFileOpenFileClose) to the analysis package and fixes two genuine violations in builtins/internal/procinfo/procinfo_linux.go. The rename of allowedsymbols/ → analysis/ is clean and mechanical. The structural checker concept is sound and fills a real gap. The findings below are about correctness holes in the checker logic that can produce false negatives.

---

Summary of Findings

Sev	Area	Finding
P1	Correctness / Security	returned map: false negative when f is returned in one branch but leaked in another
P2	Correctness	isClosedTransitive only one hop deep — chained hand-offs are missed
P2	Correctness	Variable name reuse (same name assigned twice via := or =) silently satisfies the earlier close check
P2	Integration	NewAnalyzer (standalone go/analysis path) does NOT invoke the structural rules
P3	Test Coverage	No test for multi-branch return (returned in one branch, leaked in another)
P3	Test Coverage	No test for chained hand-off (a=f; b=a; defer b.Close())
P3	Test Coverage	No test for variable name reuse (f reassigned via second :=)
P3	Test Coverage	collectViolations silently drops format args — only violation counts are checkable

---

The procinfo_linux.go fixes

Both .Buffer() additions are correct: they cap the scanner at 1 MiB (matching the project-wide convention) and are placed immediately after bufio.NewScanner(...), before the first Scan() call. No issues.

forEachFuncBody + inspectBody correctness

forEachFuncBody correctly enumerates every scope: ast.Inspect visits FuncDecl and FuncLit nodes and calls fn for each independently. inspectBody stops at nested FuncLit boundaries (return false), so inner scopes do not bleed into the outer check. Each scope is visited exactly once. The design is correct.

NewAnalyzer gap (P2 — discussed in review body due to few changed lines in that file)

NewAnalyzer in analyzer.go returns a standalone go/analysis.Analyzer for use with go vet/gopls. Its run closure calls only checkFileImports and checkFileSelectors — it does not call checkFileScannerBuffer or checkFileOpenFileClose. Structural rules are therefore absent when NewAnalyzer is used as a standalone analyzer. The ScannerBufferAnalyzer and OpenFileCloseAnalyzer vars are exported but have no registration point in this repo (no cmd/, no CI step invoking them). Currently the test-harness path in checkAllowedSymbols is the only place structural rules run in CI, which is sufficient for now — but this should be documented in the NewAnalyzer godoc so future integrators know they must register both structural analyzers separately.

[astuyve]

I think just remove the dead method and buffers and we're good

[astuyve]

I think you're already reading the whole file into memory with os.ReadFile so this is just an extra allocation, no?

[thieman]

[Claude Sonnet 4.6] Good catch on the readUID case — you're right that os.ReadFile already loads everything into memory, so the buffer cap is technically redundant there. However, the checkFileScannerBuffer structural rule added by this PR enforces that all bufio.NewScanner results have .Buffer() called, regardless of the reader type. Removing it would cause our own checker to fail CI. I've left both calls in place for consistency with the rule, and the rule itself doesn't distinguish between file-backed and in-memory readers.

[astuyve]

same here

[thieman]

[Claude Sonnet 4.6] Same reasoning applies — the checkFileScannerBuffer rule requires the call. For procBootTime specifically the buffer cap is also useful since it reads from a real file handle, not an in-memory reader.