From f8cf42eccd7ab0890e5b3a039f726e2b03e1f968 Mon Sep 17 00:00:00 2001 From: Alex Santamaura Date: Tue, 25 Jul 2023 16:32:02 -0400 Subject: [PATCH] lint: add new deferunlocktest pass This commit adds a new pass which checks for `...Unlock()` expressions without `defer` which could result in a lock being held indefinitely. Resolves #105366 Release note: None --- BUILD.bazel | 1 + build/bazelutil/nogo_config.json | 6 ++ pkg/BUILD.bazel | 3 + pkg/cmd/roachvet/BUILD.bazel | 1 + pkg/cmd/roachvet/main.go | 2 + .../lint/passes/deferunlockcheck/BUILD.bazel | 30 ++++++++++ .../deferunlockcheck/deferunlockcheck.go | 59 +++++++++++++++++++ .../deferunlockcheck/deferunlockcheck_test.go | 34 +++++++++++ .../deferunlockcheck/testdata/src/a/a.go | 58 ++++++++++++++++++ .../cockroach/pkg/util/syncutil/mutex_sync.go | 21 +++++++ 10 files changed, 215 insertions(+) create mode 100644 pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel create mode 100644 pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go create mode 100644 pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go create mode 100644 pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go create mode 100644 pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go diff --git a/BUILD.bazel b/BUILD.bazel index 2cec258e1e18..ef2254d8df86 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -186,6 +186,7 @@ nogo( "@org_golang_x_tools//go/analysis/passes/unreachable:go_default_library", "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_default_library", "@org_golang_x_tools//go/analysis/passes/unusedresult:go_default_library", + "//pkg/testutils/lint/passes/deferunlockcheck", "//pkg/testutils/lint/passes/descriptormarshal", "//pkg/testutils/lint/passes/errcheck", "//pkg/testutils/lint/passes/errcmp", diff --git a/build/bazelutil/nogo_config.json b/build/bazelutil/nogo_config.json index cddfbc90d5e8..2fdec81004c4 100644 --- a/build/bazelutil/nogo_config.json +++ b/build/bazelutil/nogo_config.json @@ -28,6 +28,12 @@ "_test\\.go$": "tests" } }, + "deferunlockcheck": { + "only_files": { + "cockroach/pkg/.*$": "first-party code", + "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code" + } + }, "errcheck": { "exclude_files": { "pkg/.*\\.eg\\.go$": "generated code", diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 8bde81d9deb5..5177ac718898 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -593,6 +593,7 @@ ALL_TESTS = [ "//pkg/testutils/fingerprintutils:fingerprintutils_test", "//pkg/testutils/floatcmp:floatcmp_test", "//pkg/testutils/keysutils:keysutils_test", + "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test", "//pkg/testutils/lint/passes/errcmp:errcmp_test", "//pkg/testutils/lint/passes/errwrap:errwrap_test", "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test", @@ -2167,6 +2168,8 @@ GO_TARGETS = [ "//pkg/testutils/keysutils:keysutils", "//pkg/testutils/keysutils:keysutils_test", "//pkg/testutils/kvclientutils:kvclientutils", + "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck", + "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test", "//pkg/testutils/lint/passes/descriptormarshal:descriptormarshal", "//pkg/testutils/lint/passes/errcheck:errcheck", "//pkg/testutils/lint/passes/errcmp:errcmp", diff --git a/pkg/cmd/roachvet/BUILD.bazel b/pkg/cmd/roachvet/BUILD.bazel index 76d7b8fb0bf4..d80616523d05 100644 --- a/pkg/cmd/roachvet/BUILD.bazel +++ b/pkg/cmd/roachvet/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachvet", visibility = ["//visibility:private"], deps = [ + "//pkg/testutils/lint/passes/deferunlockcheck", "//pkg/testutils/lint/passes/errcmp", "//pkg/testutils/lint/passes/errwrap", "//pkg/testutils/lint/passes/fmtsafe", diff --git a/pkg/cmd/roachvet/main.go b/pkg/cmd/roachvet/main.go index f51c4a67b4c9..e5b654fd9c6b 100644 --- a/pkg/cmd/roachvet/main.go +++ b/pkg/cmd/roachvet/main.go @@ -14,6 +14,7 @@ package main import ( + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errcmp" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe" @@ -70,6 +71,7 @@ func main() { nilness.Analyzer, errwrap.Analyzer, loopvarcapture.Analyzer, + deferunlockcheck.Analyzer, ) // Standard go vet analyzers: diff --git a/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel b/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel new file mode 100644 index 000000000000..96831507e7ff --- /dev/null +++ b/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel @@ -0,0 +1,30 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "deferunlockcheck", + srcs = ["deferunlockcheck.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck", + visibility = ["//visibility:public"], + deps = [ + "//pkg/testutils/lint/passes/passesutil", + "@org_golang_x_tools//go/analysis", + "@org_golang_x_tools//go/analysis/passes/inspect", + "@org_golang_x_tools//go/ast/inspector", + ], +) + +go_test( + name = "deferunlockcheck_test", + srcs = ["deferunlockcheck_test.go"], + args = ["-test.timeout=295s"], + data = glob(["testdata/**"]) + [ + "@go_sdk//:files", + ], + deps = [ + ":deferunlockcheck", + "//pkg/build/bazel", + "//pkg/testutils/datapathutils", + "//pkg/testutils/skip", + "@org_golang_x_tools//go/analysis/analysistest", + ], +) diff --git a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go new file mode 100644 index 000000000000..4b04889d32f7 --- /dev/null +++ b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go @@ -0,0 +1,59 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package deferunlockcheck + +import ( + "go/ast" + + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/passesutil" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Doc documents this pass. +const Doc = "checks that usages of mutex Unlock() are deferred." + +const noLintName = "deferunlock" + +// Analyzer is an analysis pass that checks for mutex unlocks which +// aren't deferred. +var Analyzer = &analysis.Analyzer{ + Name: "deferunlockcheck", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + astInspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + // Note: defer ...Unlock() expressions are captured as ast.DeferStmt nodes so we don't have to worry about those results returning + // for ast.ExprStmt nodes. + filter := []ast.Node{ + (*ast.ExprStmt)(nil), + } + astInspector.Preorder(filter, func(n ast.Node) { + stmt := n.(*ast.ExprStmt) + expr, ok := stmt.X.(*ast.CallExpr) + if !ok { + return + } + sel, ok := expr.Fun.(*ast.SelectorExpr) + if !ok { + return + } + if sel.Sel != nil && (sel.Sel.Name == "Unlock" || sel.Sel.Name == "RUnlock") && !passesutil.HasNolintComment(pass, n, noLintName) { + pass.Reportf(sel.Pos(), "Mutex %s not deferred", sel.Sel.Name) + } + }) + + return nil, nil +} diff --git a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go new file mode 100644 index 000000000000..ccdabf5b9e88 --- /dev/null +++ b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go @@ -0,0 +1,34 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package deferunlockcheck_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/build/bazel" + "github.com/cockroachdb/cockroach/pkg/testutils/datapathutils" + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "golang.org/x/tools/go/analysis/analysistest" +) + +func init() { + if bazel.BuiltWithBazel() { + bazel.SetGoEnv() + } +} + +func Test(t *testing.T) { + skip.UnderStress(t) + testdata := datapathutils.TestDataPath(t) + analysistest.TestData = func() string { return testdata } + analysistest.Run(t, testdata, deferunlockcheck.Analyzer, "a") +} diff --git a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go new file mode 100644 index 000000000000..0297007d25d7 --- /dev/null +++ b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go @@ -0,0 +1,58 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package a + +import ( + "github.com/cockroachdb/cockroach/pkg/util/syncutil" +) + +type TestUnlockLint struct { + mu struct { + syncutil.Mutex + } +} + +func init() { + var mut syncutil.Mutex + var rwmut syncutil.RWMutex + testUnlock := &TestUnlockLint{} + + // Test the main use case. + // Should only capture Unlock() + testUnlock.mu.Lock() + testUnlock.mu.Unlock() // want `Mutex Unlock not deferred` + // This should pass. + defer testUnlock.mu.Unlock() + + // Test within a function. + okFn := func() { + testUnlock.mu.Lock() + defer testUnlock.mu.Unlock() + } + failFn := func() { + testUnlock.mu.Lock() + testUnlock.mu.Unlock() // want `Mutex Unlock not deferred` + } + okFn() + failFn() + + // Test mut variation. + defer mut.Unlock() + mut.Unlock() // want `Mutex Unlock not deferred` + + // Test RUnlock + defer rwmut.RUnlock() + rwmut.RUnlock() // want `Mutex RUnlock not deferred` + + // Test the no lint rule. + // nolint:deferunlock + testUnlock.mu.Unlock() +} diff --git a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go new file mode 100644 index 000000000000..a7066e32dad3 --- /dev/null +++ b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go @@ -0,0 +1,21 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package syncutil + +import "sync" + +type Mutex struct { + sync.Mutex +} + +type RWMutex struct { + sync.RWMutex +}