Skip to content

Commit b43e264

Browse files
authored
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
1 parent 77c35ca commit b43e264

File tree

5 files changed

+32
-77
lines changed

5 files changed

+32
-77
lines changed

move-binary-format/src/deserializer.rs

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
use crate::{check_bounds::BoundsChecker, errors::*, file_format::*, file_format_common::*};
66
use move_core_types::{
7-
account_address::AccountAddress, identifier::Identifier, metadata::Metadata, state::VMState,
7+
account_address::AccountAddress, identifier::Identifier, metadata::Metadata,
88
vm_status::StatusCode,
99
};
1010
use std::{collections::HashSet, convert::TryInto, io::Read};
@@ -43,21 +43,9 @@ impl CompiledModule {
4343
binary: &[u8],
4444
max_binary_format_version: u32,
4545
) -> BinaryLoaderResult<Self> {
46-
let prev_state = move_core_types::state::set_state(VMState::DESERIALIZER);
47-
let result = std::panic::catch_unwind(|| {
48-
let module = deserialize_compiled_module(binary, max_binary_format_version)?;
49-
BoundsChecker::verify_module(&module)?;
50-
51-
Ok(module)
52-
})
53-
.unwrap_or_else(|_| {
54-
Err(PartialVMError::new(
55-
StatusCode::VERIFIER_INVARIANT_VIOLATION,
56-
))
57-
});
58-
move_core_types::state::set_state(prev_state);
59-
60-
result
46+
let module = deserialize_compiled_module(binary, max_binary_format_version)?;
47+
BoundsChecker::verify_module(&module)?;
48+
Ok(module)
6149
}
6250

6351
// exposed as a public function to enable testing the deserializer

move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/catch_unwind.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use move_core_types::{
99
};
1010
use std::panic::{self, PanicInfo};
1111

