lint: add check to ensure mutex Unlock() calls are deferred

[abarganier]

Is your feature request related to a problem? Please describe.
We recently dealt with some deadlocks due to code written in this style:

func myFunc() error {
  // do some work...
  p.Lock()
  if err := somethingThatCanPanic(); err != nil {
    p.Unlock()
    return err
  }
  p.Unlock()
  // do some more work...
  return nil
}

A safer way to write this code would be something like:

func myFunc() error {
  // do some work...
  doRiskyWork := func() error {
    p.Lock()
    defer p.Unlock()
    return somethingThatCanPanic()
  }
  if err := doRiskyWork(); err != nil {
    return err
  }
  // do some more work...
  return nil
}

It would be nice to have a lint rule that enforces .Unlock() calls on mutexes use the defer keyword, to help avoid this pitfall in the future (and identify lingering cases of it).

We recognize that in some cases, using defer is not ideal, so we should ensure that programmers have the option to use lint:ignore for this rule. However, as the organization scales, we should set the default expectation to help avoid this pattern from being used in the future.

Jira issue: CRDB-29001

[tbg]

This would've caught #106078 (comment)

[tbg]

Returning to this issue due to #106568. Just reiterating that it could be high-value to introduce this lint since it might point us at the location of the replica mutex leak referred to in that issue.

[tbg]

re: the UX that results from the linter, it is often cumbersome to introduce anonymous functions to guide the defer calls. Another approach could be the following:

// package syncutil
type IdempotentUnlocker struct{mu interface{ Unlock() }}

func (iu *IdempotentUnlocker) IdempotentUnlock() {
  if iu.mu == nil { return }
  iu.mu.Unlock()
  iu.mu = nil
}

func Lock(mu *sync.Mutex) IdempotentUnlocker() {
  mu.Lock()
  return IdempotentUnlocker{mu}
}

// something similar for RLock

func (r *Replica) foo() {
  mu := syncutil.Lock(r.mu)
  defer mu.IdempotentUnlock()

  if riskyWorkFailed() {
    return nil
  }

  mu.IdempotentUnlock()
  // do some more work outside of crit section
}

I think that one will not allocate (have to check though) so it could be suitable for hot code paths as well.

[abarganier]

@tbg the team is picking this up as part of Code Yellow. Thanks for the pointers above 🙏 we will post updates here!

[srosenberg]

I wrote a crude linter using revive framework.

The linter performs super naive flow propagation to track if a lock is held during function calls. It doesn't perform a proper data flow propagation (needed for precision), nor is it aware of control-flow reachability. Instead, it uses an exclusion list (see prettyPrintLockExprs) of hand-vetted functions which don't panic. (Reachability analysis performed by govulncheck can be extended to do this automatically; I'll come back to it when I have more time.) The linter counts the number of function calls and ifStmts while a lock is held and outputs them, e.g.,

lockExpr: w.latency.Current.RecordValue
lockExpr: elapsed.Nanoseconds
lockExpr: z.zipfGenMu.r.Float64
lockExpr: math.Pow
found "Unlock" outside of defer at /Users/srosenberg/workspace/go/src/github.com/cockroachdb/cockroach/pkg/workload/ycsb/zipfgenerator.go:146:2
2 lockExprs, 3 ifStmts

The linter also has another mode which performs a crude analysis to determine if a lock is potentially leaked. It does this by maintaining a stack (using lexical order) and popping each time Unlock is seen (outside of defer). If the stack isn't empty on return, we have a potentially leaked lock, e.g.,

lock "ex.mu" may have leaked at /Users/srosenberg/workspace/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2927:4

Despite being super naive, it was able to find a recently fixed leak [1], as well as, some new ones. (I'll open separate issues for those [2].)

To run,

./revive -config ./test.toml $GOPATH/src/github.com/cockroachdb/cockroach/pkg/... > ~/unsafe_lock.txt

As you can imagine, this type of linter will produce a lot of false positives,

grep "found \"Unlock" ~/unsafe_lock.txt|wc -l
     273

A manual audit is probably still warranted. However, with a bit more work (e.g., reachability analysis to exclude all functions that don't panic), I am confident we could reduce the false positives to a more manageable size.

Attaching the output for both modes against master,

leaked_mutex.txt
unsafe_lock.txt

[1] e3c0767
[2] #107433

[Santamaura]

I wrote a crude linter using revive framework.

The linter performs super naive flow propagation to track if a lock is held during function calls. It doesn't perform a proper data flow propagation (needed for precision), nor is it aware of control-flow reachability. Instead, it uses an exclusion list (see prettyPrintLockExprs) of hand-vetted functions which don't panic. (Reachability analysis performed by govulncheck can be extended to do this automatically; I'll come back to it when I have more time.) The linter counts the number of function calls and ifStmts while a lock is held and outputs them, e.g.,

lockExpr: w.latency.Current.RecordValue
lockExpr: elapsed.Nanoseconds
lockExpr: z.zipfGenMu.r.Float64
lockExpr: math.Pow
found "Unlock" outside of defer at /Users/srosenberg/workspace/go/src/github.com/cockroachdb/cockroach/pkg/workload/ycsb/zipfgenerator.go:146:2
2 lockExprs, 3 ifStmts

The linter also has another mode which performs a crude analysis to determine if a lock is potentially leaked. It does this by maintaining a stack (using lexical order) and popping each time Unlock is seen (outside of defer). If the stack isn't empty on return, we have a potentially leaked lock, e.g.,

lock "ex.mu" may have leaked at /Users/srosenberg/workspace/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2927:4

Despite being super naive, it was able to find a recently fixed leak [1], as well as, some new ones. (I'll open separate issues for those [2].)

To run,

./revive -config ./test.toml $GOPATH/src/github.com/cockroachdb/cockroach/pkg/... > ~/unsafe_lock.txt

As you can imagine, this type of linter will produce a lot of false positives,

grep "found \"Unlock" ~/unsafe_lock.txt|wc -l
     273

A manual audit is probably still warranted. However, with a bit more work (e.g., reachability analysis to exclude all functions that don't panic), I am confident we could reduce the false positives to a more manageable size.

Attaching the output for both modes against master,

leaked_mutex.txt unsafe_lock.txt

[1] e3c0767 [2] #107433

@srosenberg this is great! So would the path forward on this issue be adding the reachability analysis, doing the manual audit, moving this logic into an analyzer so it can be used in CI?