From effba1fa47317ca4b341c10590c37baaed43ef1e Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 11 Jun 2024 17:51:41 +0300 Subject: [PATCH] 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) +}