From e30349d410389161a83cfeb785e5ed1d3b23b421 Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Mon, 27 Apr 2020 18:13:33 -0700 Subject: [PATCH] Add static analysis to enforce usage of InitWithReset (#5654) * Refactor attestation packing slightly to reduce the skip slot / HTR of process slots * Merge branch 'master' into refactor-attestation-packing * gofmt * Merge branch 'refactor-attestation-packing' of github.com:prysmaticlabs/prysm into refactor-attestation-packing * Merge branch 'master' of github.com:prysmaticlabs/prysm into refactor-attestation-packing * Add static analysis to enforce usage of InitWithReset * Add comment / lint * fix a few usages * more fixes for featureconfig.Init * Fix analyzer * Merge branch 'sa-fc-init' of github.com:prysmaticlabs/prysm into sa-fc-init * Merge refs/heads/master into sa-fc-init * Merge refs/heads/master into sa-fc-init --- BUILD.bazel | 1 + beacon-chain/db/BUILD.bazel | 6 +- beacon-chain/db/kafka/BUILD.bazel | 1 + nogo_config.json | 5 ++ shared/featureconfig/config.go | 3 +- tools/analyzers/featureconfig/BUILD.bazel | 25 ++++++ tools/analyzers/featureconfig/analyzer.go | 91 ++++++++++++++++++++++ tools/extractor/main.go | 8 +- validator/client/runner_test.go | 3 +- validator/client/validator_attest_test.go | 9 ++- validator/client/validator_propose_test.go | 22 +++--- 11 files changed, 151 insertions(+), 23 deletions(-) create mode 100644 tools/analyzers/featureconfig/BUILD.bazel create mode 100644 tools/analyzers/featureconfig/analyzer.go diff --git a/BUILD.bazel b/BUILD.bazel index b6a2621591ef..35e033f59f69 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -106,6 +106,7 @@ nogo( "//tools/analyzers/maligned:go_tool_library", "//tools/analyzers/roughtime:go_tool_library", "//tools/analyzers/errcheck:go_tool_library", + "//tools/analyzers/featureconfig:go_tool_library", ], ) diff --git a/beacon-chain/db/BUILD.bazel b/beacon-chain/db/BUILD.bazel index dbebc3618e50..cdffcb76b64e 100644 --- a/beacon-chain/db/BUILD.bazel +++ b/beacon-chain/db/BUILD.bazel @@ -13,12 +13,12 @@ go_library( "alias.go", "http_backup_handler.go", ] + select({ - "//conditions:default": [ - "db_kafka_wrapped.go", - ], ":kafka_disabled": [ "db.go", ], + "//conditions:default": [ + "db_kafka_wrapped.go", + ], }), importpath = "github.com/prysmaticlabs/prysm/beacon-chain/db", visibility = [ diff --git a/beacon-chain/db/kafka/BUILD.bazel b/beacon-chain/db/kafka/BUILD.bazel index 526bc2192623..00fd619f76d8 100644 --- a/beacon-chain/db/kafka/BUILD.bazel +++ b/beacon-chain/db/kafka/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "passthrough.go", ], importpath = "github.com/prysmaticlabs/prysm/beacon-chain/db/kafka", + tags = ["manual"], visibility = ["//beacon-chain/db:__pkg__"], deps = [ "//beacon-chain/db/filters:go_default_library", diff --git a/nogo_config.json b/nogo_config.json index cd9a828e3bb2..0be88678814c 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -84,5 +84,10 @@ ".*/.*mock\\.go": "Mocks are OK", ".*/testmain\\.go": "Test runner generated code" } + }, + "featureconfig": { + "only_files": { + ".*_test\\.go": "Only tests" + } } } diff --git a/shared/featureconfig/config.go b/shared/featureconfig/config.go index b111f18e03a1..549bdfaf8374 100644 --- a/shared/featureconfig/config.go +++ b/shared/featureconfig/config.go @@ -12,7 +12,8 @@ The process for implementing new features using this package is as follows: cfg := &featureconfig.Flags{ VerifyAttestationSigs: true, } - featureconfig.Init(cfg) + resetCfg := featureconfig.InitWithReset(cfg) + defer resetCfg() 6. Add the string for the flags that should be running within E2E to E2EValidatorFlags and E2EBeaconChainFlags. */ diff --git a/tools/analyzers/featureconfig/BUILD.bazel b/tools/analyzers/featureconfig/BUILD.bazel new file mode 100644 index 000000000000..7d265bc9a853 --- /dev/null +++ b/tools/analyzers/featureconfig/BUILD.bazel @@ -0,0 +1,25 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/featureconfig", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_default_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library", + "@org_golang_x_tools//go/ast/inspector:go_default_library", + ], +) + +go_tool_library( + name = "go_tool_library", + srcs = ["analyzer.go"], + importpath = "featureconfig", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_tool_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library", + "@org_golang_x_tools//go/ast/inspector:go_tool_library", + ], +) diff --git a/tools/analyzers/featureconfig/analyzer.go b/tools/analyzers/featureconfig/analyzer.go new file mode 100644 index 000000000000..48db5c7629c3 --- /dev/null +++ b/tools/analyzers/featureconfig/analyzer.go @@ -0,0 +1,91 @@ +package featureconfig + +import ( + "errors" + "go/ast" + "go/token" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Doc explaining the tool. +const Doc = "Enforce usage of featureconfig.InitWithReset to prevent leaking globals in tests." + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "featureconfig", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, errors.New("analyzer is not type *inspector.Inspector") + } + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + (*ast.ExprStmt)(nil), + (*ast.GoStmt)(nil), + (*ast.DeferStmt)(nil), + (*ast.AssignStmt)(nil), + } + + inspect.Preorder(nodeFilter, func(node ast.Node) { + if ce, ok := node.(*ast.CallExpr); ok && isPkgDot(ce.Fun, "featureconfig", "Init") { + reportForbiddenUsage(pass, ce.Pos()) + return + } + switch stmt := node.(type) { + case *ast.ExprStmt: + if call, ok := stmt.X.(*ast.CallExpr); ok&& isPkgDot(call.Fun, "featureconfig", "InitWithReset"){ + reportUnhandledReset(pass, call.Lparen) + } + case *ast.GoStmt: + if isPkgDot(stmt.Call, "featureconfig", "InitWithReset") { + reportUnhandledReset(pass, stmt.Call.Lparen) + } + case *ast.DeferStmt: + if isPkgDot(stmt.Call, "featureconfig", "InitWithReset") { + reportUnhandledReset(pass, stmt.Call.Lparen) + } + case *ast.AssignStmt: + if ce, ok := stmt.Rhs[0].(*ast.CallExpr); ok && isPkgDot(ce.Fun, "featureconfig", "InitWithReset") { + for i := 0; i < len(stmt.Lhs); i++ { + if id, ok := stmt.Lhs[i].(*ast.Ident); ok { + if id.Name == "_" { + reportUnhandledReset(pass, id.NamePos) + } + } + } + } + default: + } + }) + + return nil, nil +} + +func reportForbiddenUsage(pass *analysis.Pass, pos token.Pos) { + pass.Reportf(pos, "Use of featureconfig.Init is forbidden in test code. Please use " + + "featureconfig.InitWithReset and call reset in the same test function.") +} + +func reportUnhandledReset(pass *analysis.Pass, pos token.Pos) { + pass.Reportf(pos, "Unhandled reset featureconfig not found in test " + + "method. Be sure to call the returned reset function from featureconfig.InitWithReset " + + "within this test method.") +} + +func isPkgDot(expr ast.Expr, pkg, name string) bool { + sel, ok := expr.(*ast.SelectorExpr) + return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) +} + +func isIdent(expr ast.Expr, ident string) bool { + id, ok := expr.(*ast.Ident) + return ok && id.Name == ident +} diff --git a/tools/extractor/main.go b/tools/extractor/main.go index 6d9f05a9f8ce..b2eb8cafcfd9 100644 --- a/tools/extractor/main.go +++ b/tools/extractor/main.go @@ -19,13 +19,9 @@ var ( state = flag.Uint("state", 0, "Extract state at this slot.") ) -func init() { - fc := featureconfig.Get() - fc.WriteSSZStateTransitions = true - featureconfig.Init(fc) -} - func main() { + resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{WriteSSZStateTransitions: true}) + defer resetCfg() flag.Parse() fmt.Println("Starting process...") d, err := db.NewDB(*datadir, cache.NewStateSummaryCache()) diff --git a/validator/client/runner_test.go b/validator/client/runner_test.go index 2f9932a56a5a..9cb8816aac66 100644 --- a/validator/client/runner_test.go +++ b/validator/client/runner_test.go @@ -37,7 +37,8 @@ func TestCancelledContext_WaitsForSynced(t *testing.T) { cfg := &featureconfig.Flags{ WaitForSynced: true, } - featureconfig.Init(cfg) + reset := featureconfig.InitWithReset(cfg) + defer reset() v := &fakeValidator{} run(cancelledContext(), v) if !v.WaitForSyncedCalled { diff --git a/validator/client/validator_attest_test.go b/validator/client/validator_attest_test.go index c6cbfbe8464b..8c160f84a7f4 100644 --- a/validator/client/validator_attest_test.go +++ b/validator/client/validator_attest_test.go @@ -132,7 +132,8 @@ func TestAttestToBlockHead_BlocksDoubleAtt(t *testing.T) { config := &featureconfig.Flags{ ProtectAttester: true, } - featureconfig.Init(config) + reset := featureconfig.InitWithReset(config) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() @@ -173,7 +174,8 @@ func TestAttestToBlockHead_BlocksSurroundAtt(t *testing.T) { config := &featureconfig.Flags{ ProtectAttester: true, } - featureconfig.Init(config) + reset := featureconfig.InitWithReset(config) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() @@ -224,7 +226,8 @@ func TestAttestToBlockHead_BlocksSurroundedAtt(t *testing.T) { config := &featureconfig.Flags{ ProtectAttester: true, } - featureconfig.Init(config) + reset := featureconfig.InitWithReset(config) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() diff --git a/validator/client/validator_propose_test.go b/validator/client/validator_propose_test.go index 7e04200d4495..063809fedb8f 100644 --- a/validator/client/validator_propose_test.go +++ b/validator/client/validator_propose_test.go @@ -33,11 +33,11 @@ func setup(t *testing.T) (*validator, *mocks, func()) { } validator := &validator{ - db: valDB, - validatorClient: m.validatorClient, - keyManager: testKeyManager, - graffiti: []byte{}, - attLogs: make(map[[32]byte]*attSubmitted), + db: valDB, + validatorClient: m.validatorClient, + keyManager: testKeyManager, + graffiti: []byte{}, + attLogs: make(map[[32]byte]*attSubmitted), aggregatedSlotCommitteeIDCache: aggregatedSlotCommitteeIDCache, } @@ -119,7 +119,8 @@ func TestProposeBlock_BlocksDoubleProposal(t *testing.T) { cfg := &featureconfig.Flags{ ProtectProposer: true, } - featureconfig.Init(cfg) + reset := featureconfig.InitWithReset(cfg) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() @@ -156,7 +157,8 @@ func TestProposeBlock_BlocksDoubleProposal_After54KEpochs(t *testing.T) { cfg := &featureconfig.Flags{ ProtectProposer: true, } - featureconfig.Init(cfg) + reset := featureconfig.InitWithReset(cfg) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() @@ -194,7 +196,8 @@ func TestProposeBlock_AllowsPastProposals(t *testing.T) { cfg := &featureconfig.Flags{ ProtectProposer: true, } - featureconfig.Init(cfg) + reset := featureconfig.InitWithReset(cfg) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish() @@ -233,7 +236,8 @@ func TestProposeBlock_AllowsSameEpoch(t *testing.T) { cfg := &featureconfig.Flags{ ProtectProposer: true, } - featureconfig.Init(cfg) + reset := featureconfig.InitWithReset(cfg) + defer reset() hook := logTest.NewGlobal() validator, m, finish := setup(t) defer finish()