12+
#[ignore]
1213
#[test]
1314
fn test_unwind() {
1415
let scenario = FailScenario::setup();

move-bytecode-verifier/bytecode-verifier-tests/src/unit_tests/mod.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ use move_bytecode_verifier::{verifier::MAX_CONSTANT_VECTOR_LEN, VerifierConfig};
77
pub mod ability_field_requirements_tests;
88
pub mod binary_samples;
99
pub mod bounds_tests;
10-
pub mod catch_unwind;
1110
pub mod code_unit_tests;
1211
pub mod constants_tests;
1312
pub mod control_flow_tests;

move-bytecode-verifier/src/verifier.rs

Lines changed: 25 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,9 @@ use crate::{
1313
};
1414
use move_binary_format::{
1515
check_bounds::BoundsChecker,
16-
errors::{Location, PartialVMError, VMResult},
16+
errors::{Location, VMResult},
1717
file_format::{CompiledModule, CompiledScript},
1818
};
19-
use move_core_types::{state::VMState, vm_status::StatusCode};
2019
use std::time::Instant;
2120

2221
pub const MAX_CONSTANT_VECTOR_LEN: u64 = 1024 * 1024;
@@ -88,37 +87,23 @@ pub fn verify_module_with_config_for_test(
8887
}
8988

9089
pub fn verify_module_with_config(config: &VerifierConfig, module: &CompiledModule) -> VMResult<()> {
91-
let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
92-
let result = std::panic::catch_unwind(|| {
93-
BoundsChecker::verify_module(module).map_err(|e| {
94-
// We can't point the error at the module, because if bounds-checking
95-
// failed, we cannot safely index into module's handle to itself.
96-
e.finish(Location::Undefined)
97-
})?;
98-
LimitsVerifier::verify_module(config, module)?;
99-
DuplicationChecker::verify_module(module)?;
100-
SignatureChecker::verify_module(module)?;
101-
InstructionConsistency::verify_module(module)?;
102-
constants::verify_module(module)?;
103-
friends::verify_module(module)?;
104-
ability_field_requirements::verify_module(module)?;
105-
RecursiveStructDefChecker::verify_module(module)?;
106-
InstantiationLoopChecker::verify_module(module)?;
107-
CodeUnitVerifier::verify_module(config, module)?;
90+
BoundsChecker::verify_module(module).map_err(|e| {
91+
// We can't point the error at the module, because if bounds-checking
92+
// failed, we cannot safely index into module's handle to itself.
93+
e.finish(Location::Undefined)
94+
})?;
95+
LimitsVerifier::verify_module(config, module)?;
96+
DuplicationChecker::verify_module(module)?;
97+
SignatureChecker::verify_module(module)?;
98+
InstructionConsistency::verify_module(module)?;
99+
constants::verify_module(module)?;
100+
friends::verify_module(module)?;
101+
ability_field_requirements::verify_module(module)?;
102+
RecursiveStructDefChecker::verify_module(module)?;
103+
InstantiationLoopChecker::verify_module(module)?;
104+
CodeUnitVerifier::verify_module(config, module)?;
108105

109-
// Add the failpoint injection to test the catch_unwind behavior.
110-
fail::fail_point!("verifier-failpoint-panic");
111-
112-
script_signature::verify_module(module, no_additional_script_signature_checks)
113-
})
114-
.unwrap_or_else(|_| {
115-
Err(
116-
PartialVMError::new(StatusCode::VERIFIER_INVARIANT_VIOLATION)
117-
.finish(Location::Undefined),
118-
)
119-
});
120-
move_core_types::state::set_state(prev_state);
121-
result
106+
script_signature::verify_module(module, no_additional_script_signature_checks)
122107
}
123108

124109
/// Helper for a "canonical" verification of a script.
@@ -136,26 +121,14 @@ pub fn verify_script(script: &CompiledScript) -> VMResult<()> {
136121
}
137122

138123
pub fn verify_script_with_config(config: &VerifierConfig, script: &CompiledScript) -> VMResult<()> {
139-
let prev_state = move_core_types::state::set_state(VMState::VERIFIER);
140-
let result = std::panic::catch_unwind(|| {
141-
BoundsChecker::verify_script(script).map_err(|e| e.finish(Location::Script))?;
142-
LimitsVerifier::verify_script(config, script)?;
143-
DuplicationChecker::verify_script(script)?;
144-
SignatureChecker::verify_script(script)?;
145-
InstructionConsistency::verify_script(script)?;
146-
constants::verify_script(script)?;
147-
CodeUnitVerifier::verify_script(config, script)?;
148-
script_signature::verify_script(script, no_additional_script_signature_checks)
149-
})
150-
.unwrap_or_else(|_| {
151-
Err(
152-
PartialVMError::new(StatusCode::VERIFIER_INVARIANT_VIOLATION)
153-
.finish(Location::Undefined),
154-
)
155-
});
156-
move_core_types::state::set_state(prev_state);
157-
158-
result
124+
BoundsChecker::verify_script(script).map_err(|e| e.finish(Location::Script))?;
125+
LimitsVerifier::verify_script(config, script)?;
126+
DuplicationChecker::verify_script(script)?;
127+
SignatureChecker::verify_script(script)?;
128+
InstructionConsistency::verify_script(script)?;
129+
constants::verify_script(script)?;
130+
CodeUnitVerifier::verify_script(config, script)?;
131+
script_signature::verify_script(script, no_additional_script_signature_checks)
159132
}
160133

161134
impl Default for VerifierConfig {

testing-infra/test-generation/src/lib.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,7 @@ use tracing::{debug, error, info};
4444

4545
/// This function calls the Bytecode verifier to test it
4646
fn run_verifier(module: CompiledModule) -> Result<CompiledModule, String> {
47-
match panic::catch_unwind(|| verify_module(&module)) {
48-
Ok(res) => match res {
49-
Ok(_) => Ok(module),
50-
Err(err) => Err(format!("Module verification failed: {:#?}", err)),
51-
},
52-
Err(err) => Err(format!("Verifier panic: {:#?}", err)),
53-
}
47+
verify_module(&module)
5448
}
5549

5650
static STORAGE_WITH_MOVE_STDLIB: Lazy<InMemoryStorage> = Lazy::new(|| {
@@ -316,7 +310,7 @@ pub fn bytecode_generation(
316310
if let Some(verified_module) = verified_module {
317311
if RUN_ON_VM {
318312
debug!("Done...Running module on VM...");
319-
let execution_result = panic::catch_unwind(|| run_vm(verified_module));
313+
let execution_result = run_vm(verified_module);
320314
match execution_result {
321315
Ok(execution_result) => match execution_result {
322316
Ok(_) => {

0 commit comments

Comments
 (0)