fix(gnovm): change initialization order so realm objects are persisted before referenced by other realms

docs/concepts/effective-gno.md

@@ -596,6 +596,149 @@ func init() {
 // the object.
 ```
 
+### Be careful sharing pointers with other realms
+
+It is possible and, in some cases, advantageous to declare global realm variables that are pointers.
+However, be careful; there may be unintended consequences when passing these global pointers as arguments
+to other realms' functions. If the other realm persists this pointer into its own realm state, the 
+underlying value being pointed to will continue to exist even if the original realm no longer maintains
+any references to it. If the object is large, this means that the owner's realm will never be able to 
+reclaim this storage, something that may have serious cost implications if gno.land moves to a rent-based
+realm storage plan in the future.
+
+This is an example of a pointer that is safe to pass to another realm:
+```go
+package obj_owner
+import (
+    "gno.land/p/large"
+    "gno.land/r/obj_ref"
+)
+
+var bigObj large.Object
+
+func init() {
+    bigObj.FillwithLotsOfData()
+}
+
+// This is safe because the object being referenced will always exist in this realm.
+func ShareReference() {
+    obj_ref.SetLargeObjRef(&bigObj)
+}
+
+------------------------------------------
+package obj_ref
+
+import "gno.land/p/large"
+
+var sharedObj *large.Object
+
+func SetLargeObjRef(ref *large.Object) {
+    sharedObj = ref
+}
+```
+
+This is an unsafe example that may result in the referenced large object being doomed to exist forever.
+```go
+package obj_owner
+import (
+    "gno.land/p/large"
+    "gno.land/r/obj_ref"
+)
+
+var bigObjs []large.Object
+
+func init() {
+    ResetBigObjs()
+}
+
+func ResetBigObjs() {
+    bigObjs = make([]bigObj, 3)
+    for i := 1; i < len(bigObj); i++ {
+        bigObjs[i].FillwithLotsOfData()
+    }
+}
+
+// This is unsafe because the object being referenced will not be referenced from this realm anymore
+// after the next call to ResetBigObjs.
+func ShareReference() {
+    if len(bigObjs) == 0 {
+        return
+    }
+
+    obj_ref.SetLargeObjRef(&bigObjs[len(bigObjs)-1])
+}
+
+------------------------------------------
+package obj_ref
+
+import "gno.land/p/large"
+
+var sharedObj *large.Object
+
+func SetLargeObjRef(ref *large.Object) {
+    sharedObj = ref
+}
+```
+
+#### Sharing pointers to non-global realm variables
+Before passing a pointer as an argument to another realm, it is important to always check how that realm will
+use that pointer. Be wary of realms that persist pointer values to their own global realm state. If you pass
+a pointer to a variable that is not part of your own global realm state, and other realm tries to persist it,
+that variable may be persisted there so any further attempts to modify it in the realm in which it originated will fail.
+For this reason, passing and persisting pointers between realms is generally discouraged unless you have a good
+understanding of how the VM handles these types of operations.
+
+Example:
+```go
+package steal_ownership
+
+var Ptr *uint32
+
+func Steal(xptr *uint32) {
+	Ptr = xptr
+}
+
+--------------------------------------------
+package mis_ownership
+
+import "gno.land/r/steal_ownership"
+
+var y *uint32
+
+func SharePtr1() {
+	x := uint32(42)
+	y = &x
+
+	// This is not okay because the x value is owned by this realm by way of y, and is
+	// currently planned to be persisted.
+	steal_ownership.Steal(&x)
+}
+
+
+func SharePtr2() {
+	x := uint32(42)
+	steal_ownership.Steal(&x)
+
+	// This is okay because the x value was already persisted and is now owned by the
+	// steal realm because it was unowned with no plan to persist it at the time.
+	// However, it is not owned by this realm now so attempting to modify the value by
+    // calling Update() will fail.
+	y = &x
+}
+
+func SharePtr3() {
+	x := uint32(42)
+
+	// This is okay because the x value is unowned and will not be persisted by this realm.
+	steal_ownership.Steal(&x)
+}
+
+func Update() {
+	*y = 43
+}
+
+```
+
 ### Choosing between Coins and GRC20 tokens
 
 In Gno, you've got two primary options: Coins or GRC20. Each option

gno.land/cmd/gnoland/dylan/ptr-to-unpersisted.txtar

@@ -0,0 +1,81 @@
+# This test is meant to exhibit how pointers to unpersisted values cannot be
+# persisted as realm variables in other realms.
+
+loadpkg gno.land/r/steal_ownership $WORK/steal
+loadpkg gno.land/r/mis_ownership $WORK/mis
+
+gnoland start
+
+! gnokey maketx call -pkgpath gno.land/r/mis_ownership -func SharePtr1 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stderr 'cannot modify external-realm or non-realm object'
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func SharePtr2 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+! gnokey maketx call -pkgpath gno.land/r/mis_ownership -func Update -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stderr 'cannot modify external-realm or non-realm object'
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func SharePtr3 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/steal_ownership -func Update -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/steal_ownership -func PtrValue -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout '(43 uint32)'
+stdout OK!
+
+-- steal/steal.gno --
+package steal_ownership
+
+var Ptr *uint32
+
+func Steal(xptr *uint32) {
+	Ptr = xptr
+}
+
+func Update() {
+	*Ptr = 43
+}
+
+func PtrValue() uint32 {
+	return *Ptr
+}
+
+-- mis/mis.gno --
+package mis_ownership
+
+import "gno.land/r/steal_ownership"
+
+var y *uint32
+
+func SharePtr1() {
+	x := uint32(42)
+	y = &x
+
+	// This is not okay because the x value is owned by this realm by way of y, and is
+	// currently planned to be persisted.
+	steal_ownership.Steal(&x)
+}
+
+
+func SharePtr2() {
+	x := uint32(42)
+	steal_ownership.Steal(&x)
+
+	// This is okay because the x value was already persisted and is now owned by the
+	// steal realm because it was unowned with no plan to persist it at the time.
+	// However, it is not owned by this realm now so any attempt to modify it will fail.
+	y = &x
+}
+
+func SharePtr3() {
+	x := uint32(42)
+
+	// This is okay because the x value is unowned and will not be persisted by this realm.
+	steal_ownership.Steal(&x)
+}
+
+func Update() {
+	*y = 43
+}

gno.land/cmd/gnoland/testdata/issue-974.txtar

@@ -0,0 +1,75 @@
+# Reproducible Test for https://github.com/gnolang/gno/issues/974
+
+loadpkg gno.land/r/steal_ownership $WORK/steal
+loadpkg gno.land/r/mis_ownership $WORK/mis
+
+gnoland start
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func PtrValues -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout '(42 uint32)'
+stdout '(42 uint32)'
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func MutateDeref -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func PtrValues -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout '(21 uint32)'
+stdout '(21 uint32)'
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func MutateValue -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func PtrValues -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout '(15 uint32)'
+stdout '(15 uint32)'
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func MutatePtr -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout OK!
+
+gnokey maketx call -pkgpath gno.land/r/mis_ownership -func PtrValues -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
+stdout '(5 uint32)'
+stdout '(15 uint32)'
+stdout OK!
+
+-- steal/steal.gno --
+package steal_ownership
+
+var Ptr *uint32
+
+func Steal(xptr *uint32) {
+	Ptr = xptr
+}
+
+-- mis/mis.gno --
+package mis_ownership
+
+import "gno.land/r/steal_ownership"
+
+var (
+	x   = uint32(42)
+	ptr = &x
+)
+
+func init() {
+	steal_ownership.Steal(ptr)
+}
+
+func MutateDeref() {
+	*ptr = 21
+}
+
+func MutatePtr() {
+	y := uint32(5)
+	ptr = &y
+}
+
+func MutateValue() {
+	x = 15
+}
+
+func PtrValues() (uint32, uint32) {
+	return *ptr, *steal_ownership.Ptr
+}

gnovm/pkg/gnolang/machine.go

@@ -271,15 +271,14 @@ func (m *Machine) runMemPackage(memPkg *std.MemPackage, save, overrides bool) (*
 		m.Store.SetCachePackage(pv)
 	}
 	m.SetActivePackage(pv)
+
 	// run files.
-	m.RunFiles(files.Files...)
-	// maybe save package value and mempackage.
 	if save {
-		// store package values and types
-		m.savePackageValuesAndTypes()
-		// store mempackage
-		m.Store.AddMemPackage(memPkg)
+		m.RunFilesWithMemPkg(memPkg, files.Files...)
+	} else {
+		m.RunFiles(files.Files...)
 	}
+
 	return pn, pv
 }
 
@@ -481,10 +480,16 @@ func (m *Machine) injectLocOnPanic() {
 // Add files to the package's *FileSet and run them.
 // This will also run each init function encountered.
 func (m *Machine) RunFiles(fns ...*FileNode) {
-	m.runFiles(fns...)
+	m.runFiles(nil, fns...)
+}
+
+// RunFilesWithMemPkg is almost the same as RunFiles; the difference is that it
+// saves the mempackage to the store.
+func (m *Machine) RunFilesWithMemPkg(memPkg *std.MemPackage, fns ...*FileNode) {
+	m.runFiles(memPkg, fns...)
 }
 
-func (m *Machine) runFiles(fns ...*FileNode) {
+func (m *Machine) runFiles(memPkg *std.MemPackage, fns ...*FileNode) {
 	// Files' package names must match the machine's active one.
 	// if there is one.
 	for _, fn := range fns {
@@ -612,6 +617,16 @@ func (m *Machine) runFiles(fns ...*FileNode) {
 		}
 	}
 
+	// Save the realm mempackage if provided. We do this here so that it occurs before
+	// the init functions are run. This avoids any realm object ownership issues that may arise
+	// when passing around unpersisted realm object pointers to other realms during initialization.
+	if memPkg != nil && IsRealmPath(memPkg.Path) {
+		// store package values and types
+		m.savePackageValuesAndTypes()
+		// store mempackage
+		m.Store.AddMemPackage(memPkg)
+	}
+
 	// Run new init functions.
 	// Go spec: "To ensure reproducible initialization
 	// behavior, build systems are encouraged to present
@@ -631,6 +646,18 @@ func (m *Machine) runFiles(fns ...*FileNode) {
 			}
 		}
 	}
+
+	// If this is a package, we can save it after the init function because init is the
+	// only way stateful realm variables can be dynamically set in a package. We don't need to
+	// worry about pointer ownership issues for packages because they (eventually) will only be
+	// able to import other packages and those packages won't be able to persist any pointers
+	// to their own state that they receive from other packages.
+	if memPkg != nil && !IsRealmPath(memPkg.Path) {
+		// store package values and types
+		m.savePackageValuesAndTypes()
+		// store mempackage
+		m.Store.AddMemPackage(memPkg)
+	}
 }
 
 // Save the machine's package using realm finalization deep crawl.