Skip to content

bufferresetbeforereuse: per-block state isolation misses reuse across if/for/switch boundaries #60170

Description

@github-actions

Summary

bufferresetbeforereuse (the newest, 68th registered analyzer -- pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go) is supposed to flag bytes.Buffer/strings.Builder reuse (write, then read via String()/Bytes()/Len(), then write again) without an intervening Reset(). The detector never links a write/read pair in an outer block with a later write inside a nested if/for/switch/select block, so the exact pattern the linter is designed to catch escapes detection whenever it spans a control-flow boundary -- arguably the most common real-world shape of this bug.

Root cause

  • analyzeBlockForBufferReuse (bufferresetbeforereuse.go:74-84) walks the whole function body with ast.Inspect and calls analyzeStraightLineBlock once per BlockStmt node it encounters (the function body itself, plus every nested if/for/switch body, since ast.Inspects default return true lets it descend into those nested blocks).
  • analyzeStraightLineBlock (bufferresetbeforereuse.go:86-130) creates brand-new written/read maps on each call (line 92-93) and calls collectEvents, which explicitly stops descent at IfStmt/ForStmt/RangeStmt/SwitchStmt/TypeSwitchStmt/SelectStmt/nested BlockStmt (collectEvents, line 133-155, the return false cases at line 137-139).
  • Net effect: each blocks write/read/reset history is analyzed in total isolation from its enclosing block. A write in the outer scope, followed by a read, followed by a reuse-write inside a nested if/for block, is invisible to the linter because the inner blocks local written/read maps start empty -- the reuse-write is treated as if it were the first write ever seen.

Reproduction (not in testdata)

func reuseCrossIf(cond bool) {
    var buf bytes.Buffer
    buf.WriteString("first")
    _ = buf.String()
    if cond {
        buf.WriteString("second") // reused without Reset -- NOT flagged
    }
}

Trace: the outer blocks collectEvents treats the whole IfStmt as opaque (stops before descending into it), so it only sees write, read for buf at the outer level -- no violation (correct, in isolation). Separately, analyzeBlockForBufferReuses outer ast.Inspect also visits the ifs BlockStmt body directly and re-invokes analyzeStraightLineBlock on it with fresh maps; that isolated call sees only buf.WriteString("second"), a first write with no prior read recorded, so it is never reported. The same escape applies to for/switch/select bodies.

Impact

This is a false negative on the linters core, advertised purpose. The existing testdata (testdata/src/a/a.go) only tests same-block reuse and one true-negative branch case (okMutuallyExclusiveBranches, an early-return before the reuse point) -- there is no test at all for the (very common) case where a write+read happens before a conditional/loop and the buffer is written again inside it. Since the analyzer is newly registered and not yet in cgo.yml LINTER_FLAGS, this is latent rather than a live CI gap, but the gap will persist invisibly once enforcement is turned on.

Recommendation

Track write/read/reset state per-variable across the whole function scope (not per-BlockStmt), the same way the existing resourcetracker framework (used by fileclosenotdeferred/contextcancelnotdeferred/manualmutexunlock) tracks acquisitions across a FuncDecl, only breaking at FuncLit boundaries (which bufferresetbeforereuse already does correctly at the top level). A single ast.Inspect per FuncDecl/FuncLit that accumulates one written/read map for the whole function (still stopping only at FuncLit, not at every control-flow block) would catch the cross-block case while keeping the existing single-block tests passing.

Validation checklist

  • Add a testdata case reproducing reuseCrossIf above (write+read outside, reuse-write inside if) and confirm it currently passes silently, then add a want comment after the fix
  • Add a for-loop-body variant (write+read before the loop, reuse-write inside the loop) to same testdata
  • Verify okMutuallyExclusiveBranches (early-return case) still reports no diagnostic after the fix
  • Confirm existing same-block cases (notOkReuseWithoutReset, etc.) still report correctly

Effort: small-medium -- the state-tracking restructure is localized to analyzeBlockForBufferReuse/analyzeStraightLineBlock/collectEvents; no changes needed to run, checkBufferReuse, or the object-identification helpers.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • api.anthropic.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "api.anthropic.com"

See Network Configuration for more information.

Generated by 🤖 Sergo - Serena Go Expert · claude · agent · 252.5 AIC · ⌖ 6.37 AIC · ⊞ 6.8K ·

  • expires on Sep 17, 2026, 7:58 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    cookieIssue Monster Loves Cookies!sergo

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions