From 74097ae8b0ce3bc12aff301988951fa5d4b50d76 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sat, 22 Aug 2020 22:19:44 +0300 Subject: [PATCH 1/5] stackitem: add NewPointerWithHash() to save on hash calculations Inspired by neo-project/neo-vm#352. We can't directly compare slices, so we're better optimize things we already have. At the same time this code would behave a bit different if A is to call B and then B is call A and then some pointer from the first A invocation is to be compared with a pointer from the second A invocation. Not sure it really matters. --- pkg/vm/stackitem/item.go | 14 +++++++++++++- pkg/vm/vm.go | 2 +- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index c20701b50..a737dff29 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -871,6 +871,18 @@ func NewPointer(pos int, script []byte) *Pointer { } } +// NewPointerWithHash returns new pointer on the specified position of the +// specified script. It differs from NewPointer in that the script hash is being +// passed explicitly to save on hash calculcation. This hash is then being used +// for pointer comparisons. +func NewPointerWithHash(pos int, script []byte, h util.Uint160) *Pointer { + return &Pointer{ + pos: pos, + script: script, + hash: h, + } +} + // String implements Item interface. func (p *Pointer) String() string { return "Pointer" @@ -1079,7 +1091,7 @@ func deepCopy(item Item, seen map[Item]Item) Item { case *Bool: return NewBool(it.value) case *Pointer: - return NewPointer(it.pos, it.script) + return NewPointerWithHash(it.pos, it.script, it.hash) case *Interop: return NewInterop(it.value) default: diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index af9cfff6e..92965c80e 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -535,7 +535,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case opcode.PUSHA: n := v.getJumpOffset(ctx, parameter) - ptr := stackitem.NewPointer(n, ctx.prog) + ptr := stackitem.NewPointerWithHash(n, ctx.prog, ctx.ScriptHash()) v.estack.PushVal(ptr) case opcode.PUSHNULL: From 77ea3d361bf6d7922bc085ccc522d7a8463aba4d Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sat, 22 Aug 2020 22:36:10 +0300 Subject: [PATCH 2/5] vm: update neo-vm tests, simplify parsing We no longer have "*N" notation, see neo-project/neo-vm#326. --- pkg/vm/json_test.go | 20 +------------------- pkg/vm/testdata/neo-vm | 2 +- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/pkg/vm/json_test.go b/pkg/vm/json_test.go index 36fb7660a..d41b6cd96 100644 --- a/pkg/vm/json_test.go +++ b/pkg/vm/json_test.go @@ -11,8 +11,6 @@ import ( "math/big" "os" "path/filepath" - "regexp" - "strconv" "strings" "testing" @@ -330,23 +328,7 @@ func (v *vmUTScript) UnmarshalJSON(data []byte) error { if b, ok := decodeSingle(ops[i]); ok { script = append(script, b...) } else { - const regex = `(?P(?:0x)?[0-9a-zA-Z]+)\*(?P[0-9]+)` - re := regexp.MustCompile(regex) - ss := re.FindStringSubmatch(ops[i]) - if len(ss) != 3 { - return fmt.Errorf("invalid script part: %s", ops[i]) - } - b, ok := decodeSingle(ss[1]) - if !ok { - return fmt.Errorf("invalid script part: %s", ops[i]) - } - num, err := strconv.Atoi(ss[2]) - if err != nil { - return fmt.Errorf("invalid script part: %s", ops[i]) - } - for i := 0; i < num; i++ { - script = append(script, b...) - } + return fmt.Errorf("invalid script part: %s", ops[i]) } } diff --git a/pkg/vm/testdata/neo-vm b/pkg/vm/testdata/neo-vm index 377464ed4..8476d0abb 160000 --- a/pkg/vm/testdata/neo-vm +++ b/pkg/vm/testdata/neo-vm @@ -1 +1 @@ -Subproject commit 377464ed475a3de108e1bf9c834bd2279b72624e +Subproject commit 8476d0abba10b2efdc94e8d4dc27f5c30c8b66e1 From 324f4c265b11be1817cf4c7b7da46e3192a35697 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sat, 22 Aug 2020 23:36:38 +0300 Subject: [PATCH 3/5] stackitem: don't copy existing slices for TryBytes Most often we only need to read them and it doesn't require copying. Make an explicit copy (and copy only things we need!) where needed. After the recent neo-vm tests update our vm package testing time jumped to ~12s, with this change it's now more like ~8s. --- pkg/vm/stackitem/item.go | 16 ++++++++-------- pkg/vm/vm.go | 19 ++++++++++++++----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index a737dff29..d928c1a62 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -33,7 +33,8 @@ type Item interface { Dup() Item // TryBool converts Item to a boolean value. TryBool() (bool, error) - // TryBytes converts Item to a byte slice. + // TryBytes converts Item to a byte slice. If the underlying type is a + // byte slice, it's returned as is without copying. TryBytes() ([]byte, error) // TryInteger converts Item to an integer. TryInteger() (*big.Int, error) @@ -151,8 +152,11 @@ func convertPrimitive(item Item, typ Type) (Item, error) { return nil, err } if typ == BufferT { - return NewBuffer(b), nil + newb := make([]byte, len(b)) + copy(newb, b) + return NewBuffer(newb), nil } + // ByteArray can't really be changed, so it's OK to reuse `b`. return NewByteArray(b), nil case BooleanT: b, err := item.TryBool() @@ -519,9 +523,7 @@ func (i *ByteArray) TryBool() (bool, error) { // TryBytes implements Item interface. func (i *ByteArray) TryBytes() ([]byte, error) { - val := make([]byte, len(i.value)) - copy(val, i.value) - return val, nil + return i.value, nil } // TryInteger implements Item interface. @@ -982,9 +984,7 @@ func (i *Buffer) TryBool() (bool, error) { // TryBytes implements Item interface. func (i *Buffer) TryBytes() ([]byte, error) { - val := make([]byte, len(i.value)) - copy(val, i.value) - return val, nil + return i.value, nil } // TryInteger implements Item interface. diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 92965c80e..cfeeeb235 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -663,10 +663,13 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case opcode.CAT: b := v.estack.Pop().Bytes() a := v.estack.Pop().Bytes() - if l := len(a) + len(b); l > stackitem.MaxSize { + l := len(a) + len(b) + if l > stackitem.MaxSize { panic(fmt.Sprintf("too big item: %d", l)) } - ab := append(a, b...) + ab := make([]byte, l) + copy(ab, a) + copy(ab[len(a):], b) v.estack.PushVal(stackitem.NewBuffer(ab)) case opcode.SUBSTR: @@ -683,7 +686,9 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro if last > len(s) { panic("invalid offset") } - v.estack.PushVal(stackitem.NewBuffer(s[o:last])) + res := make([]byte, l) + copy(res, s[o:last]) + v.estack.PushVal(stackitem.NewBuffer(res)) case opcode.LEFT: l := int(v.estack.Pop().BigInt().Int64()) @@ -694,7 +699,9 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro if t := len(s); l > t { panic("size is too big") } - v.estack.PushVal(stackitem.NewBuffer(s[:l])) + res := make([]byte, l) + copy(res, s[:l]) + v.estack.PushVal(stackitem.NewBuffer(res)) case opcode.RIGHT: l := int(v.estack.Pop().BigInt().Int64()) @@ -702,7 +709,9 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro panic("negative length") } s := v.estack.Pop().Bytes() - v.estack.PushVal(stackitem.NewBuffer(s[len(s)-l:])) + res := make([]byte, l) + copy(res, s[len(s)-l:]) + v.estack.PushVal(stackitem.NewBuffer(res)) case opcode.DEPTH: v.estack.PushVal(v.estack.Len()) From 32112249d55ac826f8403be651e102f76fc8f169 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sun, 23 Aug 2020 00:19:28 +0300 Subject: [PATCH 4/5] vm: limit maximum nesting of exception contexts Follow neo-project/neo#365. neo-vm submodule is updated just to show the relevant latest commit, nothing really changed there. --- pkg/vm/testdata/neo-vm | 2 +- pkg/vm/vm.go | 7 +++++++ pkg/vm/vm_test.go | 10 ++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/vm/testdata/neo-vm b/pkg/vm/testdata/neo-vm index 8476d0abb..e3f1584b1 160000 --- a/pkg/vm/testdata/neo-vm +++ b/pkg/vm/testdata/neo-vm @@ -1 +1 @@ -Subproject commit 8476d0abba10b2efdc94e8d4dc27f5c30c8b66e1 +Subproject commit e3f1584b1953dcc13075a57803240858c8a480de diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index cfeeeb235..284b05f3d 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -45,6 +45,10 @@ const ( // MaxInvocationStackSize is the maximum size of an invocation stack. MaxInvocationStackSize = 1024 + // MaxTryNestingDepth is the maximum level of TRY nesting allowed, + // that is you can't have more exception handling contexts than this. + MaxTryNestingDepth = 16 + // MaxStackSize is the maximum number of items allowed to be // on all stacks at once. MaxStackSize = 2 * 1024 @@ -1361,6 +1365,9 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case opcode.TRY, opcode.TRYL: catchP, finallyP := getTryParams(op, parameter) + if ctx.tryStack.Len() >= MaxTryNestingDepth { + panic("maximum TRY depth exceeded") + } cOffset := v.getJumpOffset(ctx, catchP) fOffset := v.getJumpOffset(ctx, finallyP) if cOffset == 0 && fOffset == 0 { diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index fbf354f05..f6403d09c 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -1321,6 +1321,16 @@ func TestTRY(t *testing.T) { inner := getTRYProgram(throw, add5, []byte{byte(opcode.THROW)}) getTRYTestFunc(32, inner, add5, add9)(t) }) + t.Run("TryMaxDepth", func(t *testing.T) { + loopTries := []byte{byte(opcode.INITSLOT), 0x01, 0x00, + byte(opcode.PUSH16), byte(opcode.INC), byte(opcode.STLOC0), + byte(opcode.TRY), 1, 1, // jump target + byte(opcode.LDLOC0), byte(opcode.DEC), byte(opcode.DUP), + byte(opcode.STLOC0), byte(opcode.PUSH0), + byte(opcode.JMPGT), 0xf8, byte(opcode.LDLOC0)} + vm := load(loopTries) + checkVMFailed(t, vm) + }) }) } From 681ae4d5d612982a1daea8dab49d84082a96e56c Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 24 Aug 2020 16:20:57 +0300 Subject: [PATCH 5/5] vm: fix TRY offsets check TRY can have an offset != 0 and still it can't have both parameters set to zero. --- pkg/vm/vm.go | 2 +- pkg/vm/vm_test.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 284b05f3d..6b838752f 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1370,7 +1370,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro } cOffset := v.getJumpOffset(ctx, catchP) fOffset := v.getJumpOffset(ctx, finallyP) - if cOffset == 0 && fOffset == 0 { + if cOffset == ctx.ip && fOffset == ctx.ip { panic("invalid offset for TRY*") } else if cOffset == ctx.ip { cOffset = -1 diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index f6403d09c..24e0a3150 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -1296,7 +1296,11 @@ func TestTRY(t *testing.T) { add5 := []byte{byte(opcode.PUSH5), byte(opcode.ADD)} add9 := []byte{byte(opcode.PUSH9), byte(opcode.ADD)} t.Run("NoCatch", func(t *testing.T) { - t.Run("NoFinally", getTRYTestFunc(nil, push1, nil, nil)) + t.Run("NoFinally", func(t *testing.T) { + prog := getTRYProgram(push1, nil, nil) + vm := load(prog) + checkVMFailed(t, vm) + }) t.Run("WithFinally", getTRYTestFunc(10, push1, nil, add9)) t.Run("Throw", getTRYTestFunc(nil, throw, nil, add9)) })