From 08427f23b67b74439c91389a65c0135922f768c4 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 29 Sep 2022 15:13:29 +0300 Subject: [PATCH 1/3] compiler: do not check Any event parameter for compliance It's possible that declared manifest event has parameter of AnyT for those cases when parameter type differs from method to method. If so, then we don't need to enforce type check after compilation. --- pkg/compiler/compiler.go | 4 ++++ pkg/compiler/compiler_test.go | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/pkg/compiler/compiler.go b/pkg/compiler/compiler.go index 2bfa66d0d..8ee039189 100644 --- a/pkg/compiler/compiler.go +++ b/pkg/compiler/compiler.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strings" + "github.com/nspcc-dev/neo-go/pkg/smartcontract" "github.com/nspcc-dev/neo-go/pkg/smartcontract/binding" "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest/standard" @@ -363,6 +364,9 @@ func CreateManifest(di *DebugInfo, o *Options) (*manifest.Manifest, error) { name, len(ev.Parameters), len(argsList[i])) } for j := range ev.Parameters { + if ev.Parameters[j].Type == smartcontract.AnyType { + continue + } expected := ev.Parameters[j].Type.String() if argsList[i][j] != expected { return nil, fmt.Errorf("event '%s' should have '%s' as type of %d parameter, "+ diff --git a/pkg/compiler/compiler_test.go b/pkg/compiler/compiler_test.go index 0e0756128..7fc6db6a6 100644 --- a/pkg/compiler/compiler_test.go +++ b/pkg/compiler/compiler_test.go @@ -174,6 +174,16 @@ func TestEventWarnings(t *testing.T) { }) require.Error(t, err) }) + t.Run("any parameter type", func(t *testing.T) { + _, err = compiler.CreateManifest(di, &compiler.Options{ + ContractEvents: []manifest.Event{{ + Name: "Event", + Parameters: []manifest.Parameter{manifest.NewParameter("number", smartcontract.AnyType)}, + }}, + Name: "payable", + }) + require.NoError(t, err) + }) t.Run("good", func(t *testing.T) { _, err = compiler.CreateManifest(di, &compiler.Options{ ContractEvents: []manifest.Event{{ From 80f71a4e6efbcc9030a562cb856d6c400590ca24 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 29 Sep 2022 15:41:53 +0300 Subject: [PATCH 2/3] compiler: do not enforce variadic event args check on ellipsis usage In case of ellipsis usage compiler defines argument type as ArrayT (which is correct, because it's a natural representation of the last argument, it represents the array of interface{}). Here goes the problem: ``` === RUN TestEventWarnings/variadic_event_args_via_ellipsis compiler_test.go:251: Error Trace: compiler_test.go:251 Error: Received unexpected error: event 'Event' should have 'Integer' as type of 1 parameter, got: Array Test: TestEventWarnings/variadic_event_args_via_ellipsis ``` Parsing the last argument in this case is a separate complicated problem due to the fact that we need to grab types of elements of []interface{} inside the fully qualified ast node which may looks like: ``` runtime.Notify("Event", (append([]interface{}{1, 2}, (([]interface{}{someVar, 4}))...))...) ``` Temporary solution is to exclude such notifications from analysis until we're able to properly resolve element types of []interface{}. --- pkg/compiler/compiler_test.go | 19 +++++++++++++++++++ pkg/compiler/inline.go | 15 ++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/pkg/compiler/compiler_test.go b/pkg/compiler/compiler_test.go index 7fc6db6a6..142c33110 100644 --- a/pkg/compiler/compiler_test.go +++ b/pkg/compiler/compiler_test.go @@ -230,6 +230,25 @@ func TestEventWarnings(t *testing.T) { require.NoError(t, err) }) }) + t.Run("variadic event args via ellipsis", func(t *testing.T) { + src := `package payable + import "github.com/nspcc-dev/neo-go/pkg/interop/runtime" + func Main() { + runtime.Notify("Event", []interface{}{1}...) + }` + + _, di, err := compiler.CompileWithOptions("eventTest.go", strings.NewReader(src), nil) + require.NoError(t, err) + + _, err = compiler.CreateManifest(di, &compiler.Options{ + Name: "eventTest", + ContractEvents: []manifest.Event{{ + Name: "Event", + Parameters: []manifest.Parameter{manifest.NewParameter("number", smartcontract.IntegerType)}, + }}, + }) + require.NoError(t, err) + }) } func TestNotifyInVerify(t *testing.T) { diff --git a/pkg/compiler/inline.go b/pkg/compiler/inline.go index 3fa502b0f..56884830a 100644 --- a/pkg/compiler/inline.go +++ b/pkg/compiler/inline.go @@ -36,7 +36,8 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { pkg := c.packageCache[f.pkg.Path()] sig := c.typeOf(n.Fun).(*types.Signature) - c.processStdlibCall(f, n.Args) + hasVarArgs := !n.Ellipsis.IsValid() + c.processStdlibCall(f, n.Args, !hasVarArgs) // When inlined call is used during global initialization // there is no func scope, thus this if. @@ -68,7 +69,6 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { scope: oldScope, }) } - hasVarArgs := !n.Ellipsis.IsValid() needPack := sig.Variadic() && hasVarArgs for i := range n.Args { c.scope.vars.locals = oldScope @@ -129,13 +129,13 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { c.pkgInfoInline = c.pkgInfoInline[:len(c.pkgInfoInline)-1] } -func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr) { +func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr, hasEllipsis bool) { if f == nil { return } if f.pkg.Path() == interopPrefix+"/runtime" && (f.name == "Notify" || f.name == "Log") { - c.processNotify(f, args) + c.processNotify(f, args, hasEllipsis) } if f.pkg.Path() == interopPrefix+"/contract" && f.name == "Call" { @@ -143,7 +143,7 @@ func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr) { } } -func (c *codegen) processNotify(f *funcScope, args []ast.Expr) { +func (c *codegen) processNotify(f *funcScope, args []ast.Expr, hasEllipsis bool) { if c.scope != nil && c.isVerifyFunc(c.scope.decl) && c.scope.pkg == c.mainPkg.Types && (c.buildInfo.options == nil || !c.buildInfo.options.NoEventsCheck) { c.prog.Err = fmt.Errorf("runtime.%s is not allowed in `Verify`", f.name) @@ -154,10 +154,11 @@ func (c *codegen) processNotify(f *funcScope, args []ast.Expr) { return } - // Sometimes event name is stored in a var. + // Sometimes event name is stored in a var. Or sometimes event args are provided + // via ellipses (`slice...`). // Skip in this case. tv := c.typeAndValueOf(args[0]) - if tv.Value == nil { + if tv.Value == nil || hasEllipsis { return } From 554e48e7b766892f3b3412218fa25ad6445b5b50 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 29 Sep 2022 16:53:28 +0300 Subject: [PATCH 3/3] compiler: enforce runtime.Notify parameters cast If notification parameters type can be defined in a compile time then enforce parameter cast to the desired type got from manifest. --- docs/compiler.md | 8 +- pkg/compiler/inline.go | 67 +++++++++++-- pkg/compiler/interop_test.go | 144 +++++++++++++++++++++++++++ pkg/smartcontract/param_type.go | 35 +++++++ pkg/smartcontract/param_type_test.go | 25 +++++ 5 files changed, 266 insertions(+), 13 deletions(-) diff --git a/docs/compiler.md b/docs/compiler.md index 7d5810177..339efd057 100644 --- a/docs/compiler.md +++ b/docs/compiler.md @@ -289,8 +289,12 @@ As an example, consider `Transfer` event from `NEP-17` standard: By default, compiler performs some sanity checks. Most of the time it will report missing events and/or parameter type mismatch. It isn't prohibited to use a variable as an event name in code, but it will prevent -the compiler from analyzing the event. It is better to use either constant or string literal. -The check can be disabled with `--no-events` flag. +the compiler from analyzing the event. It is better to use either constant or string literal. +It isn't prohibited to use ellipsis expression as an event arguments, but it will also +prevent the compiler from analyzing the event. It is better to provide arguments directly +without `...`. The type conversion code will be emitted for checked events, it will cast +argument types to ones specified in the contract manifest. These checks and conversion can +be disabled with `--no-events` flag. ##### Permissions Each permission specifies contracts and methods allowed for this permission. diff --git a/pkg/compiler/inline.go b/pkg/compiler/inline.go index 56884830a..0f82d95ba 100644 --- a/pkg/compiler/inline.go +++ b/pkg/compiler/inline.go @@ -11,6 +11,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/util" "github.com/nspcc-dev/neo-go/pkg/vm/emit" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" + "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" ) // inlineCall inlines call of n for function represented by f. @@ -37,7 +38,7 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { sig := c.typeOf(n.Fun).(*types.Signature) hasVarArgs := !n.Ellipsis.IsValid() - c.processStdlibCall(f, n.Args, !hasVarArgs) + eventParams := c.processStdlibCall(f, n.Args, !hasVarArgs) // When inlined call is used during global initialization // there is no func scope, thus this if. @@ -102,6 +103,11 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { c.scope.vars.locals = oldScope for i := sig.Params().Len() - 1; i < len(n.Args); i++ { ast.Walk(c, n.Args[i]) + // In case of runtime.Notify, its arguments need to be converted to proper type. + // i's initial value is 1 (variadic args start). + if eventParams != nil && eventParams[i-1] != nil { + c.emitConvert(*eventParams[i-1]) + } } c.scope.vars.locals = newScope c.packVarArgs(n, sig) @@ -129,52 +135,91 @@ func (c *codegen) inlineCall(f *funcScope, n *ast.CallExpr) { c.pkgInfoInline = c.pkgInfoInline[:len(c.pkgInfoInline)-1] } -func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr, hasEllipsis bool) { +func (c *codegen) processStdlibCall(f *funcScope, args []ast.Expr, hasEllipsis bool) []*stackitem.Type { if f == nil { - return + return nil } + var eventParams []*stackitem.Type if f.pkg.Path() == interopPrefix+"/runtime" && (f.name == "Notify" || f.name == "Log") { - c.processNotify(f, args, hasEllipsis) + eventParams = c.processNotify(f, args, hasEllipsis) } if f.pkg.Path() == interopPrefix+"/contract" && f.name == "Call" { c.processContractCall(f, args) } + return eventParams } -func (c *codegen) processNotify(f *funcScope, args []ast.Expr, hasEllipsis bool) { +// processNotify checks whether notification emitting rules are met and returns expected +// notification signature if found. +func (c *codegen) processNotify(f *funcScope, args []ast.Expr, hasEllipsis bool) []*stackitem.Type { if c.scope != nil && c.isVerifyFunc(c.scope.decl) && c.scope.pkg == c.mainPkg.Types && (c.buildInfo.options == nil || !c.buildInfo.options.NoEventsCheck) { c.prog.Err = fmt.Errorf("runtime.%s is not allowed in `Verify`", f.name) - return + return nil } if f.name == "Log" { - return + return nil } // Sometimes event name is stored in a var. Or sometimes event args are provided // via ellipses (`slice...`). - // Skip in this case. + // Skip in this case. Also, don't enforce runtime.Notify parameters conversion. tv := c.typeAndValueOf(args[0]) if tv.Value == nil || hasEllipsis { - return + return nil } params := make([]string, 0, len(args[1:])) + vParams := make([]*stackitem.Type, 0, len(args[1:])) for _, p := range args[1:] { - st, _, _ := c.scAndVMTypeFromExpr(p) + st, vt, _ := c.scAndVMTypeFromExpr(p) params = append(params, st.String()) + vParams = append(vParams, &vt) } name := constant.StringVal(tv.Value) if len(name) > runtime.MaxEventNameLen { c.prog.Err = fmt.Errorf("event name '%s' should be less than %d", name, runtime.MaxEventNameLen) - return + return nil + } + var eventFound bool + if c.buildInfo.options != nil && c.buildInfo.options.ContractEvents != nil && !c.buildInfo.options.NoEventsCheck { + for _, e := range c.buildInfo.options.ContractEvents { + if e.Name == name && len(e.Parameters) == len(vParams) { + eventFound = true + for i, scParam := range e.Parameters { + expectedType := scParam.Type.ConvertToStackitemType() + // No need to cast if the desired type is unknown. + if expectedType == stackitem.AnyT || + // Do not cast if desired type is Interop, the actual type is likely to be Any, leave the resolving to runtime.Notify. + expectedType == stackitem.InteropT || + // No need to cast if actual parameter type matches the desired one. + *vParams[i] == expectedType || + // expectedType doesn't contain Buffer anyway, but if actual variable type is Buffer, + // then runtime.Notify will convert it to ByteArray automatically, thus no need to emit conversion code. + (*vParams[i] == stackitem.BufferT && expectedType == stackitem.ByteArrayT) { + vParams[i] = nil + } else { + // For other cases the conversion code will be emitted using vParams... + vParams[i] = &expectedType + // ...thus, update emitted notification info in advance. + params[i] = scParam.Type.String() + } + } + } + } } c.emittedEvents[name] = append(c.emittedEvents[name], params) + // Do not enforce perfect expected/actual events match on this step, the final + // check wil be performed after compilation if --no-events option is off. + if eventFound { + return vParams + } + return nil } func (c *codegen) processContractCall(f *funcScope, args []ast.Expr) { diff --git a/pkg/compiler/interop_test.go b/pkg/compiler/interop_test.go index 07a617560..360c81f3a 100644 --- a/pkg/compiler/interop_test.go +++ b/pkg/compiler/interop_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "math/big" + "strconv" "strings" "testing" @@ -22,6 +23,7 @@ import ( cinterop "github.com/nspcc-dev/neo-go/pkg/interop" "github.com/nspcc-dev/neo-go/pkg/neotest" "github.com/nspcc-dev/neo-go/pkg/neotest/chain" + "github.com/nspcc-dev/neo-go/pkg/smartcontract" "github.com/nspcc-dev/neo-go/pkg/smartcontract/callflag" "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" "github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger" @@ -548,3 +550,145 @@ func TestCallWithVersion(t *testing.T) { c.InvokeFail(t, "contract version mismatch", "callWithVersion", policyH.BytesBE(), 1, "getExecFeeFactor") }) } + +func TestForcedNotifyArgumentsConversion(t *testing.T) { + const methodWithEllipsis = "withEllipsis" + const methodWithoutEllipsis = "withoutEllipsis" + check := func(t *testing.T, method string, targetSCParamTypes []smartcontract.ParamType, expectedVMParamTypes []stackitem.Type, noEventsCheck bool) { + bc, acc := chain.NewSingle(t) + e := neotest.NewExecutor(t, bc, acc, acc) + src := `package foo + import "github.com/nspcc-dev/neo-go/pkg/interop/runtime" + const arg4 = 4 // Const value. + func WithoutEllipsis() { + var arg0 int // Default value. + var arg1 int = 1 // Initialized value. + arg2 := 2 // Short decl. + var arg3 int + arg3 = 3 // Declare first, change value afterwards. + runtime.Notify("withoutEllipsis", arg0, arg1, arg2, arg3, arg4, 5, f(6)) // The fifth argument is basic literal. + } + func WithEllipsis() { + arg := []interface{}{0, 1, f(2), 3, 4, 5, 6} + runtime.Notify("withEllipsis", arg...) + } + func f(i int) int { + return i + }` + count := len(targetSCParamTypes) + if count != len(expectedVMParamTypes) { + t.Fatalf("parameters count mismatch: %d vs %d", count, len(expectedVMParamTypes)) + } + scParams := make([]manifest.Parameter, len(targetSCParamTypes)) + vmParams := make([]stackitem.Item, len(expectedVMParamTypes)) + for i := range scParams { + scParams[i] = manifest.Parameter{ + Name: strconv.Itoa(i), + Type: targetSCParamTypes[i], + } + defaultValue := stackitem.NewBigInteger(big.NewInt(int64(i))) + var ( + val stackitem.Item + err error + ) + if expectedVMParamTypes[i] == stackitem.IntegerT { + val = defaultValue + } else { + val, err = defaultValue.Convert(expectedVMParamTypes[i]) // exactly the same conversion should be emitted by compiler and performed by the contract code. + require.NoError(t, err) + } + vmParams[i] = val + } + ctr := neotest.CompileSource(t, e.CommitteeHash, strings.NewReader(src), &compiler.Options{ + Name: "Helper", + ContractEvents: []manifest.Event{ + { + Name: methodWithoutEllipsis, + Parameters: scParams, + }, + { + Name: methodWithEllipsis, + Parameters: scParams, + }, + }, + NoEventsCheck: noEventsCheck, + }) + e.DeployContract(t, ctr, nil) + c := e.CommitteeInvoker(ctr.Hash) + + t.Run(method, func(t *testing.T) { + h := c.Invoke(t, stackitem.Null{}, method) + aer := c.GetTxExecResult(t, h) + require.Equal(t, 1, len(aer.Events)) + require.Equal(t, stackitem.NewArray(vmParams), aer.Events[0].Item) + }) + } + checkSingleType := func(t *testing.T, method string, targetSCEventType smartcontract.ParamType, expectedVMType stackitem.Type, noEventsCheck ...bool) { + count := 7 + scParams := make([]smartcontract.ParamType, count) + vmParams := make([]stackitem.Type, count) + for i := range scParams { + scParams[i] = targetSCEventType + vmParams[i] = expectedVMType + } + var noEvents bool + if len(noEventsCheck) > 0 { + noEvents = noEventsCheck[0] + } + check(t, method, scParams, vmParams, noEvents) + } + + t.Run("good, single type, default values", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.IntegerType, stackitem.IntegerT) + }) + t.Run("good, single type, conversion to BooleanT", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.BoolType, stackitem.BooleanT) + }) + t.Run("good, single type, Hash160Type->ByteArray", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.Hash160Type, stackitem.ByteArrayT) + }) + t.Run("good, single type, Hash256Type->ByteArray", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.Hash256Type, stackitem.ByteArrayT) + }) + t.Run("good, single type, Signature->ByteArray", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.SignatureType, stackitem.ByteArrayT) + }) + t.Run("good, single type, String->ByteArray", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.StringType, stackitem.ByteArrayT) // Special case, runtime.Notify will convert any Buffer to ByteArray. + }) + t.Run("good, single type, PublicKeyType->ByteArray", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.PublicKeyType, stackitem.ByteArrayT) + }) + t.Run("good, single type, AnyType->do not change initial type", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.AnyType, stackitem.IntegerT) // Special case, compiler should leave the type "as is" and do not emit conversion code. + }) + // Test for InteropInterface->... is missing, because we don't enforce conversion to stackitem.InteropInterface, + // but compiler still checks these notifications against expected manifest. + t.Run("good, multiple types, check the conversion order", func(t *testing.T) { + check(t, methodWithoutEllipsis, []smartcontract.ParamType{ + smartcontract.IntegerType, + smartcontract.BoolType, + smartcontract.ByteArrayType, + smartcontract.PublicKeyType, + smartcontract.Hash160Type, + smartcontract.AnyType, // leave initial type + smartcontract.StringType, + }, []stackitem.Type{ + stackitem.IntegerT, + stackitem.BooleanT, + stackitem.ByteArrayT, + stackitem.ByteArrayT, + stackitem.ByteArrayT, + stackitem.IntegerT, // leave initial type + stackitem.ByteArrayT, + }, false) + }) + t.Run("with ellipsis, do not emit conversion code", func(t *testing.T) { + checkSingleType(t, methodWithEllipsis, smartcontract.IntegerType, stackitem.IntegerT) + checkSingleType(t, methodWithEllipsis, smartcontract.BoolType, stackitem.IntegerT) + checkSingleType(t, methodWithEllipsis, smartcontract.ByteArrayType, stackitem.IntegerT) + }) + t.Run("no events check => no conversion code", func(t *testing.T) { + checkSingleType(t, methodWithoutEllipsis, smartcontract.PublicKeyType, stackitem.IntegerT, true) + }) +} diff --git a/pkg/smartcontract/param_type.go b/pkg/smartcontract/param_type.go index 0160e357c..5b36de52f 100644 --- a/pkg/smartcontract/param_type.go +++ b/pkg/smartcontract/param_type.go @@ -331,3 +331,38 @@ func ConvertToParamType(val int) (ParamType, error) { } return UnknownType, errors.New("unknown parameter type") } + +// ConvertToStackitemType converts ParamType to corresponding Stackitem.Type. +func (pt ParamType) ConvertToStackitemType() stackitem.Type { + switch pt { + case SignatureType: + return stackitem.ByteArrayT + case BoolType: + return stackitem.BooleanT + case IntegerType: + return stackitem.IntegerT + case Hash160Type: + return stackitem.ByteArrayT + case Hash256Type: + return stackitem.ByteArrayT + case ByteArrayType: + return stackitem.ByteArrayT + case PublicKeyType: + return stackitem.ByteArrayT + case StringType: + // Do not use BufferT to match System.Runtime.Notify conversion rules. + return stackitem.ByteArrayT + case ArrayType: + return stackitem.ArrayT + case MapType: + return stackitem.MapT + case InteropInterfaceType: + return stackitem.InteropT + case VoidType: + return stackitem.AnyT + case AnyType: + return stackitem.AnyT + default: + panic(fmt.Sprintf("unknown param type %d", pt)) + } +} diff --git a/pkg/smartcontract/param_type_test.go b/pkg/smartcontract/param_type_test.go index 181c584f8..a9f52c11d 100644 --- a/pkg/smartcontract/param_type_test.go +++ b/pkg/smartcontract/param_type_test.go @@ -393,3 +393,28 @@ func TestConvertToParamType(t *testing.T) { _, err := ConvertToParamType(0x01) require.NotNil(t, err) } + +func TestConvertToStackitemType(t *testing.T) { + for p, expected := range map[ParamType]stackitem.Type{ + AnyType: stackitem.AnyT, + BoolType: stackitem.BooleanT, + IntegerType: stackitem.IntegerT, + ByteArrayType: stackitem.ByteArrayT, + StringType: stackitem.ByteArrayT, + Hash160Type: stackitem.ByteArrayT, + Hash256Type: stackitem.ByteArrayT, + PublicKeyType: stackitem.ByteArrayT, + SignatureType: stackitem.ByteArrayT, + ArrayType: stackitem.ArrayT, + MapType: stackitem.MapT, + InteropInterfaceType: stackitem.InteropT, + VoidType: stackitem.AnyT, + } { + actual := p.ConvertToStackitemType() + require.Equal(t, expected, actual) + } + + require.Panics(t, func() { + UnknownType.ConvertToStackitemType() + }) +}