From 107f5e0793e89c9898217c016ce254db0c8160ec Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 31 May 2022 08:02:13 +0300 Subject: [PATCH 1/3] vm: adjust refcount operations order Perform add/set/remove operations with VM type firstly, and with refcounter after that. It is needed to be able to extend add/set/remove operations with extra checks. --- pkg/vm/vm.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index c9843a48d..bf3358145 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1301,24 +1301,28 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro if k < 0 || k >= len(a) { panic("REMOVE: invalid index") } - v.refs.Remove(a[k]) + toRemove := a[k] t.Remove(k) + v.refs.Remove(toRemove) case *stackitem.Struct: a := t.Value().([]stackitem.Item) k := toInt(key.BigInt()) if k < 0 || k >= len(a) { panic("REMOVE: invalid index") } - v.refs.Remove(a[k]) + toRemove := a[k] t.Remove(k) + v.refs.Remove(toRemove) case *stackitem.Map: index := t.Index(key.Item()) // NEO 2.0 doesn't error on missing key. if index >= 0 { elems := t.Value().([]stackitem.MapElement) - v.refs.Remove(elems[index].Key) - v.refs.Remove(elems[index].Value) + key := elems[index].Key + val := elems[index].Value t.Drop(index) + v.refs.Remove(key) + v.refs.Remove(val) } default: panic("REMOVE: invalid type") @@ -1353,6 +1357,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro elems := arr.Value().([]stackitem.Item) index := len(elems) - 1 elem := elems[index] + v.estack.PushItem(elem) // push item on stack firstly, to match the reference behaviour. switch item := arr.(type) { case *stackitem.Array: item.Remove(index) @@ -1360,7 +1365,6 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro item.Remove(index) } v.refs.Remove(elem) - v.estack.PushItem(elem) case opcode.SIZE: elem := v.estack.Pop() From 7296f0c9136a869d060c837c17689c2f83449e37 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 30 May 2022 10:41:00 +0300 Subject: [PATCH 2/3] vm: support immutable compound types --- pkg/core/interop/runtime/engine.go | 2 +- pkg/core/interop/runtime/engine_test.go | 1 + pkg/core/interop/runtime/util.go | 2 +- pkg/core/interop/runtime/util_test.go | 1 + pkg/core/interop_system_core_test.go | 1 + pkg/vm/opcodebench_test.go | 2 +- pkg/vm/stackitem/immutable.go | 21 ++++++++++ pkg/vm/stackitem/item.go | 53 +++++++++++++++++++++---- pkg/vm/stackitem/item_test.go | 16 +++++--- pkg/vm/vm.go | 18 +++++++++ 10 files changed, 102 insertions(+), 15 deletions(-) create mode 100644 pkg/vm/stackitem/immutable.go diff --git a/pkg/core/interop/runtime/engine.go b/pkg/core/interop/runtime/engine.go index f34358479..c5ff4a79b 100644 --- a/pkg/core/interop/runtime/engine.go +++ b/pkg/core/interop/runtime/engine.go @@ -68,7 +68,7 @@ func Notify(ic *interop.Context) error { if len(bytes) > MaxNotificationSize { return fmt.Errorf("notification size shouldn't exceed %d", MaxNotificationSize) } - ic.AddNotification(ic.VM.GetCurrentScriptHash(), name, stackitem.DeepCopy(stackitem.NewArray(args)).(*stackitem.Array)) + ic.AddNotification(ic.VM.GetCurrentScriptHash(), name, stackitem.DeepCopy(stackitem.NewArray(args), false).(*stackitem.Array)) return nil } diff --git a/pkg/core/interop/runtime/engine_test.go b/pkg/core/interop/runtime/engine_test.go index 41b1c2dc9..70769bc26 100644 --- a/pkg/core/interop/runtime/engine_test.go +++ b/pkg/core/interop/runtime/engine_test.go @@ -161,6 +161,7 @@ func TestNotify(t *testing.T) { require.NoError(t, Notify(ic)) require.Equal(t, 1, len(ic.Notifications)) + arr.MarkAsReadOnly() // tiny hack for test to be able to compare object references. ev := ic.Notifications[0] require.Equal(t, "good event", ev.Name) require.Equal(t, h, ev.ScriptHash) diff --git a/pkg/core/interop/runtime/util.go b/pkg/core/interop/runtime/util.go index e00345f87..2e13ced6d 100644 --- a/pkg/core/interop/runtime/util.go +++ b/pkg/core/interop/runtime/util.go @@ -53,7 +53,7 @@ func GetNotifications(ic *interop.Context) error { ev := stackitem.NewArray([]stackitem.Item{ stackitem.NewByteArray(notifications[i].ScriptHash.BytesBE()), stackitem.Make(notifications[i].Name), - stackitem.DeepCopy(notifications[i].Item).(*stackitem.Array), + stackitem.DeepCopy(notifications[i].Item, false).(*stackitem.Array), }) arr.Append(ev) } diff --git a/pkg/core/interop/runtime/util_test.go b/pkg/core/interop/runtime/util_test.go index 88b1fbdd2..7e6519d51 100644 --- a/pkg/core/interop/runtime/util_test.go +++ b/pkg/core/interop/runtime/util_test.go @@ -54,6 +54,7 @@ func TestRuntimeGetNotifications(t *testing.T) { name, err := stackitem.ToString(elem[1]) require.NoError(t, err) require.Equal(t, ic.Notifications[i].Name, name) + ic.Notifications[i].Item.MarkAsReadOnly() // tiny hack for test to be able to compare object references. require.Equal(t, ic.Notifications[i].Item, elem[2]) } }) diff --git a/pkg/core/interop_system_core_test.go b/pkg/core/interop_system_core_test.go index 2c7b34307..a086f04c1 100644 --- a/pkg/core/interop_system_core_test.go +++ b/pkg/core/interop_system_core_test.go @@ -109,6 +109,7 @@ func TestRuntimeGetNotifications(t *testing.T) { name, err := stackitem.ToString(elem[1]) require.NoError(t, err) require.Equal(t, ic.Notifications[i].Name, name) + ic.Notifications[i].Item.MarkAsReadOnly() // tiny hack for test to be able to compare object references. require.Equal(t, ic.Notifications[i].Item, elem[2]) } }) diff --git a/pkg/vm/opcodebench_test.go b/pkg/vm/opcodebench_test.go index 0c68ae647..34942397a 100644 --- a/pkg/vm/opcodebench_test.go +++ b/pkg/vm/opcodebench_test.go @@ -65,7 +65,7 @@ func opParamSlotsPushVM(op opcode.Opcode, param []byte, sslot int, slotloc int, for i := range items { item, ok := items[i].(stackitem.Item) if ok { - item = stackitem.DeepCopy(item) + item = stackitem.DeepCopy(item, false) } else { item = stackitem.Make(items[i]) } diff --git a/pkg/vm/stackitem/immutable.go b/pkg/vm/stackitem/immutable.go new file mode 100644 index 000000000..98bc9688a --- /dev/null +++ b/pkg/vm/stackitem/immutable.go @@ -0,0 +1,21 @@ +package stackitem + +type ro struct { + isReadOnly bool +} + +// IsReadOnly implements Immutable interface. +func (r *ro) IsReadOnly() bool { + return r.isReadOnly +} + +// MarkAsReadOnly implements immutable interface. +func (r *ro) MarkAsReadOnly() { + r.isReadOnly = true +} + +// Immutable is an interface supported by compound types (Array, Map, Struct). +type Immutable interface { + IsReadOnly() bool + MarkAsReadOnly() +} diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index 18afb7349..d7cf0f0ed 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -70,6 +70,8 @@ var ( // can also be returned by serialization functions if the resulting // value exceeds MaxSize. ErrTooBig = errors.New("too big") + // ErrReadOnly is returned on attempt to modify immutable stack item. + ErrReadOnly = errors.New("item is read-only") errTooBigComparable = fmt.Errorf("%w: uncomparable", ErrTooBig) errTooBigInteger = fmt.Errorf("%w: integer", ErrTooBig) @@ -185,6 +187,7 @@ func convertPrimitive(item Item, typ Type) (Item, error) { type Struct struct { value []Item rc + ro } // NewStruct returns a new Struct object. @@ -202,16 +205,25 @@ func (i *Struct) Value() interface{} { // Remove removes the element at `pos` index from the Struct value. // It will panic if a bad index given. func (i *Struct) Remove(pos int) { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = append(i.value[:pos], i.value[pos+1:]...) } // Append adds an Item to the end of the Struct value. func (i *Struct) Append(item Item) { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = append(i.value, item) } // Clear removes all elements from the Struct item value. func (i *Struct) Clear() { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = i.value[:0] } @@ -662,6 +674,7 @@ func (i *ByteArray) Convert(typ Type) (Item, error) { type Array struct { value []Item rc + ro } // NewArray returns a new Array object. @@ -679,16 +692,25 @@ func (i *Array) Value() interface{} { // Remove removes the element at `pos` index from Array value. // It will panics on bad index. func (i *Array) Remove(pos int) { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = append(i.value[:pos], i.value[pos+1:]...) } // Append adds an Item to the end of the Array value. func (i *Array) Append(item Item) { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = append(i.value, item) } // Clear removes all elements from the Array item value. func (i *Array) Clear() { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = i.value[:0] } @@ -763,6 +785,7 @@ type MapElement struct { type Map struct { value []MapElement rc + ro } // NewMap returns a new Map object. @@ -789,6 +812,9 @@ func (i *Map) Value() interface{} { // Clear removes all elements from the Map item value. func (i *Map) Clear() { + if i.IsReadOnly() { + panic(ErrReadOnly) + } i.value = i.value[:0] } @@ -860,6 +886,9 @@ func (i *Map) Add(key, value Item) { if err := IsValidMapKey(key); err != nil { panic(err) } + if i.IsReadOnly() { + panic(ErrReadOnly) + } index := i.Index(key) if index >= 0 { i.value[index].Value = value @@ -870,6 +899,9 @@ func (i *Map) Add(key, value Item) { // Drop removes the given index from the map (no bounds check done here). func (i *Map) Drop(index int) { + if i.IsReadOnly() { + panic(ErrReadOnly) + } copy(i.value[index:], i.value[index+1:]) i.value = i.value[:len(i.value)-1] } @@ -1139,12 +1171,12 @@ func (i *Buffer) Len() int { // DeepCopy returns a new deep copy of the provided item. // Values of Interop items are not deeply copied. // It does preserve duplicates only for non-primitive types. -func DeepCopy(item Item) Item { +func DeepCopy(item Item, asImmutable bool) Item { seen := make(map[Item]Item, typicalNumOfItems) - return deepCopy(item, seen) + return deepCopy(item, seen, asImmutable) } -func deepCopy(item Item, seen map[Item]Item) Item { +func deepCopy(item Item, seen map[Item]Item, asImmutable bool) Item { if it := seen[item]; it != nil { return it } @@ -1155,24 +1187,28 @@ func deepCopy(item Item, seen map[Item]Item) Item { arr := NewArray(make([]Item, len(it.value))) seen[item] = arr for i := range it.value { - arr.value[i] = deepCopy(it.value[i], seen) + arr.value[i] = deepCopy(it.value[i], seen, asImmutable) } + arr.MarkAsReadOnly() return arr case *Struct: arr := NewStruct(make([]Item, len(it.value))) seen[item] = arr for i := range it.value { - arr.value[i] = deepCopy(it.value[i], seen) + arr.value[i] = deepCopy(it.value[i], seen, asImmutable) } + arr.MarkAsReadOnly() return arr case *Map: m := NewMap() seen[item] = m for i := range it.value { - key := deepCopy(it.value[i].Key, seen) - value := deepCopy(it.value[i].Value, seen) + key := deepCopy(it.value[i].Key, seen, + false) // Key is always primitive and not a Buffer. + value := deepCopy(it.value[i].Value, seen, asImmutable) m.Add(key, value) } + m.MarkAsReadOnly() return m case *BigInteger: bi := new(big.Int).Set(it.Big()) @@ -1180,6 +1216,9 @@ func deepCopy(item Item, seen map[Item]Item) Item { case *ByteArray: return NewByteArray(slice.Copy(*it)) case *Buffer: + if asImmutable { + return NewByteArray(slice.Copy(*it)) // TODO: ported as is from C#, but is this correct? + } return NewBuffer(slice.Copy(*it)) case Bool: return it diff --git a/pkg/vm/stackitem/item_test.go b/pkg/vm/stackitem/item_test.go index 72920c8e2..13d3fcd6c 100644 --- a/pkg/vm/stackitem/item_test.go +++ b/pkg/vm/stackitem/item_test.go @@ -530,7 +530,10 @@ func TestDeepCopy(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - actual := DeepCopy(tc.item) + actual := DeepCopy(tc.item, false) + if immut, ok := tc.item.(Immutable); ok { + immut.MarkAsReadOnly() // tiny hack for test to be able to compare object references. + } require.Equal(t, tc.item, actual) if tc.item.Type() != BooleanT { require.False(t, actual == tc.item) @@ -539,7 +542,7 @@ func TestDeepCopy(t *testing.T) { } t.Run("Null", func(t *testing.T) { - require.Equal(t, Null{}, DeepCopy(Null{})) + require.Equal(t, Null{}, DeepCopy(Null{}, false)) }) t.Run("Array", func(t *testing.T) { @@ -547,7 +550,8 @@ func TestDeepCopy(t *testing.T) { arr.value[0] = NewBool(true) arr.value[1] = arr - actual := DeepCopy(arr) + actual := DeepCopy(arr, false) + arr.isReadOnly = true // tiny hack for test to be able to compare object references. require.Equal(t, arr, actual) require.False(t, arr == actual) require.True(t, actual == actual.(*Array).value[1]) @@ -558,7 +562,8 @@ func TestDeepCopy(t *testing.T) { arr.value[0] = NewBool(true) arr.value[1] = arr - actual := DeepCopy(arr) + actual := DeepCopy(arr, false) + arr.isReadOnly = true // tiny hack for test to be able to compare object references. require.Equal(t, arr, actual) require.False(t, arr == actual) require.True(t, actual == actual.(*Struct).value[1]) @@ -569,7 +574,8 @@ func TestDeepCopy(t *testing.T) { m.value[0] = MapElement{Key: NewBool(true), Value: m} m.value[1] = MapElement{Key: NewBigInteger(big.NewInt(1)), Value: NewByteArray([]byte{1, 2, 3})} - actual := DeepCopy(m) + actual := DeepCopy(m, false) + m.isReadOnly = true // tiny hack for test to be able to compare object references. require.Equal(t, m, actual) require.False(t, m == actual) require.True(t, actual == actual.(*Map).value[0].Value) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index bf3358145..7df97ae97 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1245,10 +1245,16 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro v.throw(stackitem.NewByteArray([]byte(msg))) return } + if t.(stackitem.Immutable).IsReadOnly() { + panic(stackitem.ErrReadOnly) + } v.refs.Remove(arr[index]) arr[index] = item v.refs.Add(arr[index]) case *stackitem.Map: + if t.IsReadOnly() { + panic(stackitem.ErrReadOnly) + } if i := t.Index(key.value); i >= 0 { v.refs.Remove(t.Value().([]stackitem.MapElement)[i].Value) } else { @@ -1279,6 +1285,9 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro item := v.estack.Pop() switch t := item.value.(type) { case *stackitem.Array, *stackitem.Struct: + if t.(stackitem.Immutable).IsReadOnly() { + panic(stackitem.ErrReadOnly) + } a := t.Value().([]stackitem.Item) for i, j := 0, len(a)-1; i < j; i, j = i+1, j-1 { a[i], a[j] = a[j], a[i] @@ -1332,16 +1341,25 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro elem := v.estack.Pop() switch t := elem.value.(type) { case *stackitem.Array: + if t.IsReadOnly() { + panic(stackitem.ErrReadOnly) + } for _, item := range t.Value().([]stackitem.Item) { v.refs.Remove(item) } t.Clear() case *stackitem.Struct: + if t.IsReadOnly() { + panic(stackitem.ErrReadOnly) + } for _, item := range t.Value().([]stackitem.Item) { v.refs.Remove(item) } t.Clear() case *stackitem.Map: + if t.IsReadOnly() { + panic(stackitem.ErrReadOnly) + } elems := t.Value().([]stackitem.MapElement) for i := range elems { v.refs.Remove(elems[i].Key) From 42a051e55ac196c90352c8adc608bba22b4c30f7 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 30 May 2022 11:01:12 +0300 Subject: [PATCH 3/3] core: DeepCopy notifiction event args inside System.Runtime.Notify --- cli/nep11_test.go | 2 +- pkg/core/interop/runtime/engine.go | 2 +- pkg/core/interop/runtime/util.go | 2 +- pkg/vm/opcodebench_test.go | 2 +- pkg/vm/stackitem/item.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/nep11_test.go b/cli/nep11_test.go index 17383af23..536c0929d 100644 --- a/cli/nep11_test.go +++ b/cli/nep11_test.go @@ -319,7 +319,7 @@ func TestNEP11_ND_OwnerOf_BalanceOf_Transfer(t *testing.T) { ScriptHash: verifyH, Name: "OnNEP11Payment", Item: stackitem.NewArray([]stackitem.Item{ - stackitem.NewBuffer(nftOwnerHash.BytesBE()), + stackitem.NewByteArray(nftOwnerHash.BytesBE()), stackitem.NewBigInteger(big.NewInt(1)), stackitem.NewByteArray(tokenID1), stackitem.NewByteArray([]byte("some_data")), diff --git a/pkg/core/interop/runtime/engine.go b/pkg/core/interop/runtime/engine.go index c5ff4a79b..d4920f584 100644 --- a/pkg/core/interop/runtime/engine.go +++ b/pkg/core/interop/runtime/engine.go @@ -68,7 +68,7 @@ func Notify(ic *interop.Context) error { if len(bytes) > MaxNotificationSize { return fmt.Errorf("notification size shouldn't exceed %d", MaxNotificationSize) } - ic.AddNotification(ic.VM.GetCurrentScriptHash(), name, stackitem.DeepCopy(stackitem.NewArray(args), false).(*stackitem.Array)) + ic.AddNotification(ic.VM.GetCurrentScriptHash(), name, stackitem.DeepCopy(stackitem.NewArray(args), true).(*stackitem.Array)) return nil } diff --git a/pkg/core/interop/runtime/util.go b/pkg/core/interop/runtime/util.go index 2e13ced6d..947b4b153 100644 --- a/pkg/core/interop/runtime/util.go +++ b/pkg/core/interop/runtime/util.go @@ -53,7 +53,7 @@ func GetNotifications(ic *interop.Context) error { ev := stackitem.NewArray([]stackitem.Item{ stackitem.NewByteArray(notifications[i].ScriptHash.BytesBE()), stackitem.Make(notifications[i].Name), - stackitem.DeepCopy(notifications[i].Item, false).(*stackitem.Array), + notifications[i].Item, }) arr.Append(ev) } diff --git a/pkg/vm/opcodebench_test.go b/pkg/vm/opcodebench_test.go index 34942397a..3ff86c09a 100644 --- a/pkg/vm/opcodebench_test.go +++ b/pkg/vm/opcodebench_test.go @@ -65,7 +65,7 @@ func opParamSlotsPushVM(op opcode.Opcode, param []byte, sslot int, slotloc int, for i := range items { item, ok := items[i].(stackitem.Item) if ok { - item = stackitem.DeepCopy(item, false) + item = stackitem.DeepCopy(item, true) } else { item = stackitem.Make(items[i]) } diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index d7cf0f0ed..4e3e0ae98 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -1217,7 +1217,7 @@ func deepCopy(item Item, seen map[Item]Item, asImmutable bool) Item { return NewByteArray(slice.Copy(*it)) case *Buffer: if asImmutable { - return NewByteArray(slice.Copy(*it)) // TODO: ported as is from C#, but is this correct? + return NewByteArray(slice.Copy(*it)) } return NewBuffer(slice.Copy(*it)) case Bool: