Skip to content

Commit

Permalink
fix: properly mark array elements when an realm slice is updated (#1305)
Browse files Browse the repository at this point in the history
Addresses #1167,  #960, and #1170 

Consider the following situation:
- A slice of structs exists with a length of zero and a capacity of one
- A new struct literal is appended to the slice
- The code panics because the newly allocated struct literal was never
marked as "new"

``` go
package append

import (
	"gno.land/p/demo/ufmt"
)

type T struct{ i int }

var a []T

func init() {
        a = make([]T, 0, 1)
}

func Append(i int) {
	a = append(a, T{i: i})
}

```

Invoking the `Append` function will cause a panic.

The solution is to traverse each of the array elements after slice
append assignment to make sure any new or updated elements are marked as
such.

This PR also includes a change to ensure that marking an object as dirty
and managing references to the object are mutually exclusive. I think
this is correct but am not sure.

The changes include txtar test cases that incorporate the issue
described by @tbruyelle in #1170

---------

Co-authored-by: jaekwon <[email protected]>
  • Loading branch information
deelawn and jaekwon authored Jan 4, 2024
1 parent b1b514c commit b525e8b
Show file tree
Hide file tree
Showing 6 changed files with 328 additions and 16 deletions.
129 changes: 129 additions & 0 deletions gno.land/cmd/gnoland/testdata/append.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
# start a new node
gnoland start

gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/append -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# Call Append 1
gnokey maketx call -pkgpath gno.land/r/append -func Append -gas-fee 1000000ugnot -gas-wanted 2000000 -args '1' -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func AppendNil -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# Call Append 2
gnokey maketx call -pkgpath gno.land/r/append -func Append -gas-fee 1000000ugnot -gas-wanted 2000000 -args '2' -broadcast -chainid=tendermint_test test1
stdout OK!

# Call Append 3
gnokey maketx call -pkgpath gno.land/r/append -func Append -gas-fee 1000000ugnot -gas-wanted 2000000 -args '3' -broadcast -chainid=tendermint_test test1
stdout OK!

# Call render
gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
stdout '("1-2-3-" string)'
stdout OK!

# Call Pop
gnokey maketx call -pkgpath gno.land/r/append -func Pop -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# Call render
gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
stdout '("2-3-" string)'
stdout OK!

# Call Append 42
gnokey maketx call -pkgpath gno.land/r/append -func Append -gas-fee 1000000ugnot -gas-wanted 2000000 -args '42' -broadcast -chainid=tendermint_test test1
stdout OK!

# Call render
gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
stdout '("2-3-42-" string)'
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func CopyAppend -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func PopB -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# Call render
gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
stdout '("2-3-42-" string)'
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func AppendMoreAndC -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func ReassignC -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args '' -broadcast -chainid=tendermint_test test1
stdout '("2-3-42-70-100-" string)'
stdout OK!

gnokey maketx call -pkgpath gno.land/r/append -func Render -gas-fee 1000000ugnot -gas-wanted 2000000 -args 'd' -broadcast -chainid=tendermint_test test1
stdout '("1-" string)'
stdout OK!

-- append.gno --
package append

import (
"gno.land/p/demo/ufmt"
)

type T struct{ i int }

var a, b, d []T
var c = []T{{i: 100}}


func init() {
a = make([]T, 0, 1)
}

func Pop() {
a = append(a[:0], a[1:]...)
}

func Append(i int) {
a = append(a, T{i: i})
}

func CopyAppend() {
b = append(a, T{i: 50}, T{i: 60})
}

func PopB() {
b = append(b[:0], b[1:]...)
}

func AppendMoreAndC() {
// Fill to capacity
a = append(a, T{i: 70})
// Above capacity; make new array
a = append(a, c...)
}

func ReassignC() {
c[0] = T{i: 200}
}

func AppendNil() {
d = append(d, a...)
}

func Render(path string) string {
source := a
if path == "d" {
source = d
}

var s string
for i:=0;i<len(source);i++{
s+=ufmt.Sprintf("%d-", source[i].i)
}
return s
}
114 changes: 114 additions & 0 deletions gno.land/cmd/gnoland/testdata/issue-1167.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# Reproducible Test for https://github.com/gnolang/gno/issues/1167

gnoland start

# add contract
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/xx -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# execute New
gnokey maketx call -pkgpath gno.land/r/demo/xx -func New -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# execute Delta for the first time
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '"1,1,1;" string'

# execute Delta for the second time
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;" string'

# execute Delta for the third time
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;3,3,3;" string'

# execute Render
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Render -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;3,3,3;" string'

-- gno.mod --
module gno.land/r/demo/xx

require (
gno.land/p/demo/avl v0.0.0-latest
)

-- realm.gno --
package xx

import (
"strconv"

"gno.land/p/demo/avl"
)

type Move struct {
N1, N2, N3 byte
}

type Position struct {
Moves []Move
}

func (p Position) clone() Position {
mv := p.Moves
return Position{Moves: mv}
}

func (oldp Position) update() Position {
p := oldp.clone()

counter++
// This is a workaround for the wrong behaviour (ie. uncomment this line):
// p.Moves = append([]Move{}, p.Moves...)
p.Moves = append(p.Moves, Move{counter, counter, counter})
return p
}

type Game struct {
Position Position
}

var games avl.Tree // id -> *Game

var counter byte

func New(s string) string {
// Bug shows if Moves has a cap > 0 when initialised.
el := &Game{Position: Position{Moves: make([]Move, 0, 2)}}
games.Set(s, el)
return values(el.Position)
}

func Delta(s string) string {
v, _ := games.Get(s)
g, ok := v.(*Game)
if !ok {
panic("invalid game")
}
n := g.Position.update()
g.Position = n
ret := values(n)
return ret
}

func Render(s string) string {
v, _ := games.Get(s)
g, ok := v.(*Game)
if !ok {
panic("invalid game")
}
return values(g.Position)
}

func values(x Position) string {
s := ""
for _, val := range x.Moves {
s += strconv.Itoa(int(val.N1)) + "," + strconv.Itoa(int(val.N2)) + "," + strconv.Itoa(int(val.N3)) + ";"
}
return s
}
2 changes: 2 additions & 0 deletions gnovm/pkg/gnolang/realm.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ func (rlm *Realm) DidUpdate(po, xo, co Object) {
// Updates to .newCreated/.newEscaped /.newDeleted made here. (first gen)
// More appends happen during FinalizeRealmTransactions(). (second+ gen)
rlm.MarkDirty(po)

if co != nil {
co.IncRefCount()
if co.GetRefCount() > 1 {
Expand All @@ -166,6 +167,7 @@ func (rlm *Realm) DidUpdate(po, xo, co Object) {
rlm.MarkNewReal(co)
}
}

if xo != nil {
xo.DecRefCount()
if xo.GetRefCount() == 0 {
Expand Down
45 changes: 29 additions & 16 deletions gnovm/pkg/gnolang/uverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,9 @@ func UverseNode() *PackageNode {
// append(nil, *SliceValue) new list ---------
list := make([]TypedValue, argsl)
if 0 < argsl {
copy(
list[:argsl],
argsb.List[argso:argso+argsl])
for i := 0; i < argsl; i++ {
list[i] = argsb.List[argso+i].unrefCopy(m.Alloc, m.Store)
}
}
m.PushValue(TypedValue{
T: xt,
Expand Down Expand Up @@ -294,14 +294,25 @@ func UverseNode() *PackageNode {
// append(*SliceValue.List, *SliceValue) ---------
list := xvb.List
if argsb.Data == nil {
copy(
list[xvo+xvl:xvo+xvl+argsl],
argsb.List[argso:argso+argsl])
for i := 0; i < argsl; i++ {
oldElem := list[xvo+xvl+i]
// unrefCopy will resolve references and copy their values
// to copy by value rather than by reference.
newElem := argsb.List[argso+i].unrefCopy(m.Alloc, m.Store)
list[xvo+xvl+i] = newElem

m.Realm.DidUpdate(
xvb,
oldElem.GetFirstObject(m.Store),
newElem.GetFirstObject(m.Store),
)
}
} else {
copyDataToList(
list[xvo+xvl:xvo+xvl+argsl],
argsb.Data[argso:argso+argsl],
xt.Elem())
m.Realm.DidUpdate(xvb, nil, nil)
}
} else {
// append(*SliceValue.Data, *SliceValue) ---------
Expand All @@ -310,6 +321,7 @@ func UverseNode() *PackageNode {
copyListToData(
data[xvo+xvl:xvo+xvl+argsl],
argsb.List[argso:argso+argsl])
m.Realm.DidUpdate(xvb, nil, nil)
} else {
copy(
data[xvo+xvl:xvo+xvl+argsl],
Expand Down Expand Up @@ -363,9 +375,9 @@ func UverseNode() *PackageNode {
list := make([]TypedValue, xvl+argsl)
if 0 < xvl {
if xvb.Data == nil {
copy(
list[:xvl],
xvb.List[xvo:xvo+xvl])
for i := 0; i < xvl; i++ {
list[i] = xvb.List[xvo+i].unrefCopy(m.Alloc, m.Store)
}
} else {
panic("should not happen")
/*
Expand All @@ -379,9 +391,9 @@ func UverseNode() *PackageNode {
}
if 0 < argsl {
if argsb.Data == nil {
copy(
list[xvl:xvl+argsl],
argsb.List[argso:argso+argsl])
for i := 0; i < argsl; i++ {
list[xvl+i] = argsb.List[argso+i].unrefCopy(m.Alloc, m.Store)
}
} else {
copyDataToList(
list[xvl:xvl+argsl],
Expand Down Expand Up @@ -457,11 +469,12 @@ func UverseNode() *PackageNode {
return
} else {
// append(*SliceValue, *NativeValue) new list --------
list := make([]TypedValue, xvl+argsl)
listLen := xvl + argsl
list := make([]TypedValue, listLen)
if 0 < xvl {
copy(
list[:xvl],
xvb.List[xvo:xvo+xvl])
for i := 0; i < listLen; i++ {
list[i] = xvb.List[xvo+i].unrefCopy(m.Alloc, m.Store)
}
}
if 0 < argsl {
copyNativeToList(
Expand Down
22 changes: 22 additions & 0 deletions gnovm/pkg/gnolang/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,28 @@ func (tv TypedValue) Copy(alloc *Allocator) (cp TypedValue) {
return
}

// unrefCopy makes a copy of the underlying value in the case of reference values.
// It copies other values as expected using the normal Copy method.
func (tv TypedValue) unrefCopy(alloc *Allocator, store Store) (cp TypedValue) {
switch tv.V.(type) {
case RefValue:
cp.T = tv.T
refObject := tv.GetFirstObject(store)
switch refObjectValue := refObject.(type) {
case *ArrayValue:
cp.V = refObjectValue.Copy(alloc)
case *StructValue:
cp.V = refObjectValue.Copy(alloc)
default:
cp = tv
}
default:
cp = tv.Copy(alloc)
}

return
}

// Returns encoded bytes for primitive values.
// These bytes are used for both value hashes as well
// as hash key bytes.
Expand Down
Loading

0 comments on commit b525e8b

Please sign in to comment.