From 63cf7fe3d5ffc60db57fba97e9fc9c5778cd559c Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Fri, 21 Jul 2023 11:39:48 -0700 Subject: [PATCH] Fix panic-string-logging code path broken by recent dynamic-borrow fix. (#960) --- soroban-env-host/src/host/error.rs | 53 ++++++++++++++++++++++++++++++ soroban-env-host/src/host/frame.rs | 45 +++++++++++++++++-------- 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/soroban-env-host/src/host/error.rs b/soroban-env-host/src/host/error.rs index b1640e432..a8e9037d1 100644 --- a/soroban-env-host/src/host/error.rs +++ b/soroban-env-host/src/host/error.rs @@ -311,6 +311,59 @@ impl Host { } }) } + + // When a `Val` enters the host from the guest, say as an incoming argument + // to a host function, it is _usually_ typechecked at some specific type + // other than just `Val`. So if a contract passes a `Val` that is an `Error` + // it will _usually_ be caught as an unexpected type, and that will turn + // into a `Err(HostError)` (albeit a weird "wrong type" error, that loses + // the original error code). + // + // There are two cases where this is not sufficient to exclude Errors + // though: + // + // - When passing `Error` as an argument to a host function taking + // polymorphic argument types that are typed simply as `Val`, such as + // the third argument to `vec_put(VecObject, I32Val, Val)` + // + // - When passing or returning values to _contract functions_ themselves, + // which are (as far as the host is concerned) superficially just typed + // as polymorphic N-ary functions `(Val,Val,...,Val) -> Val`. + // + // In both these cases we _could_ allow passing `Error` as a legitimate type + // of `Val`, but we instead take a more conservative approach: `Error` is + // simply not allowed to cross the host-to-guest boundary as a `Val` at all + // (eg. inside `Ok(Val)`). + // + // We do make some exceptions to this strict rule, specifically to allow + // returning `Error` from a host function that's intended to be _fallible_ + // from the guest's perspective, i.e. the host returns `Ok(Error)` to the + // guest so that the guest VM does not trap but continues running and can + // turn `Error` into `Result::Err`, and pass it to user code typed as + // `Result`. An example host function that works this way is + // `try_call`. + // + // All other cases, including "inserting or extracting values in a + // polymorphic container", will turn an `Ok(Error)` into `Err(HostError)`, + // which will usually trap the guest (or panic if native). To enforce this + // even more strictly, we define `Error` as an invalid element of a `Vec`, + // and an invalid key or value of a `Map`, as well. + // + // Put differently: `Error` is mostly not considered a legitimate payload + // for values that are conceptually `Ok(..)` at the host/guest interface + // layer; it's _only_ allowed to be used to express `Err(..)`. This does + // cause a few cases to not-work the way users might want, but the + // alternative -- letting `Ok(Error)` cross the boundary and hoping users do + // a tag-test on their `Val`s -- is too likely to hide user errors they are + // expecting to ultimately result in transaction aborts. + // + pub fn escalate_val_error_to_hosterror(&self, val: Val) -> Result { + if let Ok(err) = Error::try_from(val) { + Err(self.error(err, "escalating Error Val to Err(HostError)", &[])) + } else { + Ok(val) + } + } } pub(crate) trait DebugArg { diff --git a/soroban-env-host/src/host/frame.rs b/soroban-env-host/src/host/frame.rs index df711eded..3db29f9b3 100644 --- a/soroban-env-host/src/host/frame.rs +++ b/soroban-env-host/src/host/frame.rs @@ -600,26 +600,43 @@ impl Host { )), Err(panic_payload) => { // Return an error indicating the contract function - // panicked. If if was a panic generated by a - // Env-upgraded HostError, it had its status - // captured by VmCallerEnv::escalate_error_to_panic: - // fish the Error stored in the frame back out and + // panicked. + // + // If it was a panic generated by a Env-upgraded + // HostError, it had its `Error` captured by + // `VmCallerEnv::escalate_error_to_panic`: fish the + // `Error` stored in the frame back out and // propagate it. - let func: Val = func.into(); - let mut error: Error = Error::from_type_and_code( - ScErrorType::Context, - ScErrorCode::InternalError, - ); - + // + // If it was a panic generated by user code calling + // panic!(...) we won't retrieve such a stored + // `Error`. Since we're trying to emulate + // what-the-VM-would-do here, and the VM traps with + // an unreachable error on contract panic, we + // generate same error (by converting a wasm + // trap-unreachable code). It's a little weird + // because we're not actually running a VM, but we + // prioritize emulation fidelity over honesty here. + let mut error: Error = + Error::from(wasmi::core::TrapCode::UnreachableCodeReached); + + let mut recovered_error_from_panic_refcell = false; if let Ok(panic) = panic.try_borrow() { if let Some(err) = *panic { + recovered_error_from_panic_refcell = true; error = err; } } - // If we're allowed to record dynamic strings (which happens - // when diagnostics are active), also log the panic payload into - // the Debug buffer. - else if self.is_debug()? { + + // If we didn't manage to recover a structured error + // code from the frame's refcell, and we're allowed + // to record dynamic strings (which happens when + // diagnostics are active), and we got a panic + // payload of a simple string, log that panic + // payload into the diagnostic event buffer. This + // code path will get hit when contracts do + // `panic!("some string")` in native testing mode. + if !recovered_error_from_panic_refcell && self.is_debug()? { if let Some(str) = panic_payload.downcast_ref::<&str>() { let msg: String = format!( "caught panic '{}' from contract function '{:?}'",