Fix panic-string-logging code path broken by recent dynamic-borrow fi…

…x. (stellar#960)

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