Skip to content

Commit

Permalink
fix(gnovm): count slice allocations before performing memory allocati…
Browse files Browse the repository at this point in the history
…ons (#2781)

<!-- please provide a detailed description of the changes made in this
pull request. -->
This is to fix the first issue mentioned in #2738.

In short, when allocating and reallocating slices' underlying arrays,
the VM was building the `TypedValue` slice before making the necessary
VM allocations. It is important the VM allocations be done before the
`TypedValue` allocations to ensure the values being allocated won't
exceed the VM's limit. In extreme cases, unchecked allocations resulted
in the VM hanging as it tried to allocate massive `TypedValue` slices in
the go runtime.
<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [x] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
  • Loading branch information
deelawn authored Sep 12, 2024
1 parent 41e0085 commit d7407f5
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 55 deletions.
16 changes: 16 additions & 0 deletions gno.land/cmd/gnoland/testdata/alloc_array.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
loadpkg gno.land/r/alloc $WORK

gnoland start

! gnokey maketx call -pkgpath gno.land/r/alloc -func DoAlloc -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stderr 'Data: allocation limit exceeded'

-- alloc.gno --
package alloc

var buffer interface{}

func DoAlloc() {
var arr [1_000_000_000_000_000]byte
buffer = arr
}
16 changes: 16 additions & 0 deletions gno.land/cmd/gnoland/testdata/alloc_byte_slice.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
loadpkg gno.land/r/alloc $WORK

gnoland start

! gnokey maketx call -pkgpath gno.land/r/alloc -func DoAlloc -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stderr 'Data: allocation limit exceeded'

-- alloc.gno --
package alloc

var buffer []byte

func DoAlloc() {
buffer := make([]byte, 1_000_000_000_000)
buffer[1] = 'a'
}
16 changes: 16 additions & 0 deletions gno.land/cmd/gnoland/testdata/alloc_slice.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
loadpkg gno.land/r/alloc $WORK

gnoland start

! gnokey maketx call -pkgpath gno.land/r/alloc -func DoAlloc -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stderr 'Data: allocation limit exceeded'

-- alloc.gno --
package alloc

var buffer []int

func DoAlloc() {
buffer := make([]int, 1_000_000_000_000)
buffer[1] = 1
}
11 changes: 9 additions & 2 deletions gnovm/pkg/gnolang/alloc.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,12 @@ func (alloc *Allocator) NewSlice(base Value, offset, length, maxcap int) *SliceV
}
}

// NOTE: also allocates the underlying array from list.
// NewSliceFromList allocates a new slice with the underlying array value
// populated from `list`. This should not be called from areas in the codebase
// that are doing allocations with potentially large user provided values, e.g.
// `make()` and `append()`. Using `Alloc.NewListArray` can be used is most cases
// to allocate the space for the `TypedValue` list before doing the allocation
// in the go runtime -- see the `make()` code in uverse.go.
func (alloc *Allocator) NewSliceFromList(list []TypedValue) *SliceValue {
alloc.AllocateSlice()
alloc.AllocateListArray(int64(cap(list)))
Expand All @@ -238,7 +243,9 @@ func (alloc *Allocator) NewSliceFromList(list []TypedValue) *SliceValue {
}
}

