From a2cf2356876d58520e53e0423d841b2eced53f61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Kapka?= Date: Thu, 17 Sep 2020 18:18:19 +0200 Subject: [PATCH] No-op Instruction analyzer (#7249) * analyzer, tests and fixes * error check * gazelle * add more asserts * Merge branch 'master' into nop-analyzer * Merge refs/heads/master into nop-analyzer * fix no-op in blst package * Merge refs/heads/master into nop-analyzer * add assert after mutation * mark test as TODO * add issue number to TODO --- BUILD.bazel | 1 + nogo_config.json | 7 +++ shared/bls/blst/signature.go | 3 +- shared/bls/blst/signature_test.go | 15 ++++++ shared/bls/herumi/signature.go | 3 +- shared/bls/herumi/signature_test.go | 44 +++++++++++------- tools/analyzers/nop/BUILD.bazel | 28 +++++++++++ tools/analyzers/nop/analyzer.go | 59 ++++++++++++++++++++++++ tools/analyzers/nop/analyzer_test.go | 11 +++++ tools/analyzers/nop/testdata/BUILD.bazel | 8 ++++ tools/analyzers/nop/testdata/no_op.go | 14 ++++++ 11 files changed, 175 insertions(+), 18 deletions(-) create mode 100644 tools/analyzers/nop/BUILD.bazel create mode 100644 tools/analyzers/nop/analyzer.go create mode 100644 tools/analyzers/nop/analyzer_test.go create mode 100644 tools/analyzers/nop/testdata/BUILD.bazel create mode 100644 tools/analyzers/nop/testdata/no_op.go diff --git a/BUILD.bazel b/BUILD.bazel index 38dd72874..765211313 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -110,6 +110,7 @@ nogo( "//tools/analyzers/featureconfig:go_tool_library", "//tools/analyzers/comparesame:go_tool_library", "//tools/analyzers/shadowpredecl:go_tool_library", + "//tools/analyzers/nop:go_tool_library", ] + select({ # nogo checks that fail with coverage enabled. ":coverage_enabled": [], diff --git a/nogo_config.json b/nogo_config.json index 73a4fccbd..787fa73e5 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -123,5 +123,12 @@ "rules_go_work-.*": "Third party code", "tools/analyzers/shadowpredecl/testdata/shadow.go": "Analyzer testdata has to break rules" } + }, + "nop": { + "exclude_files": { + "external/.*": "Third party code", + "rules_go_work-.*": "Third party code", + "tools/analyzers/nop/testdata/no_op.go": "Analyzer testdata has to break rules" + } } } diff --git a/shared/bls/blst/signature.go b/shared/bls/blst/signature.go index d2c1600b8..ac7db7805 100644 --- a/shared/bls/blst/signature.go +++ b/shared/bls/blst/signature.go @@ -205,7 +205,8 @@ func (s *Signature) Marshal() []byte { // Copy returns a full deep copy of a signature. func (s *Signature) Copy() iface.Signature { - return &Signature{s: &*s.s} + sign := *s.s + return &Signature{s: &sign} } // VerifyCompressed verifies that the compressed signature and pubkey diff --git a/shared/bls/blst/signature_test.go b/shared/bls/blst/signature_test.go index 125d4ba0e..843ad47b5 100644 --- a/shared/bls/blst/signature_test.go +++ b/shared/bls/blst/signature_test.go @@ -137,3 +137,18 @@ func TestSignatureFromBytes(t *testing.T) { }) } } + +// TODO(7249): Make this test work +/*func TestCopy(t *testing.T) { + key := RandKey().(*bls12SecretKey) + signatureA := &Signature{s: new(blstSignature).Sign(key.p, []byte("foo"), dst)} + signatureB, ok := signatureA.Copy().(*Signature) + require.Equal(t, true, ok) + + assert.NotEqual(t, signatureA, signatureB) + assert.NotEqual(t, signatureA.s, signatureB.s) + assert.DeepEqual(t, signatureA, signatureB) + + signatureA.s = new(blstSignature).Sign(key.p, []byte("bar"), dst) + assert.DeepNotEqual(t, signatureA, signatureB) +}*/ diff --git a/shared/bls/herumi/signature.go b/shared/bls/herumi/signature.go index 008e1450f..3249771ae 100644 --- a/shared/bls/herumi/signature.go +++ b/shared/bls/herumi/signature.go @@ -210,5 +210,6 @@ func (s *Signature) Marshal() []byte { // Copy returns a full deep copy of a signature. func (s *Signature) Copy() iface.Signature { - return &Signature{s: &*s.s} + sign := *s.s + return &Signature{s: &sign} } diff --git a/shared/bls/herumi/signature_test.go b/shared/bls/herumi/signature_test.go index 79efeb2b7..e29c8a9c8 100644 --- a/shared/bls/herumi/signature_test.go +++ b/shared/bls/herumi/signature_test.go @@ -1,18 +1,17 @@ -package herumi_test +package herumi import ( "errors" "testing" bls12 "github.com/herumi/bls-eth-go-binary/bls" - "github.com/prysmaticlabs/prysm/shared/bls/herumi" "github.com/prysmaticlabs/prysm/shared/bls/iface" "github.com/prysmaticlabs/prysm/shared/testutil/assert" "github.com/prysmaticlabs/prysm/shared/testutil/require" ) func TestSignVerify(t *testing.T) { - priv := herumi.RandKey() + priv := RandKey() pub := priv.PublicKey() msg := []byte("hello") sig := priv.Sign(msg) @@ -25,14 +24,14 @@ func TestAggregateVerify(t *testing.T) { var msgs [][32]byte for i := 0; i < 100; i++ { msg := [32]byte{'h', 'e', 'l', 'l', 'o', byte(i)} - priv := herumi.RandKey() + priv := RandKey() pub := priv.PublicKey() sig := priv.Sign(msg[:]) pubkeys = append(pubkeys, pub) sigs = append(sigs, sig) msgs = append(msgs, msg) } - aggSig := herumi.Aggregate(sigs) + aggSig := Aggregate(sigs) assert.DeepEqual(t, true, aggSig.AggregateVerify(pubkeys, msgs)) } @@ -41,13 +40,13 @@ func TestFastAggregateVerify(t *testing.T) { sigs := make([]iface.Signature, 0, 100) msg := [32]byte{'h', 'e', 'l', 'l', 'o'} for i := 0; i < 100; i++ { - priv := herumi.RandKey() + priv := RandKey() pub := priv.PublicKey() sig := priv.Sign(msg[:]) pubkeys = append(pubkeys, pub) sigs = append(sigs, sig) } - aggSig := herumi.AggregateSignatures(sigs) + aggSig := AggregateSignatures(sigs) assert.DeepEqual(t, true, aggSig.FastAggregateVerify(pubkeys, msg)) } @@ -57,14 +56,14 @@ func TestMultipleSignatureVerification(t *testing.T) { var msgs [][32]byte for i := 0; i < 100; i++ { msg := [32]byte{'h', 'e', 'l', 'l', 'o', byte(i)} - priv := herumi.RandKey() + priv := RandKey() pub := priv.PublicKey() sig := priv.Sign(msg[:]) pubkeys = append(pubkeys, pub) sigs = append(sigs, sig) msgs = append(msgs, msg) } - verify, err := herumi.VerifyMultipleSignatures(sigs, msgs, pubkeys) + verify, err := VerifyMultipleSignatures(sigs, msgs, pubkeys) assert.NoError(t, err) assert.Equal(t, true, verify, "Signature did not verify") } @@ -75,7 +74,7 @@ func TestMultipleSignatureVerification_FailsCorrectly(t *testing.T) { var msgs [][32]byte for i := 0; i < 100; i++ { msg := [32]byte{'h', 'e', 'l', 'l', 'o', byte(i)} - priv := herumi.RandKey() + priv := RandKey() pub := priv.PublicKey() sig := priv.Sign(msg[:]) pubkeys = append(pubkeys, pub) @@ -113,21 +112,21 @@ func TestMultipleSignatureVerification_FailsCorrectly(t *testing.T) { bls12.G2Add(firstG2, firstG2, g2Point) bls12.G2Sub(secondG2, secondG2, g2Point) - lastSig, err := herumi.SignatureFromBytes(rawSig.Serialize()) + lastSig, err := SignatureFromBytes(rawSig.Serialize()) require.NoError(t, err) - secondLastSig, err = herumi.SignatureFromBytes(rawSig2.Serialize()) + secondLastSig, err = SignatureFromBytes(rawSig2.Serialize()) require.NoError(t, err) sigs[len(sigs)-1] = lastSig sigs[len(sigs)-2] = secondLastSig // This method is expected to pass, as it would not // be able to detect bad signatures - aggSig := herumi.AggregateSignatures(sigs) + aggSig := AggregateSignatures(sigs) if !aggSig.AggregateVerify(pubkeys, msgs) { t.Error("Signature did not verify") } // This method would be expected to fail. - verify, err := herumi.VerifyMultipleSignatures(sigs, msgs, pubkeys) + verify, err := VerifyMultipleSignatures(sigs, msgs, pubkeys) assert.NoError(t, err) assert.Equal(t, false, verify, "Signature verified when it was not supposed to") } @@ -136,7 +135,7 @@ func TestFastAggregateVerify_ReturnsFalseOnEmptyPubKeyList(t *testing.T) { var pubkeys []iface.PublicKey msg := [32]byte{'h', 'e', 'l', 'l', 'o'} - aggSig := herumi.NewAggregateSignature() + aggSig := NewAggregateSignature() if aggSig.FastAggregateVerify(pubkeys, msg) != false { t.Error("Expected FastAggregateVerify to return false with empty input " + "of public keys.") @@ -181,7 +180,7 @@ func TestSignatureFromBytes(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - res, err := herumi.SignatureFromBytes(test.input) + res, err := SignatureFromBytes(test.input) if test.err != nil { assert.ErrorContains(t, test.err.Error(), err) } else { @@ -191,3 +190,16 @@ func TestSignatureFromBytes(t *testing.T) { }) } } + +func TestCopy(t *testing.T) { + signatureA := &Signature{s: bls12.HashAndMapToSignature([]byte("foo"))} + signatureB, ok := signatureA.Copy().(*Signature) + require.Equal(t, true, ok) + + assert.NotEqual(t, signatureA, signatureB) + assert.NotEqual(t, signatureA.s, signatureB.s) + assert.DeepEqual(t, signatureA, signatureB) + + signatureA.s.Add(bls12.HashAndMapToSignature([]byte("bar"))) + assert.DeepNotEqual(t, signatureA, signatureB) +} diff --git a/tools/analyzers/nop/BUILD.bazel b/tools/analyzers/nop/BUILD.bazel new file mode 100644 index 000000000..2e1a05666 --- /dev/null +++ b/tools/analyzers/nop/BUILD.bazel @@ -0,0 +1,28 @@ +load("@prysm//tools/go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/nop", + 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 = "github.com/prysmaticlabs/prysm/tools/analyzers/nop", + 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", + ], +) + +# gazelle:exclude analyzer_test.go diff --git a/tools/analyzers/nop/analyzer.go b/tools/analyzers/nop/analyzer.go new file mode 100644 index 000000000..66b885d54 --- /dev/null +++ b/tools/analyzers/nop/analyzer.go @@ -0,0 +1,59 @@ +package nop + +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 = "Tool to detect no-op instructions." + +const message = "Found a no-op instruction that can be safely removed. " + + "It might be a result of writing code that does not do what was intended." + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "nop", + 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.StarExpr)(nil), + (*ast.UnaryExpr)(nil), + } + + inspect.Preorder(nodeFilter, func(node ast.Node) { + switch expr := node.(type) { + case *ast.StarExpr: + unaryExpr, ok := expr.X.(*ast.UnaryExpr) + if !ok { + return + } + + if unaryExpr.Op == token.AND { + pass.Reportf(expr.Star, message) + } + case *ast.UnaryExpr: + if expr.Op == token.AND { + if _, ok := expr.X.(*ast.StarExpr); ok { + pass.Reportf(expr.OpPos, message) + } + } + } + }) + + return nil, nil +} diff --git a/tools/analyzers/nop/analyzer_test.go b/tools/analyzers/nop/analyzer_test.go new file mode 100644 index 000000000..be1180af8 --- /dev/null +++ b/tools/analyzers/nop/analyzer_test.go @@ -0,0 +1,11 @@ +package nop + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer) +} diff --git a/tools/analyzers/nop/testdata/BUILD.bazel b/tools/analyzers/nop/testdata/BUILD.bazel new file mode 100644 index 000000000..d22fd1d81 --- /dev/null +++ b/tools/analyzers/nop/testdata/BUILD.bazel @@ -0,0 +1,8 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["no_op.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/nop/testdata", + visibility = ["//visibility:public"], +) diff --git a/tools/analyzers/nop/testdata/no_op.go b/tools/analyzers/nop/testdata/no_op.go new file mode 100644 index 000000000..b10d1fdec --- /dev/null +++ b/tools/analyzers/nop/testdata/no_op.go @@ -0,0 +1,14 @@ +package testdata + +type foo struct { +} + +func AddressOfDereferencedValue() { + x := &foo{} + _ = &*x // want "Found a no-op instruction that can be safely removed. It might be a result of writing code that does not work as expected." +} + +func DereferencedAddressOfValue() { + x := foo{} + _ = *&x // want "Found a no-op instruction that can be safely removed. It might be a result of writing code that does not work as expected." +}