From cfe41abd35dd804330a5a7d8b87ac9907ddbe352 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sun, 11 Jul 2021 18:47:50 +0300 Subject: [PATCH 1/3] vm: fix OOM during nested structure cloning Resulting item can't have more than MaxStackSize elements. Technically this limits to MaxStackSize cloned elements but it's considered to be enough to mitigate the issue (the next size check is going to happen during push to the stack). See neo-project/neo#2534, thanks @vang1ong7ang. --- pkg/vm/stackitem/item.go | 19 ++++++++++++++++--- pkg/vm/stackitem/item_test.go | 9 +++++++++ pkg/vm/vm.go | 22 ++++++++++++++++------ pkg/vm/vm_test.go | 18 ++++++++++++++++++ 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index db2c1cac4..c5d7007fe 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -298,17 +298,30 @@ func (i *Struct) Convert(typ Type) (Item, error) { // Clone returns a Struct with all Struct fields copied by value. // Array fields are still copied by reference. -func (i *Struct) Clone() *Struct { +func (i *Struct) Clone(limit int) (*Struct, error) { + return i.clone(&limit) +} + +func (i *Struct) clone(limit *int) (*Struct, error) { ret := &Struct{make([]Item, len(i.value))} for j := range i.value { switch t := i.value[j].(type) { case *Struct: - ret.value[j] = t.Clone() + var err error + + ret.value[j], err = t.clone(limit) + if err != nil { + return nil, err + } + *limit-- default: ret.value[j] = t } + if *limit < 0 { + return nil, ErrTooBig + } } - return ret + return ret, nil } // Null represents null on the stack. diff --git a/pkg/vm/stackitem/item_test.go b/pkg/vm/stackitem/item_test.go index ef6fc5762..1244074a0 100644 --- a/pkg/vm/stackitem/item_test.go +++ b/pkg/vm/stackitem/item_test.go @@ -465,6 +465,15 @@ func TestNewVeryBigInteger(t *testing.T) { check(false, new(big.Int).Mul(maxBitSet, big.NewInt(2))) } +func TestStructClone(t *testing.T) { + st0 := Struct{} + st := Struct{value: []Item{&st0}} + _, err := st.Clone(1) + require.NoError(t, err) + _, err = st.Clone(0) + require.Error(t, err) +} + func TestDeepCopy(t *testing.T) { testCases := []struct { name string diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 12014aeaa..ad2fe2d1f 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1056,7 +1056,10 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro itemElem := v.estack.Pop() arrElem := v.estack.Pop() - val := cloneIfStruct(itemElem.value) + val, err := cloneIfStruct(itemElem.value) + if err != nil { + panic(err) + } switch t := arrElem.value.(type) { case *stackitem.Array: @@ -1370,12 +1373,19 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro src := t.Value().([]stackitem.Item) arr = make([]stackitem.Item, len(src)) for i := range src { - arr[i] = cloneIfStruct(src[i]) + arr[i], err = cloneIfStruct(src[i]) + if err != nil { + panic(err) + } } case *stackitem.Map: arr = make([]stackitem.Item, 0, t.Len()) for k := range t.Value().([]stackitem.MapElement) { - arr = append(arr, cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value)) + elem, err := cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value) + if err != nil { + panic(err) + } + arr = append(arr, elem) } default: panic("not a Map, Array or Struct") @@ -1741,12 +1751,12 @@ func checkMultisig1(v *VM, curve elliptic.Curve, h []byte, pkeys [][]byte, sig [ return false } -func cloneIfStruct(item stackitem.Item) stackitem.Item { +func cloneIfStruct(item stackitem.Item) (stackitem.Item, error) { switch it := item.(type) { case *stackitem.Struct: - return it.Clone() + return it.Clone(MaxStackSize) default: - return it + return it, nil } } diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index 91870e236..8a176a88f 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -3,6 +3,7 @@ package vm import ( "bytes" "encoding/binary" + "encoding/hex" "errors" "fmt" "math" @@ -2431,6 +2432,23 @@ func TestSLOTOpcodes(t *testing.T) { }) } +func TestNestedStructClone(t *testing.T) { + progs := []string{ + // VALUES for deeply nested structs, see neo-project/neo#2534. + "5601c501fe0360589d604a12c0db415824f7cd45", + // APPEND of deeply nested struct to empty array. + "5601c2c501fe0360589d604a12c0db415824f7cf45", + // VALUES for map with deeply nested struct. + "5601c84a11c501fe0060589d604a12c0db415824f7d0cd45", + } + for _, h := range progs { + prog, err := hex.DecodeString(h) + require.NoError(t, err) + vm := load(prog) + checkVMFailed(t, vm) + } +} + func makeProgram(opcodes ...opcode.Opcode) []byte { prog := make([]byte, len(opcodes)+1) // RET for i := 0; i < len(opcodes); i++ { From 4000dd692c161cabba132cc52795045334fd8476 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Sun, 11 Jul 2021 19:37:06 +0300 Subject: [PATCH 2/3] vm: make cloning limits a bit more effective Take actual reference counter value into account. --- pkg/vm/vm.go | 10 +++++----- pkg/vm/vm_test.go | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index ad2fe2d1f..c91d5c881 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1056,7 +1056,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro itemElem := v.estack.Pop() arrElem := v.estack.Pop() - val, err := cloneIfStruct(itemElem.value) + val, err := cloneIfStruct(itemElem.value, MaxStackSize-v.refs.size) if err != nil { panic(err) } @@ -1373,7 +1373,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro src := t.Value().([]stackitem.Item) arr = make([]stackitem.Item, len(src)) for i := range src { - arr[i], err = cloneIfStruct(src[i]) + arr[i], err = cloneIfStruct(src[i], MaxStackSize-v.refs.size) if err != nil { panic(err) } @@ -1381,7 +1381,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case *stackitem.Map: arr = make([]stackitem.Item, 0, t.Len()) for k := range t.Value().([]stackitem.MapElement) { - elem, err := cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value) + elem, err := cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value, MaxStackSize-v.refs.size) if err != nil { panic(err) } @@ -1751,10 +1751,10 @@ func checkMultisig1(v *VM, curve elliptic.Curve, h []byte, pkeys [][]byte, sig [ return false } -func cloneIfStruct(item stackitem.Item) (stackitem.Item, error) { +func cloneIfStruct(item stackitem.Item, limit int) (stackitem.Item, error) { switch it := item.(type) { case *stackitem.Struct: - return it.Clone(MaxStackSize) + return it.Clone(limit) default: return it, nil } diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index 8a176a88f..484b3cda1 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -2440,6 +2440,8 @@ func TestNestedStructClone(t *testing.T) { "5601c2c501fe0360589d604a12c0db415824f7cf45", // VALUES for map with deeply nested struct. "5601c84a11c501fe0060589d604a12c0db415824f7d0cd45", + // VALUES for a lot of not-so-deep nested structs. + "5601c5000a60589d604a12c0db415824f701fe03504a519d4a102afa01ff03c0cd45", } for _, h := range progs { prog, err := hex.DecodeString(h) From fe3c68b92d86a9fc957518c273e33bb547aada3e Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 12 Jul 2021 11:51:49 +0300 Subject: [PATCH 3/3] vm: panic in cloneIfStruct It's internal, so we can deduplicate code a bit. --- pkg/vm/vm.go | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index c91d5c881..7ae6d5cdf 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1056,10 +1056,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro itemElem := v.estack.Pop() arrElem := v.estack.Pop() - val, err := cloneIfStruct(itemElem.value, MaxStackSize-v.refs.size) - if err != nil { - panic(err) - } + val := cloneIfStruct(itemElem.value, MaxStackSize-v.refs.size) switch t := arrElem.value.(type) { case *stackitem.Array: @@ -1373,19 +1370,12 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro src := t.Value().([]stackitem.Item) arr = make([]stackitem.Item, len(src)) for i := range src { - arr[i], err = cloneIfStruct(src[i], MaxStackSize-v.refs.size) - if err != nil { - panic(err) - } + arr[i] = cloneIfStruct(src[i], MaxStackSize-v.refs.size) } case *stackitem.Map: arr = make([]stackitem.Item, 0, t.Len()) for k := range t.Value().([]stackitem.MapElement) { - elem, err := cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value, MaxStackSize-v.refs.size) - if err != nil { - panic(err) - } - arr = append(arr, elem) + arr = append(arr, cloneIfStruct(t.Value().([]stackitem.MapElement)[k].Value, MaxStackSize-v.refs.size)) } default: panic("not a Map, Array or Struct") @@ -1751,12 +1741,16 @@ func checkMultisig1(v *VM, curve elliptic.Curve, h []byte, pkeys [][]byte, sig [ return false } -func cloneIfStruct(item stackitem.Item, limit int) (stackitem.Item, error) { +func cloneIfStruct(item stackitem.Item, limit int) stackitem.Item { switch it := item.(type) { case *stackitem.Struct: - return it.Clone(limit) + ret, err := it.Clone(limit) + if err != nil { + panic(err) + } + return ret default: - return it, nil + return it } }