// NOTE: also allocates the underlying array from data.
// NewSliceFromData allocates a new slice with the underlying data array
// value populated from `data`. See the doc for `NewSliceFromList` for
// correct usage notes.
func (alloc *Allocator) NewSliceFromData(data []byte) *SliceValue {
alloc.AllocateSlice()
alloc.AllocateDataArray(int64(cap(data)))
Expand Down
116 changes: 63 additions & 53 deletions gnovm/pkg/gnolang/uverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,32 +193,32 @@ func UverseNode() *PackageNode {
return
} else if arg0Type.Elem().Kind() == Uint8Kind {
// append(nil, *SliceValue) new data bytes ---
data := make([]byte, arg1Length)
arrayValue := m.Alloc.NewDataArray(arg1Length)
if arg1Base.Data == nil {
copyListToData(
data[:arg1Length],
arrayValue.Data[:arg1Length],
arg1Base.List[arg1Offset:arg1EndIndex])
} else {
copy(
data[:arg1Length],
arrayValue.Data[:arg1Length],
arg1Base.Data[arg1Offset:arg1EndIndex])
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, arg1Length, arg1Length),
})
return
} else {
// append(nil, *SliceValue) new list ---------
list := make([]TypedValue, arg1Length)
if 0 < arg1Length {
arrayValue := m.Alloc.NewListArray(arg1Length)
if arg1Length > 0 {
for i := 0; i < arg1Length; i++ {
list[i] = arg1Base.List[arg1Offset+i].unrefCopy(m.Alloc, m.Store)
arrayValue.List[i] = arg1Base.List[arg1Offset+i].unrefCopy(m.Alloc, m.Store)
}
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, arg1Length, arg1Length),
})
return
}
Expand All @@ -236,27 +236,27 @@ func UverseNode() *PackageNode {
return
} else if arg0Type.Elem().Kind() == Uint8Kind {
// append(nil, *NativeValue) new data bytes --
data := make([]byte, arg1NativeValueLength)
arrayValue := m.Alloc.NewDataArray(arg1NativeValueLength)
copyNativeToData(
data[:arg1NativeValueLength],
arrayValue.Data[:arg1NativeValueLength],
arg1NativeValue, arg1NativeValueLength)
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, arg1NativeValueLength, arg1NativeValueLength),
})
return
} else {
// append(nil, *NativeValue) new list --------
list := make([]TypedValue, arg1NativeValueLength)
if 0 < arg1NativeValueLength {
arrayValue := m.Alloc.NewListArray(arg1NativeValueLength)
if arg1NativeValueLength > 0 {
copyNativeToList(
m.Alloc,
list[:arg1NativeValueLength],
arrayValue.List[:arg1NativeValueLength],
arg1NativeValue, arg1NativeValueLength)
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, arg1NativeValueLength, arg1NativeValueLength),
})
return
}
Expand Down Expand Up @@ -344,63 +344,65 @@ func UverseNode() *PackageNode {
}
} else if arg0Type.Elem().Kind() == Uint8Kind {
// append(*SliceValue, *SliceValue) new data bytes ---
data := make([]byte, arg0Length+arg1Length)
newLength := arg0Length + arg1Length
arrayValue := m.Alloc.NewDataArray(newLength)
if 0 < arg0Length {
if arg0Base.Data == nil {
copyListToData(
data[:arg0Length],
arrayValue.Data[:arg0Length],
arg0Base.List[arg0Offset:arg0Offset+arg0Length])
} else {
copy(
data[:arg0Length],
arrayValue.Data[:arg0Length],
arg0Base.Data[arg0Offset:arg0Offset+arg0Length])
}
}
if 0 < arg1Length {
if arg1Base.Data == nil {
copyListToData(
data[arg0Length:arg0Length+arg1Length],
arrayValue.Data[arg0Length:newLength],
arg1Base.List[arg1Offset:arg1Offset+arg1Length])
} else {
copy(
data[arg0Length:arg0Length+arg1Length],
arrayValue.Data[arg0Length:newLength],
arg1Base.Data[arg1Offset:arg1Offset+arg1Length])
}
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, newLength, newLength),
})
return
} else {
// append(*SliceValue, *SliceValue) new list ---------
list := make([]TypedValue, arg0Length+arg1Length)
if 0 < arg0Length {
arrayLen := arg0Length + arg1Length
arrayValue := m.Alloc.NewListArray(arrayLen)
if arg0Length > 0 {
if arg0Base.Data == nil {
for i := 0; i < arg0Length; i++ {
list[i] = arg0Base.List[arg0Offset+i].unrefCopy(m.Alloc, m.Store)
arrayValue.List[i] = arg0Base.List[arg0Offset+i].unrefCopy(m.Alloc, m.Store)
}
} else {
panic("should not happen")
}
}

if 0 < arg1Length {
if arg1Length > 0 {
if arg1Base.Data == nil {
for i := 0; i < arg1Length; i++ {
list[arg0Length+i] = arg1Base.List[arg1Offset+i].unrefCopy(m.Alloc, m.Store)
arrayValue.List[arg0Length+i] = arg1Base.List[arg1Offset+i].unrefCopy(m.Alloc, m.Store)
}
} else {
copyDataToList(
list[arg0Length:arg0Length+arg1Length],
arrayValue.List[arg0Length:arg0Length+arg1Length],
arg1Base.Data[arg1Offset:arg1Offset+arg1Length],
arg1Type.Elem(),
)
}
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, arrayLen, arrayLen),
})
return
}
Expand Down Expand Up @@ -441,46 +443,47 @@ func UverseNode() *PackageNode {
}
} else if arg0Type.Elem().Kind() == Uint8Kind {
// append(*SliceValue, *NativeValue) new data bytes --
data := make([]byte, arg0Length+arg1NativeValueLength)
newLength := arg0Length + arg1NativeValueLength
arrayValue := m.Alloc.NewDataArray(newLength)
if 0 < arg0Length {
if arg0Base.Data == nil {
copyListToData(
data[:arg0Length],
arrayValue.Data[:arg0Length],
arg0Base.List[arg0Offset:arg0Offset+arg0Length])
} else {
copy(
data[:arg0Length],
arrayValue.Data[:arg0Length],
arg0Base.Data[arg0Offset:arg0Offset+arg0Length])
}
}
if 0 < arg1NativeValueLength {
copyNativeToData(
data[arg0Length:arg0Length+arg1NativeValueLength],
arrayValue.Data[arg0Length:newLength],
arg1NativeValue, arg1NativeValueLength)
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, newLength, newLength),
})
return
} else {
// append(*SliceValue, *NativeValue) new list --------
listLen := arg0Length + arg1NativeValueLength
list := make([]TypedValue, listLen)
arrayValue := m.Alloc.NewListArray(listLen)
if 0 < arg0Length {
for i := 0; i < listLen; i++ {
list[i] = arg0Base.List[arg0Offset+i].unrefCopy(m.Alloc, m.Store)
arrayValue.List[i] = arg0Base.List[arg0Offset+i].unrefCopy(m.Alloc, m.Store)
}
}
if 0 < arg1NativeValueLength {
copyNativeToList(
m.Alloc,
list[arg0Length:listLen],
arrayValue.List[arg0Length:listLen],
arg1NativeValue, arg1NativeValueLength)
}
m.PushValue(TypedValue{
T: arg0Type,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, listLen, listLen),
})
return
}
Expand Down Expand Up @@ -779,25 +782,25 @@ func UverseNode() *PackageNode {
lv := vargs.TV.GetPointerAtIndexInt(m.Store, 0).Deref()
li := lv.ConvertGetInt()
if et.Kind() == Uint8Kind {
data := make([]byte, li)
arrayValue := m.Alloc.NewDataArray(li)
m.PushValue(TypedValue{
T: tt,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, li, li),
})
return
} else {
list := make([]TypedValue, li)
arrayValue := m.Alloc.NewListArray(li)
if et.Kind() == InterfaceKind {
// leave as is
} else {
// init zero elements with concrete type.
for i := 0; i < li; i++ {
list[i] = defaultTypedValue(m.Alloc, et)
arrayValue.List[i] = defaultTypedValue(m.Alloc, et)
}
}
m.PushValue(TypedValue{
T: tt,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, li, li),
})
return
}
Expand All @@ -807,30 +810,37 @@ func UverseNode() *PackageNode {
cv := vargs.TV.GetPointerAtIndexInt(m.Store, 1).Deref()
ci := cv.ConvertGetInt()
if et.Kind() == Uint8Kind {
data := make([]byte, li, ci)
arrayValue := m.Alloc.NewDataArray(ci)
m.PushValue(TypedValue{
T: tt,
V: m.Alloc.NewSliceFromData(data),
V: m.Alloc.NewSlice(arrayValue, 0, li, ci),
})
return
} else {
list := make([]TypedValue, li, ci)
arrayValue := m.Alloc.NewListArray(ci)
if et := bt.Elem(); et.Kind() == InterfaceKind {
// leave as is
} else {
// init zero elements with concrete type.
// the elements beyond len l within cap c
// must also be initialized, for a future
// slice operation may refer to them.
// XXX can this be removed?
list2 := list[:ci]
// Initialize all elements within capacity with default
// type values. These need to be initialized because future
// slice operations could get messy otherwise. Simple capacity
// expansions like `a = a[:cap(a)]` would make it trivial to
// initialize zero values at the time of the slice operation.
// But sequences of operations like:
// a := make([]int, 1, 10)
// a = a[7:cap(a)]
// a = a[3:5]
//
// require a bit more work to handle correctly, requiring that
// all new TypedValue slice elements be checked to ensure they have
// a value for every slice operation, which is not desirable.
for i := 0; i < ci; i++ {
list2[i] = defaultTypedValue(m.Alloc, et)
arrayValue.List[i] = defaultTypedValue(m.Alloc, et)
}
}
m.PushValue(TypedValue{
T: tt,
V: m.Alloc.NewSliceFromList(list),
V: m.Alloc.NewSlice(arrayValue, 0, li, ci),
})
return
}
Expand Down

0 comments on commit d7407f5

Please sign in to comment.