Skip to content

httprespbodyclose: FuncDecl-only scope + FuncLit skip misses responses closed inside closures (goroutines/handlers) — false nega [Content truncated due to length] #43465

Description

@github-actions

Summary

The new httprespbodyclose analyzer never inspects response handling that is fully contained inside a function literal (closure). Its node filter registers only *ast.FuncDecl, and its inner walk explicitly bails at every *ast.FuncLit, so a resp that is both assigned and manually closed inside the same closure is entirely invisible to the pass. This re-introduces the exact bug class that was already fixed for ctxbackground in #41164 (closed).

Location

pkg/linters/httprespbodyclose/httprespbodyclose.go

  • Node filter is FuncDecl-only:
// httprespbodyclose.go:34-36
nodeFilter := []ast.Node{
	(*ast.FuncDecl)(nil),
}
  • The inner walk returns early for every FuncLit, so ast.Inspect never descends into a closure body:
// httprespbodyclose.go:75-77
if _, ok := node.(*ast.FuncLit); ok {
	return false
}

Because respVars is populated only while walking the enclosing FuncDecl body (and that walk stops at any FuncLit), a response whose assignment and manual close both live inside a closure is never tracked → no diagnostic.

Failure scenario (missed true positive)

The canonical HTTP-in-closure leak pattern — the very place resource leaks bite hardest (goroutines, http.HandleFunc, errgroup.Go) — escapes the linter:

func register(mux *http.ServeMux, client *http.Client, req *http.Request) {
    mux.HandleFunc("/x", func(w http.ResponseWriter, r *http.Request) {
        resp, err := client.Do(req) // assigned INSIDE the closure
        if err != nil {
            return
        }
        io.Copy(io.Discard, resp.Body)
        resp.Body.Close()           // manual, non-deferred — should be flagged, but is NOT
    })
}

If this exact body were a top-level func, it is flagged (see testdata BadManualClose). Moved verbatim into a closure, it is silently ignored.

Why the existing testdata does not cover this

testdata/src/httprespbodyclose/httprespbodyclose.go has GoodCloseInsideClosure (lines 34-43), but that case assigns resp in the outer scope and closes it inside a go func(){} — a defensible deliberate skip (async close timing). It does not exercise the assigned-and-closed-within-the-same-closure case, which is structurally identical to a plain function body and has no false-positive risk to justify skipping.

Precedent

httprespbodyclose is the only recent linter that omits FuncLit.

Recommended fix

Mirror seenmapbool (pkg/linters/seenmapbool/seenmapbool.go:34-58): add (*ast.FuncLit)(nil) to the node filter, type-switch to extract each function body, and run the per-scope respVars walk on both FuncDecl and FuncLit bodies. The existing FuncLit early-return at line 75 should be kept — it stops the inner ast.Inspect at nested closures so each scope is analyzed exactly once, which also avoids the double-report pitfall seen in seenmapbool (#aw_sg44a1). Test-file skip currently lives on the FuncDecl path (line 52); it must be preserved (a nested FuncLit inherits the enclosing file, so gate on the enclosing FuncDecl's filename or re-check per top-level node).

Validation checklist

  • Add testdata: response assigned and manually closed inside a func(){}() / go func(){}() / handler closure → expect a want diagnostic.
  • Add testdata: response assigned and correctly defer-closed inside a closure → expect no diagnostic (guards against a new FP).
  • Keep GoodCloseInsideClosure (assigned-outer / closed-inner) unflagged.
  • Confirm no double-reporting for a single-scope violation after adding FuncLit to the filter.

Effort

Small — ~15-20 lines, one-file change with a direct seenmapbool template and existing testdata harness.

Generated by 🤖 Sergo - Serena Go Expert · 336.6 AIC · ⌖ 13.7 AIC · ⊞ 5.9K ·

  • expires on Jul 11, 2026, 9:14 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

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