-
Notifications
You must be signed in to change notification settings - Fork 427
feat(gnovm): gnomod.toml add new field private
#4422
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.
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
base: master
Are you sure you want to change the base?
feat(gnovm): gnomod.toml add new field private
#4422
Conversation
Co-authored-by: Manfred Touron <[email protected]>
Co-authored-by: Manfred Touron <[email protected]>
Co-authored-by: Manfred Touron <[email protected]>
…o mikaelvallenet/gnovm/gnomod/add-new-draft-field
## Description Renaming to Gno.land, adding back in the gnoclient docs that were dropped with the Docs V2.
## Description Fixing the coin checker so it supports the newest gno-forms PR. The issue was that the form can only take you to a pre-determined render path within the realm, without the ability to parametrize it. I had to switch to parametrizing via query parameters. Would appreciate if someone can play around a bit.
## Description Fixes outdated link.
…o mikaelvallenet/gnovm/gnomod/add-new-draft-field
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial, initial review of the changes in gnolang
.
I don't think we should have private/public information in the ObjectID, it is quite unflexible.
@ltzmaxwell Do you agree it is better suited to be in the Realm
struct?
gnovm/pkg/gnolang/ownership.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced about storing private/public information in the ObjectID. While this does simplify checks when running FinalizeRealm
, it makes it very inflexible to ever switch from a private realm to a public realm.
I think the information on whether a package is private should be fetched either on the Realm, or if that is too slow, maybe in another database field keeping track of the attributes of a given package.
gnovm/pkg/gnolang/realm.go
Outdated
// assertNoPrivateDeps ensures that the object is not private | ||
// it does not recursively check the values, but it does check recursively the types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot just check the types if you have interfaces; interfaces may contain values from any package within....
@MikaelVallenet thank you for working on this! I have two questions here
I'm trying to understand the purpose and trade-offs behind these constraints. Any insights would be appreciated. |
Hi,
|
A proposal to improve complexity: Actually, we only ever persist:
saveUnsavedObjectRecursively already walks that exact set depth‑first:
Because of this recursion, every object that will be persisted is inevitably revisited as the “current oo” in its own recursive call. That’s the ideal moment to validate it. We don’t need I think an early deep scan over the whole graph or all nested types: the traversal guarantees coverage. If an object appears multiple times in the graph, flags (IsNewReal/IsDirty/IsEscaped) and the guards in the recursion prevent double persistence and thus avoid duplicate validation. Escaped/new‑escaped nodes are intentionally skipped from preemptive saving (and thus from validation here), since they’re not persisted via this path. Practically, this means:
Somethinh like this I think --- a/gnovm/pkg/gnolang/realm.go
+++ b/gnovm/pkg/gnolang/realm.go
@@ -801,9 +801,6 @@ func (rlm *Realm) saveUnsavedObjectRecursively(store Store, oo Object) {
}
}
- // assert object have no private dependencies.
- rlm.assertObjectIsPublic(oo, store)
-
// first, save unsaved children.
unsaved := getUnsavedChildObjects(oo)
for _, uch := range unsaved {
@@ -852,6 +849,19 @@ func (rlm *Realm) saveObject(store Store, oo Object) {
oo.SetIsEscaped(true)
// XXX anything else to do?
}
+
+ if fv, ok := oo.(*FuncValue); ok {
+ if fv.PkgPath != rlm.Path && IsPkgPrivateFromPkgPath(fv.PkgPath) {
+ panic("cannot persist function or method from private realm")
+ }
+ }
+
+ if oo.GetObjectID().PkgID != rlm.ID && IsPkgPrivateFromPkgID(oo.GetObjectID().PkgID) {
+ panic("cannot persist reference of object from private realm")
+ }
+
+ rlm.assertTypeCanBeOwned(oo)
+
// set object to store.
// NOTE: also sets the hash to object.
rlm.sumDiff += store.SetObject(oo)
@@ -904,143 +914,6 @@ func (rlm *Realm) clearMarks() {
rlm.escaped = nil
}
-// assertObjectIsPublic ensures that the object is public and does not have any private dependencies.
-// it check recursively the types of the object
-// it does not recursively check the values because
-// child objects are validated separately during the save traversal (saveUnsavedObjectRecursively)
-func (rlm *Realm) assertObjectIsPublic(obj Object, store Store) {
- objID := obj.GetObjectID()
- if objID.PkgID != rlm.ID && IsPkgPrivateFromPkgID(objID.PkgID) {
- panic("cannot persist reference of object from private realm")
- }
-
- // NOTE: should i set the visited tids map at the higher level so it's set one time.
- // it could help to reduce the number of checks for the same type.
- tids := make(map[TypeID]struct{})
- switch v := obj.(type) {
- case *HeapItemValue:
- if v.Value.T != nil {
- rlm.assertTypeIsPublic(store, v.Value.T, tids)
- }
- case *ArrayValue:
- for _, av := range v.List {
- if av.T != nil {
- rlm.assertTypeIsPublic(store, av.T, tids)
- }
- }
- case *StructValue:
- for _, sv := range v.Fields {
- if sv.T != nil {
- rlm.assertTypeIsPublic(store, sv.T, tids)
- }
- }
- case *MapValue:
- for head := v.List.Head; head != nil; head = head.Next {
- if head.Key.T != nil {
- rlm.assertTypeIsPublic(store, head.Key.T, tids)
- }
- if head.Value.T != nil {
- rlm.assertTypeIsPublic(store, head.Value.T, tids)
- }
- }
- case *FuncValue:
- if v.PkgPath != rlm.Path && IsPkgPrivateFromPkgPath(v.PkgPath) {
- panic("cannot persist function or method from private realm")
- }
- if v.Type != nil {
- rlm.assertTypeIsPublic(store, v.Type, tids)
- }
- for _, capture := range v.Captures {
- if capture.T != nil {
- rlm.assertTypeIsPublic(store, capture.T, tids)
- }
- }
- case *BoundMethodValue:
- if v.Func.PkgPath != rlm.Path && IsPkgPrivateFromPkgPath(v.Func.PkgPath) {
- panic("cannot persist bound method from private realm")
- }
- if v.Receiver.T != nil {
- rlm.assertTypeIsPublic(store, v.Receiver.T, tids)
- }
- case *Block:
- for _, bv := range v.Values {
- if bv.T != nil {
- rlm.assertTypeIsPublic(store, bv.T, tids)
- }
- }
- if v.Blank.T != nil {
- rlm.assertTypeIsPublic(store, v.Blank.T, tids)
- }
- case *PackageValue:
- if v.PkgPath != rlm.Path && IsPkgPrivateFromPkgPath(v.PkgPath) {
- panic("cannot persist package from private realm")
- }
- default:
- panic(fmt.Sprintf("assertNoPrivateDeps: unhandled object type %T", v))
- }
-}
-
-// assertTypeIsPublic ensure that the type t is not defined in a private realm.
-// it do it recursively for all types in t and have recursive guard to avoid infinite recursion on declared types.
-func (rlm *Realm) assertTypeIsPublic(store Store, t Type, visited map[TypeID]struct{}) {
- pkgPath := ""
- switch tt := t.(type) {
- case *FuncType:
- for _, param := range tt.Params {
- rlm.assertTypeIsPublic(store, param, visited)
- }
- for _, result := range tt.Results {
- rlm.assertTypeIsPublic(store, result, visited)
- }
- case FieldType:
- rlm.assertTypeIsPublic(store, tt.Type, visited)
- case *SliceType, *ArrayType, *ChanType, *PointerType:
- rlm.assertTypeIsPublic(store, tt.Elem(), visited)
- case *tupleType:
- for _, et := range tt.Elts {
- rlm.assertTypeIsPublic(store, et, visited)
- }
- case *MapType:
- rlm.assertTypeIsPublic(store, tt.Key, visited)
- rlm.assertTypeIsPublic(store, tt.Elem(), visited)
- case *InterfaceType:
- pkgPath = tt.GetPkgPath()
- for _, method := range tt.Methods {
- rlm.assertTypeIsPublic(store, method, visited)
- }
- case *StructType:
- pkgPath = tt.GetPkgPath()
- for _, field := range tt.Fields {
- rlm.assertTypeIsPublic(store, field, visited)
- }
- case *DeclaredType:
- tid := tt.TypeID()
- if _, exists := visited[tid]; !exists {
- visited[tid] = struct{}{}
- rlm.assertTypeIsPublic(store, tt.Base, visited)
- for _, method := range tt.Methods {
- rlm.assertTypeIsPublic(store, method.T, visited)
- if mv, ok := method.V.(*FuncValue); ok {
- rlm.assertTypeIsPublic(store, mv.Type, visited)
- }
- }
- }
- pkgPath = tt.GetPkgPath()
- case *RefType:
- t2 := store.GetType(tt.TypeID())
- rlm.assertTypeIsPublic(store, t2, visited)
- case PrimitiveType, *TypeType, *PackageType, blockType, heapItemType:
- // these types do not have a package path.
- // NOTE: PackageType have a TypeID, should i loat it from store and check it?
- return
- default:
- panic(fmt.Sprintf("assertTypeIsPublic: unhandled type %T", tt))
- }
- if pkgPath != "" && pkgPath != rlm.Path && IsPkgPrivateFromPkgPath(pkgPath) {
- panic("cannot persist object of type defined in a private realm")
- }
-}
-
//----------------------------------------
// getSelfOrChildObjects
@@ -1567,6 +1440,75 @@ func fillTypesTV(store Store, tv *TypedValue) {
tv.V = fillTypesOfValue(store, tv.V)
}
+func (rlm *Realm) assertTypeCanBeOwned(oo Object) {
+ switch cv := oo.(type) {
+ case nil:
+ case *ArrayValue:
+ for _, ctv := range cv.List {
+ rlm.assertTypeCanBeOwned2(ctv.T)
+ }
+ case *StructValue:
+ for _, ctv := range cv.Fields {
+ rlm.assertTypeCanBeOwned2(ctv.T)
+ }
+ case *FuncValue:
+ for _, c := range cv.Captures {
+ rlm.assertTypeCanBeOwned2(c.T)
+ }
+ case *BoundMethodValue:
+ rlm.assertTypeCanBeOwned2(cv.Func.Type)
+ rlm.assertTypeCanBeOwned2(cv.Receiver.T)
+ case *MapValue:
+ for cur := cv.List.Head; cur != nil; cur = cur.Next {
+ rlm.assertTypeCanBeOwned2(cur.Key.T)
+ rlm.assertTypeCanBeOwned2(cur.Value.T)
+ }
+ case *PackageValue:
+ case *Block:
+ case *HeapItemValue:
+ rlm.assertTypeCanBeOwned2(cv.Value.T)
+ default:
+ panic(fmt.Sprintf(
+ "unexpected type %v",
+ reflect.TypeOf(oo)))
+ }
+
+}
+
+func (rlm *Realm) assertTypeCanBeOwned2(ty Type) {
+ var pkgPaths []string
+ switch tt := ty.(type) {
+ case nil:
+ case *FuncType: // maybe impossible
+ case FieldType:
+ fmt.Printf("assertTypeCanBeOwned2: %T\n", ty)
+ case *SliceType, *ArrayType, *ChanType, *PointerType:
+ pkgPaths = append(pkgPaths, tt.Elem().GetPkgPath())
+ case *tupleType: // maybe impossible
+ case *MapType:
+ pkgPaths = append(pkgPaths, tt.Key.GetPkgPath())
+ pkgPaths = append(pkgPaths, tt.Elem().GetPkgPath())
+ case *InterfaceType: // maybe impossible
+ fmt.Printf("assertTypeCanBeOwned2: %T\n", ty)
+ pkgPaths = append(pkgPaths, tt.GetPkgPath())
+ case *StructType:
+ pkgPaths = append(pkgPaths, tt.GetPkgPath())
+ case *DeclaredType:
+ pkgPaths = append(pkgPaths, tt.GetPkgPath())
+ case *RefType:
+ pkgPaths = append(pkgPaths, tt.GetPkgPath())
+ case PrimitiveType, *TypeType, *PackageType, blockType, heapItemType:
+ default:
+ panic(fmt.Sprintf("assertTypeIsPublic: unhandled type %T", tt))
+ }
+
+ for _, pkgPath := range pkgPaths {
+ if pkgPath != "" && pkgPath != rlm.Path && IsPkgPrivateFromPkgPath(pkgPath) {
+ panic("cannot persist object of type defined in a private realm")
+ }
+ }
+}
+
// Partially fills loaded objects shallowly, similarly to
// getUnsavedTypes. Replaces all RefTypes with corresponding types.
func fillTypesOfValue(store Store, val Value) Value {
WDYT ? |
if you don't check type recursively you open vuln i think func SaveNilSliceOfSliceOfPrivateToPublicRealm(cur realm) {
// nil value with type [][]*PrivateData
var arr [][]*PrivateData = nil
publicrealm.SaveYourInterface(cross, arr)
} This succeed with your impl. when it should fails. |
I think this could be a solution, but I'm not entirely certain. My sample doesn't handle empty elements, which should be improved. To address your issue maybe this (we should handle map too): func (rlm *Realm) assertTypeCanBeOwned2(ty Type) {
var pkgPaths []string
fmt.Printf("assertTypeCanBeOwned2: %T %v\n", ty, ty)
switch tt := ty.(type) {
case nil:
case *FuncType: // maybe impossible
case FieldType:
case *SliceType, *ArrayType, *ChanType, *PointerType:
ty = tt.Elem()
Loop:
for { // When slice or array are empty we will not have any value in get child object so we should check all the declaration
switch tt := ty.(type) {
case *SliceType, *ArrayType, *ChanType, *PointerType:
ty = tt.Elem()
default:
pkgPaths = append(pkgPaths, ty.GetPkgPath())
break Loop
}
}
case *tupleType:
case *MapType: // maybe handle empty case
pkgPaths = append(pkgPaths, tt.Key.GetPkgPath())
pkgPaths = append(pkgPaths, tt.Elem().GetPkgPath())
case *InterfaceType:
pkgPaths = append(pkgPaths, tt.GetPkgPath())
case *StructType:
pkgPaths = append(pkgPaths, tt.GetPkgPath())
case *DeclaredType:
pkgPaths = append(pkgPaths, tt.GetPkgPath())
case *RefType:
pkgPaths = append(pkgPaths, tt.GetPkgPath())
case PrimitiveType, *TypeType, *PackageType, blockType, heapItemType:
default:
panic(fmt.Sprintf("assertTypeIsPublic: unhandled type %T", tt))
}
for _, pkgPath := range pkgPaths {
if pkgPath != "" && pkgPath != rlm.Path && IsPkgPrivateFromPkgPath(pkgPath) {
panic("cannot persist object of type defined in a private realm")
}
}
} WDYT ? |
would not be safer to use a visited map ? : c73fde0 ? |
This PR add a new optional flag
private
in thegnomod.toml
file.Only realm can use this flag.
The rules of a private realm are:
These rules aim to one main goal: make the realm swappable without any effect on other realms.
Actual Working Process