From b2609786e9e9074174e82dabba354393d6551026 Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Fri, 18 Oct 2019 13:01:51 +0300 Subject: [PATCH] vm: copy slice in NEWARRAY/NEWSTRUCT When performing NEWARRAY on a Struct or NEWSTRUCT on a Array, underlying slice needs to be copied, because when it's capacity doesn't matches it's length, underlying storage will be used for appends even if it is already pointed at by another slice. --- pkg/vm/vm.go | 8 ++++-- pkg/vm/vm_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 8671d5acb..ae839260e 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -756,7 +756,9 @@ func (v *VM) execute(ctx *Context, op Instruction, parameter []byte) { item := v.estack.Pop() switch t := item.value.(type) { case *StructItem: - v.estack.PushVal(&ArrayItem{t.value}) + arr := make([]StackItem, len(t.value)) + copy(arr, t.value) + v.estack.PushVal(&ArrayItem{arr}) case *ArrayItem: v.estack.PushVal(t) default: @@ -772,7 +774,9 @@ func (v *VM) execute(ctx *Context, op Instruction, parameter []byte) { item := v.estack.Pop() switch t := item.value.(type) { case *ArrayItem: - v.estack.PushVal(&StructItem{t.value}) + arr := make([]StackItem, len(t.value)) + copy(arr, t.value) + v.estack.PushVal(&StructItem{arr}) case *StructItem: v.estack.PushVal(t) default: diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index 7b870a733..fe08e2edf 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -528,6 +528,39 @@ func TestNEWARRAYStruct(t *testing.T) { assert.Equal(t, &ArrayItem{arr}, vm.estack.Pop().value) } +func testNEWARRAYIssue437(t *testing.T, i1, i2 Instruction, appended bool) { + prog := makeProgram( + PUSH2, i1, + DUP, PUSH3, APPEND, + TOALTSTACK, DUPFROMALTSTACK, i2, + DUP, PUSH4, APPEND, + FROMALTSTACK, PUSH5, APPEND) + vm := load(prog) + vm.Run() + + arr := makeArrayOfFalses(4) + arr[2] = makeStackItem(3) + arr[3] = makeStackItem(4) + if appended { + arr = append(arr, makeStackItem(5)) + } + + assert.Equal(t, false, vm.HasFailed()) + assert.Equal(t, 1, vm.estack.Len()) + if i2 == NEWARRAY { + assert.Equal(t, &ArrayItem{arr}, vm.estack.Pop().value) + } else { + assert.Equal(t, &StructItem{arr}, vm.estack.Pop().value) + } +} + +func TestNEWARRAYIssue437(t *testing.T) { + t.Run("Array+Array", func(t *testing.T) { testNEWARRAYIssue437(t, NEWARRAY, NEWARRAY, true) }) + t.Run("Struct+Struct", func(t *testing.T) { testNEWARRAYIssue437(t, NEWSTRUCT, NEWSTRUCT, true) }) + t.Run("Array+Struct", func(t *testing.T) { testNEWARRAYIssue437(t, NEWARRAY, NEWSTRUCT, false) }) + t.Run("Struct+Array", func(t *testing.T) { testNEWARRAYIssue437(t, NEWSTRUCT, NEWARRAY, false) }) +} + func TestNEWARRAYArray(t *testing.T) { prog := makeProgram(NEWARRAY) vm := load(prog) @@ -1732,6 +1765,39 @@ func TestREVERSEBadNotArray(t *testing.T) { assert.Equal(t, true, vm.HasFailed()) } +func testREVERSEIssue437(t *testing.T, i1, i2 Instruction, reversed bool) { + prog := makeProgram( + PUSH0, i1, + DUP, PUSH1, APPEND, + DUP, PUSH2, APPEND, + DUP, i2, REVERSE) + vm := load(prog) + vm.Run() + + arr := make([]StackItem, 2) + if reversed { + arr[0] = makeStackItem(2) + arr[1] = makeStackItem(1) + } else { + arr[0] = makeStackItem(1) + arr[1] = makeStackItem(2) + } + assert.Equal(t, false, vm.HasFailed()) + assert.Equal(t, 1, vm.estack.Len()) + if i1 == NEWARRAY { + assert.Equal(t, &ArrayItem{arr}, vm.estack.Pop().value) + } else { + assert.Equal(t, &StructItem{arr}, vm.estack.Pop().value) + } +} + +func TestREVERSEIssue437(t *testing.T) { + t.Run("Array+Array", func(t *testing.T) { testREVERSEIssue437(t, NEWARRAY, NEWARRAY, true) }) + t.Run("Struct+Struct", func(t *testing.T) { testREVERSEIssue437(t, NEWSTRUCT, NEWSTRUCT, true) }) + t.Run("Array+Struct", func(t *testing.T) { testREVERSEIssue437(t, NEWARRAY, NEWSTRUCT, false) }) + t.Run("Struct+Array", func(t *testing.T) { testREVERSEIssue437(t, NEWSTRUCT, NEWARRAY, false) }) +} + func TestREVERSEGoodOneElem(t *testing.T) { prog := makeProgram(DUP, REVERSE) elements := []int{22}