Skip to content

Commit

Permalink
Fix fuzzer-found frame stack corruption in rollback if instance stora…
Browse files Browse the repository at this point in the history
…ge fails (#1173)

This used to be a panic but as of #1160 it's just an `InternalError`.
Still, not something we want!
  • Loading branch information
graydon authored Nov 3, 2023
1 parent 696d6f9 commit dce34f9
Showing 1 changed file with 12 additions and 7 deletions.
19 changes: 12 additions & 7 deletions soroban-env-host/src/host/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,6 @@ impl Host {
/// and storage map to the state in the provided [`RollbackPoint`].
pub(super) fn pop_frame(&self, orp: Option<RollbackPoint>) -> Result<(), HostError> {
let _span = tracy_span!("pop frame");
// Instance storage is tied to the frame and only exists in-memory. So
// instead of snapshotting it and rolling it back, we just flush the
// changes only when rollback is not needed.
if orp.is_none() {
self.persist_instance_storage()?;
}

if self.try_borrow_context_mut()?.pop().is_none() {
return Err(self.err(
Expand Down Expand Up @@ -358,7 +352,7 @@ impl Host {
}
let rp = self.push_frame(frame)?;
let res = f();
let res = if let Ok(v) = res {
let mut res = if let Ok(v) = res {
if let Ok(err) = Error::try_from(v) {
Err(self.error(err, "escalating Ok(Error) frame-exit to Err(Error)", &[]))
} else {
Expand All @@ -367,6 +361,17 @@ impl Host {
} else {
res
};

// We try flushing instance storage at the end of the frame if nothing
// else failed. Unfortunately flushing instance storage is _itself_
// fallible in a variety of ways, and if it fails we want to roll back
// everything else.
if res.is_ok() {
if let Err(e) = self.persist_instance_storage() {
res = Err(e)
}
}

if res.is_err() {
// Pop and rollback on error.
self.pop_frame(Some(rp))?;
Expand Down

0 comments on commit dce34f9

Please sign in to comment.