[vm] Fix native function errors

- Some native functions (most notably LCS) threw system-level errors instead of aborting.
- Changed the NativeContext to prevent accidentally propegatting errors in a few spots
- Fixed spot in LCS
- Fixed code that needlessly was wrapped in a Result

Closes: aptos-labs#5680

language/move-lang/functional-tests/tests/natives/lcs_large_value.move

@@ -0,0 +1,87 @@
+
+module M {
+    use 0x1::LCS;
+
+    struct Box<T> { x: T }
+    struct Box3<T> { x: Box<Box<T>> }
+    struct Box7<T> { x: Box3<Box3<T>> }
+    struct Box15<T> { x: Box7<Box7<T>> }
+    struct Box31<T> { x: Box15<Box15<T>> }
+    struct Box63<T> { x: Box31<Box31<T>> }
+    struct Box127<T> { x: Box63<Box63<T>> }
+    struct Box255<T> { x: Box127<Box127<T>> }
+
+    fun box3<T>(x: T): Box3<T> {
+        Box3 { x: Box { x: Box { x } } }
+    }
+
+    fun box7<T>(x: T): Box7<T> {
+        Box7 { x: box3(box3(x)) }
+    }
+
+    fun box15<T>(x: T): Box15<T> {
+        Box15 { x: box7(box7(x)) }
+    }
+
+    fun box31<T>(x: T): Box31<T> {
+        Box31 { x: box15(box15(x)) }
+    }
+
+    fun box63<T>(x: T): Box63<T> {
+        Box63 { x: box31(box31(x)) }
+    }
+
+    fun box127<T>(x: T): Box127<T> {
+        Box127 { x: box63(box63(x)) }
+    }
+
+    fun box255<T>(x: T): Box255<T> {
+        Box255 { x: box127(box127(x)) }
+    }
+
+    public fun encode_128(): vector<u8> {
+        LCS::to_bytes(&box127(true))
+    }
+
+    public fun encode_256(): vector<u8> {
+        LCS::to_bytes(&box255(true))
+    }
+
+    public fun encode_257(): vector<u8> {
+        LCS::to_bytes(&Box { x: box255(true) })
+    }
+}
+// check: EXECUTED
+
+
+//! new-transaction
+script {
+    use {{default}}::M;
+
+    fun main() {
+        M::encode_128();
+    }
+}
+// check: EXECUTED
+
+
+//! new-transaction
+script {
+    use {{default}}::M;
+
+    fun main() {
+        M::encode_256();
+    }
+}
+// check: EXECUTED
+
+
+//! new-transaction
+script {
+    use {{default}}::M;
+
+    fun main() {
+        M::encode_257();
+    }
+}
+// check: ABORTED { code: 453

language/move-vm/natives/src/event.rs

@@ -23,7 +23,7 @@ pub fn native_emit_event(
     let seq_num = pop_arg!(arguments, u64);
     let guid = pop_arg!(arguments, Vec<u8>);
 
-    context.save_event(guid, seq_num, ty, msg)?;
+    context.save_event(guid, seq_num, ty, msg);
 
     Ok(NativeResult::ok(ZERO_GAS_UNITS, vec![]))
 }

language/move-vm/natives/src/lcs.rs

