Merge branch 'rent_fix' of https://github.com/dmkozh/rs-soroban-env i…

…nto rent_fix

soroban-env-common/src/error.rs

@@ -161,7 +161,7 @@ impl From<stellar_xdr::Error> for Error {
 impl From<wasmi::core::TrapCode> 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

soroban-env-host/src/host.rs

@@ -678,7 +678,7 @@ impl Host {
             .ok_or_else(|| {
                 self.err(
                     ScErrorType::Auth,
-                    ScErrorCode::InternalError,
+                    ScErrorCode::InvalidAction,
                     "previous invocation is missing - no auth data to get",
                     &[],
                 )
@@ -751,7 +751,7 @@ impl Host {
                 .ok_or_else(|| {
                     self.err(
                         ScErrorType::Auth,
-                        ScErrorCode::InternalError,
+                        ScErrorCode::InvalidAction,
                         "previous invocation is missing - no auth data to get",
                         &[],
                     )
@@ -2365,7 +2365,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)
+                }
             }
         }
     }
@@ -2914,7 +2921,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",
                         &[],
                     ))

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<T> From<T> for HostError
@@ -286,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<Val,Error>`. 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<Val, HostError> {
+        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 {

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 '{:?}'",

soroban-env-host/src/storage.rs

@@ -116,17 +116,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::<Rc<LedgerKey>>(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())
         }
     }
 }
@@ -494,7 +500,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(())
     }
@@ -514,7 +520,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(())
     }

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(())
 }

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);