diff --git a/lint/lockio/lockio_test.go b/lint/lockio/lockio_test.go index e086d59f..ba1492ad 100644 --- a/lint/lockio/lockio_test.go +++ b/lint/lockio/lockio_test.go @@ -164,6 +164,37 @@ func (a *App) badForInitAcquireLeaksInBody() error { } } +// TestForLoopPostReleaseDoesNotMaskFirstIterationLeak is a regression guard +// for a real gap found in quality review of a downstream backport of this +// exact ForStmt case (workflow-compute's local mutation-lifecycle guard +// test): unlike Init/Cond (and RangeStmt's X, SwitchStmt's Tag, +// TypeSwitchStmt's Assign — all of which execute unconditionally before any +// entry into their body), a for-loop's Post clause only runs after a +// completed, non-returning iteration — never before the body's first entry. +// Folding a release call found in Post into releaseSeen before recursing +// into Body made the checker believe the body's first-iteration return path +// was already covered when it never is on that iteration — a demonstrated +// false negative in the one direction this checker's design explicitly +// favors avoiding (over-flagging is acceptable, under-flagging is not). +func TestForLoopPostReleaseDoesNotMaskFirstIterationLeak(t *testing.T) { + fset, f := parseFixture(t, ` +func (a *App) badForPostReleaseMasksFirstIterationLeak() error { + for i := 0; i < 3; a.Commit(0) { + a.AcquireLease() + if somethingWrong() { + return errors.New("nope") + } + i++ + } + return nil +} +`) + violations := FindViolations(fset, []*ast.File{f}, testConfig()) + if !hasViolation(violations, "badForPostReleaseMasksFirstIterationLeak", ClassUncoveredReturnPath) { + t.Fatalf("checker bug: a release call in a for-loop's Post clause masked a genuine leak on the body's first-iteration return path; violations=%+v", violations) + } +} + func TestSwitchHeaderAcquireIsDetectedInBody(t *testing.T) { fset, f := parseFixture(t, ` func (a *App) badSwitchInitAcquireLeaksInBody() error { diff --git a/lint/lockio/walk.go b/lint/lockio/walk.go index 2f2a3204..f5aa60cf 100644 --- a/lint/lockio/walk.go +++ b/lint/lockio/walk.go @@ -192,11 +192,24 @@ func (w *returnPathWalker) walkBlock(stmts []ast.Stmt, sawAcquire, releaseSeen b case *ast.BlockStmt: w.walkBlock(s.List, sawAcquire, releaseSeen) case *ast.ForStmt: - // Init/Cond/Post execute unconditionally on reaching this - // statement (Init and Cond at least once; Post only after a - // completed iteration, but a call there is still reachable - // before any return inside Body), so — like IfStmt's Init/Cond — - // they extend the running state for the loop body. + // Init and Cond execute unconditionally before the body's first + // iteration, so — like IfStmt's Init/Cond — they extend the + // running state for the loop body soundly in both directions + // (sawAcquire and releaseSeen). + // + // Post is different and needs asymmetric treatment: it runs + // only after a completed, non-returning iteration — never + // before the body's first entry. Folding sawAcquire from Post + // is still safe (it can only make the checker more suspicious + // of an unqualified return, matching this checker's + // over-flag-over-under-flag design), but folding releaseSeen + // from Post is unsound: a release call written in Post would + // make the checker believe the body's first-iteration return + // path is already covered when it never actually is on that + // iteration, masking a genuine leak (a real gap found in + // quality review of a downstream backport of this exact case — + // see TestForLoopPostReleaseDoesNotMaskFirstIterationLeak). So + // Post contributes to sawAcquire only. if s.Init != nil { sawAcquire = sawAcquire || nodeContainsCall(s.Init, w.cfg.isAcquire) releaseSeen = releaseSeen || nodeContainsSyncCall(s.Init, w.cfg.isRelease) @@ -205,7 +218,6 @@ func (w *returnPathWalker) walkBlock(stmts []ast.Stmt, sawAcquire, releaseSeen b releaseSeen = releaseSeen || nodeContainsSyncCall(s.Cond, w.cfg.isRelease) if s.Post != nil { sawAcquire = sawAcquire || nodeContainsCall(s.Post, w.cfg.isAcquire) - releaseSeen = releaseSeen || nodeContainsSyncCall(s.Post, w.cfg.isRelease) } if s.Body != nil { w.walkBlock(s.Body.List, sawAcquire, releaseSeen)