@@ -24,8 +24,11 @@ pub fn native_to_bytes(
 
     let arg_type = ty_args.pop().unwrap();
     // delegate to the LCS serialization for `Value`
-    let layout = context.type_to_type_layout(&arg_type)?;
-    let serialized_value = match ref_to_val.read_ref()?.simple_serialize(&layout) {
+    let serialized_value_opt = match context.type_to_type_layout(&arg_type)? {
+        None => None,
+        Some(layout) => ref_to_val.read_ref()?.simple_serialize(&layout),
+    };
+    let serialized_value = match serialized_value_opt {
         None => {
             let cost = native_gas(context.cost_table(), NativeCostIndex::LCS_TO_BYTES, 1);
             return Ok(NativeResult::err(cost, NFE_LCS_SERIALIZATION_FAILURE));

language/move-vm/runtime/src/loader.rs

@@ -796,7 +796,7 @@ impl Loader {
             ));
         }
         for (ty, expected_k) in ty_args.iter().zip(constraints) {
-            let k = if self.is_resource(ty)? {
+            let k = if self.is_resource(ty) {
                 Kind::Resource
             } else {
                 Kind::Copyable
@@ -842,14 +842,15 @@ impl Loader {
         )
     }
 
-    fn is_resource(&self, type_: &Type) -> PartialVMResult<bool> {
+    fn is_resource(&self, type_: &Type) -> bool {
         match type_ {
-            Type::Struct(idx) => Ok(self
-                .module_cache
-                .lock()
-                .unwrap()
-                .struct_at(*idx)
-                .is_resource),
+            Type::Struct(idx) => {
+                self.module_cache
+                    .lock()
+                    .unwrap()
+                    .struct_at(*idx)
+                    .is_resource
+            }
             Type::StructInstantiation(idx, instantiation) => {
                 if self
                     .module_cache
@@ -858,18 +859,18 @@ impl Loader {
                     .struct_at(*idx)
                     .is_resource
                 {
-                    Ok(true)
+                    true
                 } else {
                     for ty in instantiation {
-                        if self.is_resource(ty)? {
-                            return Ok(true);
+                        if self.is_resource(ty) {
+                            return true;
                         }
                     }
-                    Ok(false)
+                    false
                 }
             }
             Type::Vector(ty) => self.is_resource(ty),
-            _ => Ok(false),
+            _ => false,
         }
     }
 }
@@ -1005,7 +1006,7 @@ impl<'a> Resolver<'a> {
         ))
     }
 
-    pub(crate) fn is_resource(&self, ty: &Type) -> PartialVMResult<bool> {
+    pub(crate) fn is_resource(&self, ty: &Type) -> bool {
         self.loader.is_resource(ty)
     }
 
@@ -1025,7 +1026,7 @@ impl<'a> Resolver<'a> {
         } else {
             let mut is_resource = false;
             for ty in &struct_inst.instantiation {
-                if self.is_resource(&ty.subst(instantiation)?)? {
+                if self.is_resource(&ty.subst(instantiation)?) {
                     is_resource = true;
                 }
             }
@@ -1855,7 +1856,7 @@ impl Loader {
         let mut is_resource = struct_type.is_resource;
         if !is_resource {
             for ty in ty_args {
-                if self.is_resource(ty)? {
+                if self.is_resource(ty) {
                     is_resource = true;
                 }
             }

language/move-vm/runtime/src/native_functions.rs

@@ -5,6 +5,7 @@ use crate::{interpreter::Interpreter, loader::Resolver};
 use libra_types::account_config::CORE_CODE_ADDRESS;
 use move_core_types::{
     account_address::AccountAddress, gas_schedule::CostTable, value::MoveTypeLayout,
+    vm_status::StatusType,
 };
 use move_vm_natives::{account, debug, event, hash, lcs, signature, signer, vector};
 use move_vm_types::{
@@ -86,7 +87,7 @@ impl NativeFunction {
         t: Vec<Type>,
         v: VecDeque<Value>,
     ) -> PartialVMResult<NativeResult> {
-        match self {
+        let result = match self {
             Self::HashSha2_256 => hash::native_sha2_256(ctx, t, v),
             Self::HashSha3_256 => hash::native_sha3_256(ctx, t, v),
             Self::PubED25519Validate => signature::native_ed25519_publickey_validation(ctx, t, v),
@@ -107,7 +108,12 @@ impl NativeFunction {
             Self::SignerBorrowAddress => signer::native_borrow_address(ctx, t, v),
             Self::CreateSigner => account::native_create_signer(ctx, t, v),
             Self::DestroySigner => account::native_destroy_signer(ctx, t, v),
-        }
+        };
+        debug_assert!(match &result {
+            Err(e) => e.major_status().status_type() == StatusType::InvariantViolation,
+            Ok(_) => true,
+        });
+        result
     }
 }
 
@@ -144,21 +150,19 @@ impl<'a> NativeContext for FunctionContext<'a> {
         self.cost_strategy.cost_table()
     }
 
-    fn save_event(
-        &mut self,
-        guid: Vec<u8>,
-        seq_num: u64,
-        ty: Type,
-        val: Value,
-    ) -> PartialVMResult<()> {
-        Ok(self.data_store.emit_event(guid, seq_num, ty, val))
+    fn save_event(&mut self, guid: Vec<u8>, seq_num: u64, ty: Type, val: Value) {
+        self.data_store.emit_event(guid, seq_num, ty, val)
     }
 
-    fn type_to_type_layout(&self, ty: &Type) -> PartialVMResult<MoveTypeLayout> {
-        self.resolver.type_to_type_layout(ty)
+    fn type_to_type_layout(&self, ty: &Type) -> PartialVMResult<Option<MoveTypeLayout>> {
+        match self.resolver.type_to_type_layout(ty) {
+            Ok(ty_layout) => Ok(Some(ty_layout)),
+            Err(e) if e.major_status().status_type() == StatusType::InvariantViolation => Err(e),
+            Err(_) => Ok(None),
+        }
     }
 
-    fn is_resource(&self, ty: &Type) -> PartialVMResult<bool> {
+    fn is_resource(&self, ty: &Type) -> bool {
         self.resolver.is_resource(ty)
     }
 }

language/move-vm/types/src/natives/function.rs

@@ -24,6 +24,9 @@ use move_core_types::{
 use std::fmt::Write;
 use vm::errors::PartialVMResult;
 
+pub use move_core_types::vm_status::StatusCode;
+pub use vm::errors::PartialVMError;
+
 /// `NativeContext` - Native function context.
 ///
 /// This is the API, the "privileges", a native function is given.
@@ -36,17 +39,11 @@ pub trait NativeContext {
     /// Gets cost table ref.
     fn cost_table(&self) -> &CostTable;
     /// Saves contract event.
-    fn save_event(
-        &mut self,
-        guid: Vec<u8>,
-        count: u64,
-        ty: Type,
-        val: Value,
-    ) -> PartialVMResult<()>;
+    fn save_event(&mut self, guid: Vec<u8>, count: u64, ty: Type, val: Value);
     /// Get the a data layout via the type.
-    fn type_to_type_layout(&self, ty: &Type) -> PartialVMResult<MoveTypeLayout>;
+    fn type_to_type_layout(&self, ty: &Type) -> PartialVMResult<Option<MoveTypeLayout>>;
     /// Whether a type is a resource or not.
-    fn is_resource(&self, ty: &Type) -> PartialVMResult<bool>;
+    fn is_resource(&self, ty: &Type) -> bool;
 }
 
 /// Result of a native function execution requires charges for execution cost.
@@ -102,6 +99,15 @@ pub fn native_gas(table: &CostTable, key: NativeCostIndex, size: usize) -> GasUn
 #[macro_export]
 macro_rules! pop_arg {
     ($arguments:ident, $t:ty) => {{
-        $arguments.pop_back().unwrap().value_as::<$t>()?
+        use $crate::natives::function::{NativeResult, PartialVMError, StatusCode};
+        match $arguments.pop_back().map(|v| v.value_as::<$t>()) {
+            None => {
+                return Err(PartialVMError::new(
+                    StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,
+                ))
+            }
+            Some(Err(e)) => return Err(e),
+            Some(Ok(v)) => v,
+        }
     }};
 }

language/move-vm/types/src/values/values_impl.rs

@@ -1790,7 +1790,7 @@ fn check_elem_layout(
     ty: &Type,
     v: &Container,
 ) -> PartialVMResult<()> {
-    let is_resource = context.is_resource(ty)?;
+    let is_resource = context.is_resource(ty);
 
     match (ty, v) {
         (Type::U8, Container::VecU8(_))
@@ -2006,7 +2006,7 @@ impl Vector {
             )))),
 
             Type::Vector(_) | Type::Struct(_) | Type::StructInstantiation(_, _) => {
-                if context.is_resource(type_param)? {
+                if context.is_resource(type_param) {
                     Value(ValueImpl::Container(Container::VecR(Rc::new(
                         RefCell::new(vec![]),
                     ))))