Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vm: fix packmap operation #3685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+26 −27
Open

vm: fix packmap operation #3685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions pkg/rpcclient/nep11/base_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,6 @@ func TestUnwrapKnownProperties(t *testing.T) {
require.NotNil(t, m)
require.Equal(t, 0, len(m))

m, err = UnwrapKnownProperties(stackitem.NewMapWithValue([]stackitem.MapElement{
{Key: stackitem.Make([]stackitem.Item{}), Value: stackitem.Make("thing")},
}), nil)
require.NoError(t, err)
require.NotNil(t, m)
require.Equal(t, 0, len(m))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless change.

_, err = UnwrapKnownProperties(stackitem.NewMapWithValue([]stackitem.MapElement{
{Key: stackitem.Make("name"), Value: stackitem.Make([]stackitem.Item{})},
}), nil)
Expand Down
10 changes: 6 additions & 4 deletions pkg/vm/stackitem/item.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,12 +826,14 @@

// NewMapWithValue returns a new Map object filled with the specified value.
func NewMapWithValue(value []MapElement) *Map {
if value != nil {
return &Map{
value: value,
res := NewMap()
for i := range value {
if err := IsValidMapKey(value[i].Key); err != nil {
panic(err)

Check warning on line 832 in pkg/vm/stackitem/item.go

View check run for this annotation

Codecov / codecov/patch

pkg/vm/stackitem/item.go#L832 

Added line #L832 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too risky to panic inside NewMapWithValue, it may be used outside the VM. Either adjust a documentation of this function and don't check for value correctness or change the signature of this function to return error. BTW, with the updated PACKMAP, do we really use this function anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, NewMapWithValue is used only in tests.

}
res.Add(value[i].Key, value[i].Value)
}
return NewMap()
return res
}

// Value implements the Item interface.
Expand Down
21 changes: 12 additions & 9 deletions pkg/vm/stackitem/item_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,14 +591,17 @@ func TestDeepCopy(t *testing.T) {
})

t.Run("Map", func(t *testing.T) {
m := NewMapWithValue(make([]MapElement, 2))
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, 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)
m := make([]MapElement, 2)
expexted := NewMap()
expexted.Add(NewBool(false), NewBool(false))
expexted.MarkAsReadOnly()
m[0] = MapElement{Key: NewBool(true), Value: expexted}
m[1] = MapElement{Key: NewBigInteger(big.NewInt(1)), Value: NewByteArray([]byte{1, 2, 3})}
res := NewMapWithValue(m)
res.MarkAsReadOnly()
actual := DeepCopy(res, false)
require.Equal(t, res, actual)
require.False(t, res == actual)
require.Equal(t, expexted, actual.(*Map).value[0].Value)
})
}
4 changes: 3 additions & 1 deletion pkg/vm/stackitem/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,10 @@ func TestToJSONWithTypesBadCases(t *testing.T) {
t.Run("overflow on map key", func(t *testing.T) {
m := NewMapWithValue([]MapElement{
{NewBool(true), NewBool(true)},
{NewByteArray(bigBuf), NewBool(true)},
})
for i := 1; i < MaxSize/85; i++ {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe MaxSerialized better

m.Add(NewBigInteger(big.NewInt(int64(i))), NewBigInteger(big.NewInt(123456)))
}
_, err := ToJSONWithTypes(m)
require.ErrorIs(t, err, errTooBigSize)
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/vm/stackitem/serialization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func TestSerializeLimited(t *testing.T) {
bigMap := make([]MapElement, customLimit/2)
for i := range bigMap {
bigMap[i] = MapElement{
Key: NewByteArray([]byte("key")),
Key: NewByteArray([]byte("key" + strconv.Itoa(i))),
Value: NewBool(true),
}
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/vm/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,15 +1214,14 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro
panic("invalid length")
}

items := make([]stackitem.MapElement, n)
for i := range n {
newMap := stackitem.NewMap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newMap

m

for range n {
key := v.estack.Pop()
validateMapKey(key)
val := v.estack.Pop().value
items[i].Key = key.value
items[i].Value = val
newMap.Add(key.Item(), val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way is not optimal, verification is performed twice.

}
v.estack.PushItem(stackitem.NewMapWithValue(items))
v.estack.PushItem(newMap)

case opcode.PACKSTRUCT, opcode.PACK:
n := toInt(v.estack.Pop().BigInt())
Expand Down
Loading