From 55fa12355e969d81124ea360234281e83bbc62ab Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 11 Jun 2024 17:50:53 +0300 Subject: [PATCH 1/3] stackitem: extend Make() with []string Allow []string to be converted to stackitem.Item. Signed-off-by: Anna Shaleva --- pkg/vm/stackitem/item.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index 33ff664bf..6eacbea18 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -128,6 +128,12 @@ func Make(v any) Item { a = append(a, Make(i)) } return Make(a) + case []string: + var a []Item + for _, i := range val { + a = append(a, Make(i)) + } + return Make(a) case []any: res := make([]Item, len(val)) for i := range val { From effba1fa47317ca4b341c10590c37baaed43ef1e Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 11 Jun 2024 17:51:41 +0300 Subject: [PATCH 2/3] core: ensure System.Runtime.GetNotifications can't break MaxStackSize This test ensures that NeoGo node doesn't have the DeepCopy problem described in https://github.com/neo-project/neo/issues/3300 and fixed in https://github.com/neo-project/neo/pull/3301. This problem leads to the fact that Notifications items are not being properly refcounted by C# node which leads to possibility to build an enormously large object on stack. Go node doesn't have this problem. The reason (at least, as I understand it) is in the fact that C# node performs objects refcounting inside the DeepCopy even if the object itself is not yet on stack. I.e. System.Runtime.Notify handler immediately adds references to the notification argumetns inside DeepCopy: https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L108 https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L75 Whereas Go node just performs the honest DeepCopy without references counting: https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stackitem/item.go#L1223 Going further, C# node clears refs for notification arguments (for array and underlying array items). System.Runtime.GetNotifications pushes the notificaiton args array back on stack and increments counter only for the external array, not for its arguments. Which results in negative refcounter once notificaiton is removed from the stack. The fix itself (https://github.com/Jim8y/neo/blob/f471c0542ddd8f527478fbdcda76a3ab9194b958/src/Neo/SmartContract/NotifyEventArgs.cs#L84) doesn't need to be ported to NeoGo because Go node adds object to the refcounter only at the moment when it's being pushed to stack by System.Runtime.GetNotifications handler. This object is treated as new object since it was deepcopied earlier by System.Runtime.Notify handler: https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stack.go#L178. Thus, no functoinal changes from the NeoGo side. And we won't intentionally break our node to follow C# pre-Domovoi invalid behaviour. Close #3484, close #3482. Signed-off-by: Anna Shaleva --- pkg/core/blockchain_neotest_test.go | 75 +++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/pkg/core/blockchain_neotest_test.go b/pkg/core/blockchain_neotest_test.go index 5f70bdf37..37c7a87a5 100644 --- a/pkg/core/blockchain_neotest_test.go +++ b/pkg/core/blockchain_neotest_test.go @@ -2620,3 +2620,78 @@ func TestBlockchain_StoreAsTransaction_ExecutableConflict(t *testing.T) { require.NoError(t, err) require.Equal(t, 2, len(aer)) } + +// TestEngineLimits ensures that MaxStackSize limit is preserved during System.Runtime.GetNotifications +// syscall handling. This test is an adjusted port of https://github.com/lazynode/Tanya/pull/33 and makes +// sure that NeoGo node is not affected by https://github.com/neo-project/neo/issues/3300 and does not need +// the https://github.com/neo-project/neo/pull/3301. +func TestEngineLimits(t *testing.T) { + bc, acc := chain.NewSingle(t) + e := neotest.NewExecutor(t, bc, acc, acc) + + src := `package test + import ( + "github.com/nspcc-dev/neo-go/pkg/interop/runtime" + ) + // args is an array of LargeEvent parameters containing 500 empty strings. + var args = []any{"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "" }; + func ProduceNumerousNotifications(count int) [][]any { + for i := 0; i < count; i++ { + runtime.Notify("LargeEvent", args...) + } + return runtime.GetNotifications(runtime.GetExecutingScriptHash()) + } + func ProduceLargeObject(count int) int { + for i := 0; i < count; i++ { + runtime.Notify("LargeEvent", args...) + } + var ( + smallObject = make([]int, 100) + res []int + ) + for i := 0; i < count; i++ { + runtime.GetNotifications(runtime.GetExecutingScriptHash()) + res = append(res, smallObject...) + } + return len(res) + }` + const eArgsCount = 500 + eParams := make([]compiler.HybridParameter, eArgsCount) + for i := range eParams { + eParams[i].Name = fmt.Sprintf("str%d", i) + eParams[i].Type = smartcontract.ByteArrayType + } + c := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(src), &compiler.Options{ + Name: "test_contract", + ContractEvents: []compiler.HybridEvent{ + { + Name: "LargeEvent", + Parameters: eParams, + }, + }, + }) + e.DeployContract(t, c, nil) + + // ProduceNumerousNotifications: 1 iteration, no limits are hit. + var args = make([]stackitem.Item, eArgsCount) + for i := range args { + args[i] = stackitem.Make("") + } + cInv := e.NewInvoker(c.Hash, acc) + cInv.Invoke(t, stackitem.Make([]stackitem.Item{ + stackitem.Make([]stackitem.Item{ + stackitem.Make(c.Hash), + stackitem.Make("LargeEvent"), + stackitem.Make(args), + }), + }), "produceNumerousNotifications", 1) + + // ProduceNumerousNotifications: hit the limit. + cInv.InvokeFail(t, "stack is too big", "produceNumerousNotifications", 500) + + // ProduceLargeObject: 1 iteration, no limits are hit. + cInv.Invoke(t, 100*1, "produceLargeObject", 1) + + // ProduceLargeObject: hit the limit. + cInv.InvokeFail(t, "stack is too big", "produceLargeObject", 500) +} From d156cea24dbcc41a86f23b0e313f0d3a1f00957e Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 11 Jun 2024 18:34:22 +0300 Subject: [PATCH 3/3] vm: improve stack size related errors No functional changes, just add more details to the error. Signed-off-by: Anna Shaleva --- pkg/vm/vm.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index d3959f1e2..2b94cbcb7 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -594,7 +594,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro err = newError(ctx.ip, op, errRecover) } else if v.refs > MaxStackSize { v.state = vmstate.Fault - err = newError(ctx.ip, op, "stack is too big") + err = newError(ctx.ip, op, fmt.Sprintf("stack is too big: %d vs %d", int(v.refs), MaxStackSize)) } }() @@ -1995,7 +1995,7 @@ func validateMapKey(key Element) { func (v *VM) checkInvocationStackSize() { if len(v.istack) >= MaxInvocationStackSize { - panic("invocation stack is too big") + panic(fmt.Sprintf("invocation stack is too big: %d", len(v.istack))) } }