WIP final gnovm fix for noncrossing functions passed to realms #4890
Conversation
github-project-automation
bot
added this to
💪 Bounties & Worx and 🧙♂️gno.land core team
🛠 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):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
| bob.Do(obj.Method) // FAIL: not yet unattached | ||
| alice.SetClosure(cls) // obj recursively attached to alice late | ||
| bob.Do(obj.Method) // SUCCESS: attached to bob |
There was a problem hiding this comment.
| bob.Do(obj.Method) // FAIL: not yet unattached | |
| alice.SetClosure(cls) // obj recursively attached to alice late | |
| bob.Do(obj.Method) // SUCCESS: attached to bob | |
| bob.Do(obj.Method) // FAIL: not yet attached | |
| alice.SetClosure(cls) // obj recursively attached to alice late | |
| bob.Do(obj.Method) // FAIL: attached to alice (but need bob) |
?
There was a problem hiding this comment.
you're right, i'll update with a better example. bob.SetClosure().
| bob.Do(obj.Method) // FAIL: not yet unattached | ||
| alice.SetClosure(cls) // obj recursively attached via cls to alice late | ||
| bob.Do(obj.Method) // SUCCESS: attached to bob |
There was a problem hiding this comment.
| bob.Do(obj.Method) // FAIL: not yet unattached | |
| alice.SetClosure(cls) // obj recursively attached via cls to alice late | |
| bob.Do(obj.Method) // SUCCESS: attached to bob | |
| bob.Do(obj.Method) // FAIL: not yet attached | |
| alice.SetClosure(cls) // obj recursively attached via cls to alice late | |
| bob.Do(obj.Method) // FAIL: attached to alice (but need bob) |
| // -------------------------------------------------------------------------------- | ||
| // CASE rC1: method has attached receiver -- storage-cross to receiver | ||
| // - in bob.Do: (storage:[], current:caller, previous:nil) | ||
| // - in obj.Method: (storage:[bob], current:caller, previous:nil) | ||
| obj := new(alice.Object) | ||
| bob.SetObject(obj) // obj attached to bob | ||
| bob.Do(obj.Method) // SUCCESS: attached to bob |
There was a problem hiding this comment.
if in package alice, Method is declared as:
func (obj *Object) Method() { // CASE rB2~4, CASE rC1~5
// mutate unintended exported global var by assignment
bob.Private2 = -1
// mutate intended global var
bob.PrivateFunc()
}it can be inferred that a realm should never declare an exported global var, if there are any method/closure attached/injected, to avoid unintended mutation, right?
There was a problem hiding this comment.
interesting point.
bob has to be careful what he exposes; if he exposes something like this, needs to be careful about what he attaches, unless it is meant to be modified this way.
it is intentional, if we think of .Exposed variables as basically having implied GetExposed() and SetExposed() functions.
There was a problem hiding this comment.
This seems to increase the developer’s mental burden — they have to be aware of whether their realm will attach functions, and whether there are any global variables that should not be exposed.
How about requiring that all global variables of realm be mutated only through functions?
ty @moul Co-authored-by: Manfred Touron <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
bot
added
the
📦 ⛰️ gno.land
Signed-off-by: moul <[email protected]>
| @@ -0,0 +1,273 @@ | |||
| adduserfrom runner 'apart roast chief monitor bundle auto fade double valid budget able average onion slam rice flame despair wage uphold nominee proud alien spider useful' | |||
There was a problem hiding this comment.
convert the spec into txtar. the pXXXs are not included here as p cannot import r.
| // XXX, this is the inital GHSA issue. | ||
| func RD1(cur realm) { | ||
| // CASE rD1: closure attached to bob | ||
| // - in bob.ExecuteClosure: (storage:[], current:bob, previous:caller) | ||
| f := func() { | ||
| bob.AllowedList = append(bob.AllowedList, 3) | ||
| println("all right") | ||
| } | ||
|
|
||
| bob.SetClosure(cross, f) | ||
| bob.ExecuteClosure(cross) | ||
| } |
There was a problem hiding this comment.
the initial "issue". the "issue" is same as previous comment: "bob has to be careful what he exposes; if he exposes something like this, needs to be careful about what he attaches, unless it is meant to be modified this way."
This PR will finalize the gno interrealm spec.
The primary goal is to address the Oak security audit issue found.
Also will unify terminology, and double-check consistency of interrealm spec.
Making a PR to allow commenting on the initial commit of mental model.