Remove catch_unwind from the VM (#10849)

## Description We compile Rust code with panic = abort so the catch_unwind here, in being questionable in the first place, is not particularly useful and possible harmful ## Test Plan Existing tests --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
MystenLabs · Apr 14, 2023 · 4b1dfe4 · 4b1dfe4
1 parent 631580c
commit 4b1dfe4
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 77 deletions.
diff --git a/external-crates/move/move-binary-format/src/deserializer.rs b/external-crates/move/move-binary-format/src/deserializer.rs
@@ -4,7 +4,7 @@
 
 use crate::{check_bounds::BoundsChecker, errors::*, file_format::*, file_format_common::*};
 use move_core_types::{
-    account_address::AccountAddress, identifier::Identifier, metadata::Metadata, state::VMState,
+    account_address::AccountAddress, identifier::Identifier, metadata::Metadata,
     vm_status::StatusCode,
 };
 use std::{collections::HashSet, convert::TryInto, io::Read};
@@ -43,21 +43,9 @@ impl CompiledModule {
         binary: &[u8],
         max_binary_format_version: u32,
     ) -> BinaryLoaderResult<Self> {
-        let prev_state = move_core_types::state::set_state(VMState::DESERIALIZER);
-        let result = std::panic::catch_unwind(|| {
-            let module = deserialize_compiled_module(binary, max_binary_format_version)?;
-            BoundsChecker::verify_module(&module)?;
-
-            Ok(module)
-        })
-        .unwrap_or_else(|_| {
-            Err(PartialVMError::new(
-                StatusCode::VERIFIER_INVARIANT_VIOLATION,
-            ))
-        });
-        move_core_types::state::set_state(prev_state);
-
-        result
+        let module = deserialize_compiled_module(binary, max_binary_format_version)?;
+        BoundsChecker::verify_module(&module)?;
+        Ok(module)
     }
 
     // exposed as a public function to enable testing the deserializer

diff --git a/...crates/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/catch_unwind.rs b/...crates/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/catch_unwind.rs
@@ -9,6 +9,7 @@ use move_core_types::{
 };
 use std::panic::{self, PanicInfo};
 
+#[ignore]
 #[test]
 fn test_unwind() {
     let scenario = FailScenario::setup();

diff --git a/external-crates/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/mod.rs b/external-crates/move/move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/mod.rs
@@ -7,7 +7,6 @@ use move_bytecode_verifier::{verifier::MAX_CONSTANT_VECTOR_LEN, VerifierConfig};
 pub mod ability_field_requirements_tests;
 pub mod binary_samples;
 pub mod bounds_tests;
-pub mod catch_unwind;
 pub mod code_unit_tests;
 pub mod constants_tests;
 pub mod control_flow_tests;

diff --git a/external-crates/move/move-bytecode-verifier/src/verifier.rs b/external-crates/move/move-bytecode-verifier/src/verifier.rs
@@ -13,10 +13,9 @@ use crate::{
 };
 use move_binary_format::{
     check_bounds::BoundsChecker,
-    errors::{Location, PartialVMError, VMResult},
+    errors::{Location, VMResult},
     file_format::{CompiledModule, CompiledScript},
 };
-use move_core_types::{state::VMState, vm_status::StatusCode};
 use std::time::Instant;
 
 pub const MAX_CONSTANT_VECTOR_LEN: u64 = 1024 * 1024;
@@ -88,37 +87,23 @@ pub fn verify_module_with_config_for_test(
 }
 
 pub fn verify_module_with_config(config: &VerifierConfig, module: &CompiledModule) -> VMResult<()> {
-    let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
-    let result = std::panic::catch_unwind(|| {
-        BoundsChecker::verify_module(module).map_err(|e| {
-            // We can't point the error at the module, because if bounds-checking
-            // failed, we cannot safely index into module's handle to itself.
-            e.finish(Location::Undefined)
-        })?;
-        LimitsVerifier::verify_module(config, module)?;
-        DuplicationChecker::verify_module(module)?;
-        SignatureChecker::verify_module(module)?;
-        InstructionConsistency::verify_module(module)?;
-        constants::verify_module(module)?;
-        friends::verify_module(module)?;
-        ability_field_requirements::verify_module(module)?;
-        RecursiveStructDefChecker::verify_module(module)?;
-        InstantiationLoopChecker::verify_module(module)?;
-        CodeUnitVerifier::verify_module(config, module)?;
+    BoundsChecker::verify_module(module).map_err(|e| {
+        // We can't point the error at the module, because if bounds-checking
+        // failed, we cannot safely index into module's handle to itself.
+        e.finish(Location::Undefined)
+    })?;
+    LimitsVerifier::verify_module(config, module)?;
+    DuplicationChecker::verify_module(module)?;
+    SignatureChecker::verify_module(module)?;
+    InstructionConsistency::verify_module(module)?;
+    constants::verify_module(module)?;
+    friends::verify_module(module)?;
+    ability_field_requirements::verify_module(module)?;
+    RecursiveStructDefChecker::verify_module(module)?;
+    InstantiationLoopChecker::verify_module(module)?;
+    CodeUnitVerifier::verify_module(config, module)?;
 
-        // Add the failpoint injection to test the catch_unwind behavior.
-        fail::fail_point!("verifier-failpoint-panic");
-
-        script_signature::verify_module(module, no_additional_script_signature_checks)
-    })
-    .unwrap_or_else(|_| {
-        Err(
-            PartialVMError::new(StatusCode::VERIFIER_INVARIANT_VIOLATION)
-                .finish(Location::Undefined),
-        )
-    });
-    move_core_types::state::set_state(prev_state);
-    result
+    script_signature::verify_module(module, no_additional_script_signature_checks)
 }
 
 /// Helper for a "canonical" verification of a script.
@@ -136,26 +121,14 @@ pub fn verify_script(script: &CompiledScript) -> VMResult<()> {
 }
 
 pub fn verify_script_with_config(config: &VerifierConfig, script: &CompiledScript) -> VMResult<()> {
-    let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
-    let result = std::panic::catch_unwind(|| {
-        BoundsChecker::verify_script(script).map_err(|e| e.finish(Location::Script))?;
-        LimitsVerifier::verify_script(config, script)?;
-        DuplicationChecker::verify_script(script)?;
-        SignatureChecker::verify_script(script)?;
-        InstructionConsistency::verify_script(script)?;
-        constants::verify_script(script)?;
-        CodeUnitVerifier::verify_script(config, script)?;
-        script_signature::verify_script(script, no_additional_script_signature_checks)
-    })
-    .unwrap_or_else(|_| {
-        Err(
-            PartialVMError::new(StatusCode::VERIFIER_INVARIANT_VIOLATION)
-                .finish(Location::Undefined),
-        )
-    });
-    move_core_types::state::set_state(prev_state);
-
-    result
+    BoundsChecker::verify_script(script).map_err(|e| e.finish(Location::Script))?;
+    LimitsVerifier::verify_script(config, script)?;
+    DuplicationChecker::verify_script(script)?;
+    SignatureChecker::verify_script(script)?;
+    InstructionConsistency::verify_script(script)?;
+    constants::verify_script(script)?;
+    CodeUnitVerifier::verify_script(config, script)?;
+    script_signature::verify_script(script, no_additional_script_signature_checks)
 }
 
 impl Default for VerifierConfig {

diff --git a/external-crates/move/testing-infra/test-generation/src/lib.rs b/external-crates/move/testing-infra/test-generation/src/lib.rs
@@ -44,13 +44,7 @@ use tracing::{debug, error, info};
 
 /// This function calls the Bytecode verifier to test it
 fn run_verifier(module: CompiledModule) -> Result<CompiledModule, String> {
-    match panic::catch_unwind(|| verify_module(&module)) {
-        Ok(res) => match res {
-            Ok(_) => Ok(module),
-            Err(err) => Err(format!("Module verification failed: {:#?}", err)),
-        },
-        Err(err) => Err(format!("Verifier panic: {:#?}", err)),
-    }
+    verify_module(&module)
 }
 
 static STORAGE_WITH_MOVE_STDLIB: Lazy<InMemoryStorage> = Lazy::new(|| {
@@ -316,7 +310,7 @@ pub fn bytecode_generation(
         if let Some(verified_module) = verified_module {
             if RUN_ON_VM {
                 debug!("Done...Running module on VM...");
-                let execution_result = panic::catch_unwind(|| run_vm(verified_module));
+                let execution_result = run_vm(verified_module);
                 match execution_result {
                     Ok(execution_result) => match execution_result {
                         Ok(_) => {