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

virtualization: properly restore cold/warm states, cache, and scry stack when catching an error #158

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

eamsden
Copy link
Collaborator

@eamsden eamsden commented Dec 4, 2023

Fixed a bug caught by @ashelkovnykov

@ashelkovnykov please confirm my understanding. In particular:

  • jet_mute and jet_mure had the same bug as mink
  • jet_mute and jet_mure additionally did not properly reset the scry stack in the context when catching an error.
  • (not changed yet) mink further does not properly reset the scry stack in the context when catching an error

@ashelkovnykov
Copy link
Collaborator

  • jet_mure and jet_mute were also not saving Cold/Warm state correctly
  • jet_mure and jet_mute did not correctly reset the scry stack on deterministic error & blocked scry
  • mink does reset the scry stack correctly in all cases (via jet_mink)

Copy link
Collaborator Author

@eamsden eamsden left a comment

Choose a reason for hiding this comment

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

Mostly just: "shouldn't we restore the saved cache after failure?" in a bunch of different places. I might not be following something...

rust/ares/src/jets/nock.rs Outdated Show resolved Hide resolved
rust/ares/src/jets/nock.rs Outdated Show resolved Hide resolved
rust/ares/src/jets/nock.rs Outdated Show resolved Hide resolved
rust/ares/src/serf.rs Outdated Show resolved Hide resolved
@eamsden
Copy link
Collaborator Author

eamsden commented Dec 5, 2023

Notes from discussion with @ashelkovnykov this morning

  • interpret() should restore everything except the scry stack on error (since mink needs to check the scry stack as of the error to see if it should re-throw scry-crashed or convert to another error
  • All virtualization functions should call util::mink
  • I need to review that leaving the scry stack in place does not cause use-after-free

@ashelkovnykov
Copy link
Collaborator

Follow-up to @eamsden 's comments above: I tried to elide several calls and unnecessary restorations in jet_mink, jet_mure, jet_mute, but it's making more problems than it's solving. It's resulting in the logic getting slightly more spread out, but also - much worse than that - it's making the logic of when we do/don't have a valid cold state / warm state / cache / scry stack hard to follow.

Therefore, we're going to put all of the logic (except scry stack) into interpret itself, and then force all possible virtualization through util::mink. Then we can verify all of the Context is in the correct state by checking only three locations: jets::nock::util::mink, interpreter::interpret, and interpreter::exit.

Copy link
Collaborator Author

@eamsden eamsden left a comment

Choose a reason for hiding this comment

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

This looks good to me now, but it needs to be tested booting a fakezod and someone not me or @ashelkovnykov needs to review it.

@ashelkovnykov
Copy link
Collaborator

@eamsden Currently failing on fakezod boot

rust/ares/src/serf.rs Outdated Show resolved Hide resolved
@eamsden eamsden mentioned this pull request Dec 6, 2023
@ashelkovnykov ashelkovnykov marked this pull request as ready for review December 6, 2023 13:40
Copy link
Member

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this looks consistent to me

@eamsden eamsden merged commit 731ac13 into status Dec 6, 2023
1 check passed
@eamsden eamsden deleted the eamsden/context-restore branch December 6, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants