From d5df310848a0d46e33a0a2b929275d38425a7a7d Mon Sep 17 00:00:00 2001 From: Dmytro Kozhevin Date: Thu, 20 Jul 2023 12:40:10 -0400 Subject: [PATCH] Make some host errors non-recoverable in `try_call`. (#945) * Make some host errors non-recoverable in `try_call`. - Internal errors that indicate host implementation/setup bugs - a good chunk of it indicates severely broken host state and we probably don't want to continue execution if that happens. We also would want to learn about these ASAP - Running out of budget - while this would be handled 'automatically' during the normal execution, this makes test experience a bit more consistent (as compiled contracts are not metered) - Accessing entries outside of the footprint - this is probably the most controversial change, but there is really no indication/expectation that storage interface doesn't behave as a key-value database and hence it's pretty easy to write logic that relies on that fact without considering the existence of the footprint. `try_call` recovering from footprint errors is functionally equivalent to `get_contract_data` recovering from footprint errors (imagine a simple storage wrapper contract) and we really don't want allowing that. * Make generic wasmi errors `InvalidAction`. This allows reserving internal errors for wasm-related logic errors on the host side. --- soroban-env-common/src/error.rs | 2 +- soroban-env-host/src/host.rs | 15 +++++++++++---- soroban-env-host/src/host/error.rs | 25 +++++++++++++++++++++++++ soroban-env-host/src/storage.rs | 14 ++++++++++---- soroban-env-host/src/test/hostile.rs | 6 +++--- soroban-env-host/src/test/invocation.rs | 8 ++++---- 6 files changed, 54 insertions(+), 16 deletions(-) diff --git a/soroban-env-common/src/error.rs b/soroban-env-common/src/error.rs index 04126e2f9..e6500eb98 100644 --- a/soroban-env-common/src/error.rs +++ b/soroban-env-common/src/error.rs @@ -161,7 +161,7 @@ impl From for Error { impl From for Error { fn from(code: wasmi::core::TrapCode) -> Self { let ec = match code { - wasmi::core::TrapCode::UnreachableCodeReached => ScErrorCode::InternalError, + wasmi::core::TrapCode::UnreachableCodeReached => ScErrorCode::InvalidAction, wasmi::core::TrapCode::MemoryOutOfBounds | wasmi::core::TrapCode::TableOutOfBounds => { ScErrorCode::IndexBounds diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index d770a6e63..6f6a945eb 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -701,7 +701,7 @@ impl Host { .ok_or_else(|| { self.err( ScErrorType::Auth, - ScErrorCode::InternalError, + ScErrorCode::InvalidAction, "previous invocation is missing - no auth data to get", &[], ) @@ -774,7 +774,7 @@ impl Host { .ok_or_else(|| { self.err( ScErrorType::Auth, - ScErrorCode::InternalError, + ScErrorCode::InvalidAction, "previous invocation is missing - no auth data to get", &[], ) @@ -2372,7 +2372,14 @@ impl VmCallerEnv for Host { &[func.to_val(), args.to_val()], ) })?; - Ok(e.error.to_val()) + // Only allow to gracefully handle the recoverable errors. + // Non-recoverable errors should still cause guest to panic and + // abort execution. + if e.is_recoverable() { + Ok(e.error.to_val()) + } else { + Err(e) + } } } } @@ -2921,7 +2928,7 @@ impl VmCallerEnv for Host { Frame::HostFunction(_) => { return Err(self.err( ScErrorType::Context, - ScErrorCode::InvalidAction, + ScErrorCode::InternalError, "require_auth is not suppported for host fns", &[], )) diff --git a/soroban-env-host/src/host/error.rs b/soroban-env-host/src/host/error.rs index f9c4cd790..b1640e432 100644 --- a/soroban-env-host/src/host/error.rs +++ b/soroban-env-host/src/host/error.rs @@ -109,6 +109,31 @@ impl HostError { } } } + + /// Identifies whether the error can be meaningfully recovered from. + /// + /// We consider errors that occur due to broken execution preconditions ( + /// such as incorrect footprint) non-recoverable. + pub fn is_recoverable(&self) -> bool { + // All internal errors that originate from the host can be considered + // non-recoverable (they should only appear if there is some bug in the + // host implementation or setup). + if !self.error.is_type(ScErrorType::Contract) + && self.error.is_code(ScErrorCode::InternalError) + { + return false; + } + // Exceeding the budget or storage limit is non-recoverable. Exceeding + // storage 'limit' is basically accessing entries outside of the + // supplied footprint. + if self.error.is_code(ScErrorCode::ExceededLimit) + && (self.error.is_type(ScErrorType::Storage) || self.error.is_type(ScErrorType::Budget)) + { + return false; + } + + true + } } impl From for HostError diff --git a/soroban-env-host/src/storage.rs b/soroban-env-host/src/storage.rs index f95357fa3..cd84b50ef 100644 --- a/soroban-env-host/src/storage.rs +++ b/soroban-env-host/src/storage.rs @@ -113,17 +113,23 @@ impl Footprint { ty: AccessType, budget: &Budget, ) -> Result<(), HostError> { + // `ExceededLimit` is not the most precise term here, but footprint has + // to be externally supplied in a similar fashion to budget and it's + // also representing an execution resource limit (number of ledger + // entries to access), so it might be considered 'exceeded'. + // This also helps distinguish access errors from the values simply + // being missing from storage (but with a valid footprint). if let Some(existing) = self.0.get::>(key, budget)? { match (existing, ty) { (AccessType::ReadOnly, AccessType::ReadOnly) => Ok(()), (AccessType::ReadOnly, AccessType::ReadWrite) => { - Err((ScErrorType::Storage, ScErrorCode::InvalidAction).into()) + Err((ScErrorType::Storage, ScErrorCode::ExceededLimit).into()) } (AccessType::ReadWrite, AccessType::ReadOnly) => Ok(()), (AccessType::ReadWrite, AccessType::ReadWrite) => Ok(()), } } else { - Err((ScErrorType::Storage, ScErrorCode::MissingValue).into()) + Err((ScErrorType::Storage, ScErrorCode::ExceededLimit).into()) } } } @@ -420,7 +426,7 @@ mod test_footprint { let res = fp.enforce_access(&key, AccessType::ReadOnly, &budget); assert!(HostError::result_matches_err( res, - (ScErrorType::Storage, ScErrorCode::MissingValue) + (ScErrorType::Storage, ScErrorCode::ExceededLimit) )); Ok(()) } @@ -440,7 +446,7 @@ mod test_footprint { let res = fp.enforce_access(&key, AccessType::ReadWrite, &budget); assert!(HostError::result_matches_err( res, - (ScErrorType::Storage, ScErrorCode::InvalidAction) + (ScErrorType::Storage, ScErrorCode::ExceededLimit) )); Ok(()) } diff --git a/soroban-env-host/src/test/hostile.rs b/soroban-env-host/src/test/hostile.rs index 25e79dbe0..956fb2a3e 100644 --- a/soroban-env-host/src/test/hostile.rs +++ b/soroban-env-host/src/test/hostile.rs @@ -55,7 +55,7 @@ fn hostile_ssmash_traps() -> Result<(), HostError> { assert!(HostError::result_matches_err( res, - (ScErrorType::WasmVm, ScErrorCode::InternalError) + (ScErrorType::WasmVm, ScErrorCode::InvalidAction) )); Ok(()) } @@ -73,7 +73,7 @@ fn hostile_oob1_traps() -> Result<(), HostError> { assert!(HostError::result_matches_err( res, - (ScErrorType::WasmVm, ScErrorCode::InternalError) + (ScErrorType::WasmVm, ScErrorCode::InvalidAction) )); Ok(()) } @@ -90,7 +90,7 @@ fn hostile_oob2_traps() -> Result<(), HostError> { ); assert!(HostError::result_matches_err( res, - (ScErrorType::WasmVm, ScErrorCode::InternalError) + (ScErrorType::WasmVm, ScErrorCode::InvalidAction) )); Ok(()) } diff --git a/soroban-env-host/src/test/invocation.rs b/soroban-env-host/src/test/invocation.rs index 9b9af7e22..61ce5eeb7 100644 --- a/soroban-env-host/src/test/invocation.rs +++ b/soroban-env-host/src/test/invocation.rs @@ -24,7 +24,7 @@ fn invoke_single_contract_function() -> Result<(), HostError> { Symbol::try_from_small_str("add")?, host.test_vec_obj(&[a, c])?, ); - let code = (ScErrorType::WasmVm, ScErrorCode::InternalError); + let code = (ScErrorType::WasmVm, ScErrorCode::InvalidAction); assert!(HostError::result_matches_err(res, code)); Ok(()) } @@ -126,7 +126,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> { // try call -- add will trap, and add_with will trap, but we will get a status let status = host.try_call(id0_obj, sym, args)?; - let code = (ScErrorType::WasmVm, ScErrorCode::InternalError); + let code = (ScErrorType::WasmVm, ScErrorCode::InvalidAction); let exp: Error = code.into(); assert_eq!(status.get_payload(), exp.to_val().get_payload()); @@ -135,7 +135,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> { let last_event = events.last().unwrap(); // run `UPDATE_EXPECT=true cargo test` to update this. let expected = expect![[ - r#"[Diagnostic Event] topics:[error, Error(WasmVm, InternalError)], data:["contract try_call failed", add_with, [2147483647, 1, Bytes()]]"# + r#"[Diagnostic Event] topics:[error, Error(WasmVm, InvalidAction)], data:["contract try_call failed", add_with, [2147483647, 1, Bytes()]]"# ]]; let actual = format!("{}", last_event); expected.assert_eq(&actual); @@ -149,7 +149,7 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> { let last_event = events.last().unwrap(); // run `UPDATE_EXPECT=true cargo test` to update this. let expected = expect![[ - r#"[Diagnostic Event] topics:[error, Error(WasmVm, InternalError)], data:["contract call failed", add_with, [2147483647, 1, Bytes()]]"# + r#"[Diagnostic Event] topics:[error, Error(WasmVm, InvalidAction)], data:["contract call failed", add_with, [2147483647, 1, Bytes()]]"# ]]; let actual = format!("{}", last_event); expected.assert_eq(&actual);