Skip to content
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

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

Closed
wants to merge 24 commits into from

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Mar 29, 2024

Closes #974.

The first part of this PR addresses the issue linked above. The issue details a scenario in which pointer ownership was able to be stolen by passing the pointer value to another realm that saved the value to it's own realm during initialization. The solution for this is to persist the mem package before running any init functions. This should be fine because realm finalization happens the same number of times.

The realm initialization behavior before (still the behavior for packages):

  1. Finalize realm after each init function
  2. Finalize the mem package

New behavior for realms:

  1. Finalize the mem package
  2. Finalize the realm after each init function

The second part of this PR attempts to clarify what the expected behavior should be when sharing pointers to values amongst realms. It achieves this by updating the effective gno docs to detail the expected behavior and to generally discourage users from doing things like this unless there is a good reason to do so and they have a strong understanding of how this is expected to work. There are also clarifying txtar tests that have been added to test the expected behavior.

TODO: remove the above docs changes. It was decided not to allow realms to store references to objects in other realms -- #2332

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@deelawn deelawn requested review from jaekwon, piux2, thehowl, moul and a team as code owners March 29, 2024 20:17
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.27%. Comparing base (b2f12a9) to head (4c59eb3).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   48.25%   48.27%   +0.02%     
==========================================
  Files         408      409       +1     
  Lines       62338    62360      +22     
==========================================
+ Hits        30081    30106      +25     
+ Misses      29749    29737      -12     
- Partials     2508     2517       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deelawn deelawn marked this pull request as draft March 29, 2024 21:15
@deelawn
Copy link
Contributor Author

deelawn commented Mar 29, 2024

Marking as draft until the failing tests are fixed. I took a brief look and I see two categories of failing tests -- I may be completely wrong about these and will need to spend more time looking at them.

  1. Tests are failing that are relying on state to be saved in packages. Tests previously allowed package state because the mem package was persisted after running init. Now that the mem package is saved before running init, state variables initialized within the init function won't get persisted (maybe) source addressed by 77032fc and
    5342c67
  2. Saving the mem package before init is causing an issue when trying to assign a value to a realm interface type variable from within the init. It's not just the assign, actually the further mutations on the object (reassignments via star expr) that are causing some issues when finalizing the realm. source

I think this change exposes a known issue with trying to assign a value to a nil realm pointer variable that is of a declared type -- #1326

}

// RunFilesWithMemPkg is almost the same as RunFiles; the difference is that it
// saves the mempackage to the store.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is inconsistent with its behavior though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gut tells me not to add saving logic into runFiles logic, but instead runFiles could return new init functions as []Name, and the caller can save before or after as needed.

I'mstill trying to wrap my head around why it's needed. thinking..

@zivkovicmilos
Copy link
Member

Relevant, older PR by @n0izn0iz :
#1074

@jaekwon
Copy link
Contributor

jaekwon commented Apr 17, 2024

back on this.

@Kouteki
Copy link
Contributor

Kouteki commented May 23, 2024

@mvertes @petar-dambovaliev please look into this

@deelawn deelawn changed the title fix: adjust and clarify behavior of pointers and values shared between realms fix(gnovm): change initialization order so realm objects are persisted before referenced by other realms Jun 11, 2024
@deelawn
Copy link
Contributor Author

deelawn commented Jun 27, 2024

Closing; #2255 also fixed this issue.

@deelawn deelawn closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: No status
Development

Successfully merging this pull request may close these issues.

Passing a pointer to another realm that assigns it allows to steal objects ownership
4 participants