diff --git a/cspell-custom-words.txt b/cspell-custom-words.txt index 37a42e9f72..301287e82a 100644 --- a/cspell-custom-words.txt +++ b/cspell-custom-words.txt @@ -165,6 +165,7 @@ Reobservation Reobservations reobserved repoint +revivelib rustup satoshi secp @@ -180,6 +181,8 @@ solana Solana Solana's spydk +Semgrep +semgrep Starport statesync struct @@ -193,6 +196,7 @@ superminority tendermint Tendermint terrad +testdata tokenbridge tokenfactory trustlessly diff --git a/custom-rules/.gitignore b/custom-rules/.gitignore new file mode 100644 index 0000000000..6ea970f2aa --- /dev/null +++ b/custom-rules/.gitignore @@ -0,0 +1,2 @@ +# don't commit the binary +wormhole-revive diff --git a/custom-rules/Makefile b/custom-rules/Makefile new file mode 100755 index 0000000000..55880dbf14 --- /dev/null +++ b/custom-rules/Makefile @@ -0,0 +1,8 @@ +.PHONY: install +## Install guardiand binary +build: + go build -o wormhole-revive . + +run: + ./wormhole-revive -config revive.toml ../node/... + ./wormhole-revive -config revive.toml ../sdk/... diff --git a/custom-rules/README.md b/custom-rules/README.md new file mode 100644 index 0000000000..15f9ab017c --- /dev/null +++ b/custom-rules/README.md @@ -0,0 +1,138 @@ +# Wormhole Custom Revive Rules + +This directory contains custom linting rules for the Wormhole project using the revive framework. + +## Custom Rules + +### already-locked-checker + +Detects functions with "alreadyLocked" in their name (case insensitive) and ensures they are called within proper mutex lock/unlock blocks. + +**What it checks:** +- Functions containing "alreadyLocked" in their name +- Verifies the calling function has mutex locking patterns: + - `mutex.Lock()` + `mutex.Unlock()` + - `mutex.Lock()` + `defer mutex.Unlock()` + +**Example violations:** +```go +// BAD: No mutex locking +func badFunction() { + obj.processDataAlreadyLocked("key") // Will be flagged +} + +// GOOD: Proper mutex locking +func goodFunction() { + obj.mu.Lock() + defer obj.mu.Unlock() + obj.processDataAlreadyLocked("key") // Will NOT be flagged +} +``` + +## Usage + +### Build the Custom Revive Binary + +```bash +cd custom-rules +go build -o wormhole-revive . +``` + +### Run with Custom Rules + +```bash +# Test on a single file +./wormhole-revive -config revive.toml path/to/file.go + +# Test on a directory +./wormhole-revive -config revive.toml ../node/pkg/accountant/ + +# Use different formatter +./wormhole-revive -config revive.toml -formatter stylish ../node/pkg/ +``` + +### Integration with Existing Workflow + +#### Add to Makefile +```makefile +lint: lint-standard lint-custom + +lint-standard: + golangci-lint run + +lint-custom: + cd custom-rules && ./wormhole-revive -config revive.toml ../node/pkg/ +``` + +## Configuration + +The `revive.toml` file configures which rules are enabled and which files to exclude: + +```toml +[rule.already-locked-checker] + severity = "error" # Make violations fail the build + # Exclude files using patterns + Exclude = ["**/*_test.go", "**/testdata/**", "**/mock*.go"] + +[rule.var-naming] + arguments = [["ID"], ["VM"]] + severity = "warning" + # Example: exclude generated files + Exclude = ["**/*.pb.go", "**/*_generated.go"] +``` + +### File Exclusion Patterns + +You can exclude files from rules using various patterns in the `Exclude` array: + +1. **Glob patterns**: `"**/*_test.go"` - exclude all test files +2. **Directory patterns**: `"**/testdata/**"` - exclude testdata directories +3. **Wildcard patterns**: `"**/mock*.go"` - exclude mock files +4. **Regex patterns**: `"~\.(pb|auto|generated)\.go$"` - exclude generated files +5. **Specific files**: `"path/to/specific/file.go"` - exclude individual files +6. **Well-known patterns**: `"TEST"` - same as `**/*_test.go` + +### Common Exclusion Examples + +```toml +# Exclude test files, generated code, and vendor directories +[rule.already-locked-checker] + Exclude = [ + "**/*_test.go", # Test files + "**/*.pb.go", # Protocol buffer generated files + "**/*_generated.go", # Generated Go files + "**/vendor/**", # Vendor dependencies + "**/testdata/**", # Test data directories + "**/mock*.go", # Mock files + "~\.(pb|auto|generated)\.go$" # Regex for generated files + ] +``` + +## Adding More Custom Rules + +To add additional custom rules: + +1. Create a new rule struct implementing the `lint.Rule` interface +2. Add it to `main.go` using `revivelib.NewExtraRule()` +3. Configure it in `revive.toml` +4. Rebuild the binary + +Example: +```go +// In main.go +cli.RunRevive( + revivelib.NewExtraRule(&AlreadyLockedRule{}, lint.RuleConfig{}), + revivelib.NewExtraRule(&MyNewRule{}, lint.RuleConfig{}), +) +``` + +## Why revive? + +Revive was selected mainly because we get the following benefits: +- it's well-maintained +- it supports AST-based rule creation +- rules can be done in pure Go + +This is in contrast to a few other linters: +- CodeQL requires a heavy indexing step and a steep learning curve +- Semgrep isn't great for complex rules and requires learning its YAML syntax diff --git a/custom-rules/already_locked_rule.go b/custom-rules/already_locked_rule.go new file mode 100644 index 0000000000..83912165bf --- /dev/null +++ b/custom-rules/already_locked_rule.go @@ -0,0 +1,146 @@ +package main + +import ( + "fmt" + "go/ast" + "strings" + + "github.com/mgechev/revive/lint" +) + +// AlreadyLockedRule checks for proper mutex usage with alreadyLocked functions +type AlreadyLockedRule struct{} + +// Name returns the rule name +func (r *AlreadyLockedRule) Name() string { + return "already-locked-checker" +} + +// Apply applies the rule to the given file +func (r *AlreadyLockedRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + // Find all function declarations and check them + for _, decl := range file.AST.Decls { + if funcDecl, ok := decl.(*ast.FuncDecl); ok { + failures = append(failures, r.checkFunction(funcDecl, file)...) + } + } + + return failures +} + +// checkFunction checks a single function for alreadyLocked calls +func (r *AlreadyLockedRule) checkFunction(funcDecl *ast.FuncDecl, file *lint.File) []lint.Failure { + var failures []lint.Failure + + if funcDecl.Body == nil { + return failures + } + + // Check if this function itself is an "alreadyLocked" function + isAlreadyLockedFunction := r.isAlreadyLockedFunction(funcDecl) + + // Check if this function has mutex locking + hasMutexLocking := r.hasMutexLocking(funcDecl) + + // If this function is itself an "alreadyLocked" function, then calls to other + // "alreadyLocked" functions are valid (the lock is held by the original caller) + if isAlreadyLockedFunction { + return failures // No need to check - this function assumes lock is already held + } + + // Find all alreadyLocked function calls + ast.Inspect(funcDecl, func(n ast.Node) bool { + if call, ok := n.(*ast.CallExpr); ok { + if failure := r.checkCallExpr(call, hasMutexLocking, file); failure != nil { + failures = append(failures, *failure) + } + } + return true + }) + + return failures +} + +// checkCallExpr checks if a function call to an "alreadyLocked" function has proper mutex usage +func (r *AlreadyLockedRule) checkCallExpr(call *ast.CallExpr, hasMutexLocking bool, file *lint.File) *lint.Failure { + // Check if this is a function call with "alreadyLocked" in the name + var funcName string + switch fun := call.Fun.(type) { + case *ast.Ident: + funcName = fun.Name + case *ast.SelectorExpr: + funcName = fun.Sel.Name + default: + return nil + } + + // Check if function name contains "alreadyLocked" (case insensitive) + if !strings.Contains(strings.ToLower(funcName), "alreadylocked") { + return nil + } + + // If no mutex locking found, report an issue + if !hasMutexLocking { + return &lint.Failure{ + Confidence: 1, + Node: call, + Category: "logic", + Failure: fmt.Sprintf("function call to '%s' should be within a mutex lock/unlock block", funcName), + } + } + + return nil +} + +// hasMutexLocking checks if a function has mutex locking patterns +func (r *AlreadyLockedRule) hasMutexLocking(funcDecl *ast.FuncDecl) bool { + hasLock := false + hasUnlock := false + hasDefer := false + + ast.Inspect(funcDecl, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CallExpr: + if r.isLockCall(node) { + hasLock = true + } + if r.isUnlockCall(node) { + hasUnlock = true + } + case *ast.DeferStmt: + if r.isUnlockCall(node.Call) { + hasDefer = true + } + } + return true + }) + + // Accept either explicit lock/unlock or defer unlock pattern + return hasLock && (hasUnlock || hasDefer) +} + +// isLockCall checks if a call expression is a mutex Lock() call +func (r *AlreadyLockedRule) isLockCall(call *ast.CallExpr) bool { + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + return sel.Sel.Name == "Lock" + } + return false +} + +// isUnlockCall checks if a call expression is a mutex Unlock() call +func (r *AlreadyLockedRule) isUnlockCall(call *ast.CallExpr) bool { + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + return sel.Sel.Name == "Unlock" + } + return false +} + +// isAlreadyLockedFunction checks if a function has "alreadyLocked" in its name +func (r *AlreadyLockedRule) isAlreadyLockedFunction(funcDecl *ast.FuncDecl) bool { + if funcDecl.Name == nil { + return false + } + return strings.Contains(strings.ToLower(funcDecl.Name.Name), "alreadylocked") +} diff --git a/custom-rules/go.mod b/custom-rules/go.mod new file mode 100644 index 0000000000..9bfa933e65 --- /dev/null +++ b/custom-rules/go.mod @@ -0,0 +1,24 @@ +module github.com/certusone/wormhole/custom-rules + +go 1.23.0 + +toolchain go1.24.5 + +require github.com/mgechev/revive v1.11.0 + +require ( + codeberg.org/chavacava/garif v0.2.0 // indirect + github.com/BurntSushi/toml v1.5.0 // indirect + github.com/fatih/color v1.18.0 // indirect + github.com/fatih/structtag v1.2.0 // indirect + github.com/hashicorp/go-version v1.7.0 // indirect + github.com/mattn/go-colorable v0.1.14 // indirect + github.com/mattn/go-isatty v0.0.20 // indirect + github.com/mgechev/dots v1.0.0 // indirect + github.com/spf13/afero v1.14.0 // indirect + golang.org/x/mod v0.26.0 // indirect + golang.org/x/sync v0.16.0 // indirect + golang.org/x/sys v0.34.0 // indirect + golang.org/x/text v0.27.0 // indirect + golang.org/x/tools v0.35.0 // indirect +) diff --git a/custom-rules/go.sum b/custom-rules/go.sum new file mode 100644 index 0000000000..2358aee9c9 --- /dev/null +++ b/custom-rules/go.sum @@ -0,0 +1,39 @@ +codeberg.org/chavacava/garif v0.2.0 h1:F0tVjhYbuOCnvNcU3YSpO6b3Waw6Bimy4K0mM8y6MfY= +codeberg.org/chavacava/garif v0.2.0/go.mod h1:P2BPbVbT4QcvLZrORc2T29szK3xEOlnl0GiPTJmEqBQ= +github.com/BurntSushi/toml v1.5.0 h1:W5quZX/G/csjUnuI8SUYlsHs9M38FC7znL0lIO+DvMg= +github.com/BurntSushi/toml v1.5.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/fatih/color v1.18.0 h1:S8gINlzdQ840/4pfAwic/ZE0djQEH3wM94VfqLTZcOM= +github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU= +github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4= +github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94= +github.com/hashicorp/go-version v1.7.0 h1:5tqGy27NaOTB8yJKUZELlFAS/LTKJkrmONwQKeRZfjY= +github.com/hashicorp/go-version v1.7.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= +github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= +github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= +github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= +github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/mgechev/dots v1.0.0 h1:o+4OJ3OjWzgQHGJXKfJ8rbH4dqDugu5BiEy84nxg0k4= +github.com/mgechev/dots v1.0.0/go.mod h1:rykuMydC9t3wfkM+ccYH3U3ss03vZGg6h3hmOznXLH0= +github.com/mgechev/revive v1.11.0 h1:b/gLLpBE427o+Xmd8G58gSA+KtBwxWinH/A565Awh0w= +github.com/mgechev/revive v1.11.0/go.mod h1:tI0oLF/2uj+InHCBLrrqfTKfjtFTBCFFfG05auyzgdw= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/spf13/afero v1.14.0 h1:9tH6MapGnn/j0eb0yIXiLjERO8RB6xIVZRDCX7PtqWA= +github.com/spf13/afero v1.14.0/go.mod h1:acJQ8t0ohCGuMN3O+Pv0V0hgMxNYDlvdk+VTfyZmbYo= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= +golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ= +golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= +golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.34.0 h1:H5Y5sJ2L2JRdyv7ROF1he/lPdvFsd0mJHFw2ThKHxLA= +golang.org/x/sys v0.34.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/text v0.27.0 h1:4fGWRpyh641NLlecmyl4LOe6yDdfaYNrGb2zdfo4JV4= +golang.org/x/text v0.27.0/go.mod h1:1D28KMCvyooCX9hBiosv5Tz/+YLxj0j7XhWjpSUF7CU= +golang.org/x/tools v0.35.0 h1:mBffYraMEf7aa0sB+NuKnuCy8qI/9Bughn8dC2Gu5r0= +golang.org/x/tools v0.35.0/go.mod h1:NKdj5HkL/73byiZSJjqJgKn3ep7KjFkBOkR/Hps3VPw= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/custom-rules/main.go b/custom-rules/main.go new file mode 100644 index 0000000000..6deea2fd72 --- /dev/null +++ b/custom-rules/main.go @@ -0,0 +1,15 @@ +package main + +import ( + "github.com/mgechev/revive/cli" + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/revivelib" +) + +func main() { + // Create our custom rule + alreadyLockedRule := &AlreadyLockedRule{} + + // Run revive with our custom rule added + cli.RunRevive(revivelib.NewExtraRule(alreadyLockedRule, lint.RuleConfig{})) +} diff --git a/custom-rules/revive.toml b/custom-rules/revive.toml new file mode 100644 index 0000000000..bbf549a0a6 --- /dev/null +++ b/custom-rules/revive.toml @@ -0,0 +1,18 @@ +ignoreGeneratedHeader = false +severity = "warning" +confidence = 0.8 +# Required so that violations return a failing exit code +errorCode = 1 +warningCode = 2 + +# Enable our custom already-locked-checker rule +[rule.already-locked-checker] + severity = "error" + # Exclude test files and other patterns as needed + # Available patterns: + # - "**/*_test.go" - exclude all test files + # - "**/testdata/**" - exclude testdata directories + # - "**/mock*.go" - exclude mock files + # - "~\.(pb|auto|generated)\.go$" - exclude generated files (regex) + # - "specific/path/file.go" - exclude specific files + Exclude = ["**/*_test.go", "**/testdata/**", "**/mock*.go", "**/*.pb.go", "**/*.proto"] diff --git a/custom-rules/test_already_locked.go b/custom-rules/test_already_locked.go new file mode 100644 index 0000000000..6b833a6196 --- /dev/null +++ b/custom-rules/test_already_locked.go @@ -0,0 +1,39 @@ +package main + +import "sync" + +type TestStruct struct { + mu sync.Mutex + data map[string]string +} + +// Good: Function with proper locking +func (t *TestStruct) goodFunction() { + t.mu.Lock() + defer t.mu.Unlock() + + // This should NOT trigger the rule because we have proper locking + t.processDataAlreadyLocked("key") +} + +// Bad: Function without proper locking +func (t *TestStruct) badFunction() { + // This SHOULD trigger the rule because no locking + t.processDataAlreadyLocked("key") +} + +// Good: Function with explicit lock/unlock +func (t *TestStruct) anotherGoodFunction() { + t.mu.Lock() + t.updateCacheAlreadyLocked("value") + t.mu.Unlock() +} + +// Helper functions that should be called with locks +func (t *TestStruct) processDataAlreadyLocked(key string) { + t.data[key] = "processed" +} + +func (t *TestStruct) updateCacheAlreadyLocked(value string) { + t.data["cache"] = value +} diff --git a/scripts/lint.sh b/scripts/lint.sh index a780879d1b..8dcc4e6d76 100755 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -75,6 +75,10 @@ lint(){ cd "${ROOT}/sdk" golangci-lint run --timeout=10m $GOLANGCI_LINT_ARGS ./... + + cd "${ROOT}/custom-rules" + make build + make run } DOCKER="false"