diff --git a/common/proptest_helpers/Cargo.toml b/common/proptest_helpers/Cargo.toml index 04450c8f4423..5b7b5d855b98 100644 --- a/common/proptest_helpers/Cargo.toml +++ b/common/proptest_helpers/Cargo.toml @@ -7,5 +7,6 @@ publish = false edition = "2018" [dependencies] +crossbeam = "0.7.1" proptest = "0.9.4" proptest-derive = "0.1.2" diff --git a/common/proptest_helpers/src/lib.rs b/common/proptest_helpers/src/lib.rs index d42655f5b313..52b2ef0ffbdb 100644 --- a/common/proptest_helpers/src/lib.rs +++ b/common/proptest_helpers/src/lib.rs @@ -12,13 +12,45 @@ mod repeat_vec; pub use crate::{growing_subset::GrowingSubset, repeat_vec::RepeatVec}; +use crossbeam::thread; use proptest::sample::Index as PropIndex; use proptest_derive::Arbitrary; use std::{ + any::Any, collections::BTreeSet, ops::{Deref, Index as OpsIndex}, }; +/// Creates a new thread with a larger stack size. +/// +/// Generating some proptest values can overflow the stack. This allows test authors to work around +/// this limitation. +/// +/// This is expected to be used with closure-style proptest invocations: +/// +/// ``` +/// use proptest::prelude::*; +/// use proptest_helpers::with_stack_size; +/// +/// with_stack_size(4 * 1024 * 1024, || proptest!(|(x in 0usize..128)| { +/// // assertions go here +/// prop_assert!(x >= 0 && x < 128); +/// })); +/// ``` +pub fn with_stack_size<'a, F, T>(size: usize, f: F) -> Result> +where + F: FnOnce() -> T + Send + 'a, + T: Send + 'a, +{ + thread::scope(|s| { + let handle = s.builder().stack_size(size).spawn(|_| f()).map_err(|err| { + let any: Box = Box::new(err); + any + })?; + handle.join() + })? +} + /// Given a maximum value `max` and a list of [`Index`](proptest::sample::Index) instances, picks /// integers in the range `[0, max)` uniformly randomly and without duplication. /// diff --git a/execution/execution_proto/Cargo.toml b/execution/execution_proto/Cargo.toml index f4ea5dec470c..442287a85107 100644 --- a/execution/execution_proto/Cargo.toml +++ b/execution/execution_proto/Cargo.toml @@ -15,6 +15,7 @@ protobuf = "2.6" crypto = { path = "../../crypto/legacy_crypto" } failure = { path = "../../common/failure_ext", package = "failure_ext" } +proptest_helpers = { path = "../../common/proptest_helpers" } proto_conv = { path = "../../common/proto_conv", features = ["derive"] } types = { path = "../../types" } diff --git a/execution/execution_proto/src/protobuf_conversion_test.rs b/execution/execution_proto/src/protobuf_conversion_test.rs index 6680ecfbc32f..a2e6d9178182 100644 --- a/execution/execution_proto/src/protobuf_conversion_test.rs +++ b/execution/execution_proto/src/protobuf_conversion_test.rs @@ -6,6 +6,7 @@ use crate::{ ExecuteChunkRequest, ExecuteChunkResponse, }; use proptest::prelude::*; +use proptest_helpers::with_stack_size; use proto_conv::test_helper::assert_protobuf_encode_decode; proptest! { @@ -16,11 +17,6 @@ proptest! { assert_protobuf_encode_decode(&execute_block_request); } - #[test] - fn test_execute_block_response_roundtrip(execute_block_response in any::()) { - assert_protobuf_encode_decode(&execute_block_response); - } - #[test] fn test_commit_block_request_roundtrip(commit_block_request in any::()) { assert_protobuf_encode_decode(&commit_block_request); @@ -43,3 +39,13 @@ proptest! { assert_protobuf_encode_decode(&execute_chunk_response); } } + +#[test] +fn test_execute_block_response_roundtrip() { + with_stack_size(4 * 1024 * 1024, || { + proptest!(|(execute_block_response in any::())| { + assert_protobuf_encode_decode(&execute_block_response); + }) + }) + .unwrap(); +} diff --git a/language/vm/cost_synthesis/src/stack_generator.rs b/language/vm/cost_synthesis/src/stack_generator.rs index b7590b895958..0f2c79ce349c 100644 --- a/language/vm/cost_synthesis/src/stack_generator.rs +++ b/language/vm/cost_synthesis/src/stack_generator.rs @@ -351,7 +351,8 @@ where let module = self .module_cache .get_loaded_module(&module_id) - .expect("[Module Lookup] Error while looking up module") + .expect("[Module Lookup] Invariant violation while looking up module") + .expect("[Module Lookup] Runtime error while looking up module") .expect("[Module Lookup] Unable to find module"); let struct_def_idx = if self.struct_handle_table.contains_key(&module_id) { self.struct_handle_table @@ -400,7 +401,8 @@ where let module = self .module_cache .get_loaded_module(&module_id) - .expect("[Module Lookup] Error while looking up module") + .expect("[Module Lookup] Invariant violation while looking up module") + .expect("[Module Lookup] Runtime error while looking up module") .expect("[Module Lookup] Unable to find module"); let function_def_idx = if self.function_handle_table.contains_key(&module_id) { *self diff --git a/language/vm/cost_synthesis/src/vm_runner.rs b/language/vm/cost_synthesis/src/vm_runner.rs index 5214908d916c..83c6336159e2 100644 --- a/language/vm/cost_synthesis/src/vm_runner.rs +++ b/language/vm/cost_synthesis/src/vm_runner.rs @@ -48,7 +48,8 @@ macro_rules! with_loaded_vm { $module_cache.cache_module(root_module.clone()); let $mod = $module_cache .get_loaded_module(&module_id) - .expect("[Module Cache] Internal error encountered when fetching module.") + .expect("[Module Lookup] Invariant violation while looking up module") + .expect("[Module Lookup] Runtime error while looking up module") .expect("[Module Cache] Unable to find module in module cache."); for m in modules { $module_cache.cache_module(m); diff --git a/language/vm/src/errors.rs b/language/vm/src/errors.rs index c6af4cce5470..094df9669b78 100644 --- a/language/vm/src/errors.rs +++ b/language/vm/src/errors.rs @@ -6,6 +6,7 @@ use failure::Fail; use std::{fmt, iter::FromIterator}; use types::{ account_address::AccountAddress, + language_storage::ModuleId, transaction::TransactionStatus, vm_error::{VMStatus, VMValidationStatus, VMVerificationError, VMVerificationStatus}, }; @@ -51,6 +52,7 @@ pub enum VMErrorKind { ValueDeserializerError, CodeSerializerError(BinaryError), CodeDeserializerError(BinaryError), + Verification(Vec), } #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] @@ -60,6 +62,8 @@ pub enum VerificationStatus { /// A verification error was detected in a module. The first element is the index of the module /// in the transaction. Module(u16, VerificationError), + /// A verification error was detected in a dependency of a module. + Dependency(ModuleId, VerificationError), } #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] @@ -702,6 +706,9 @@ impl From<&VerificationStatus> for VMVerificationStatus { VerificationStatus::Module(module_idx, err) => { VMVerificationStatus::Module(*module_idx, err.into()) } + VerificationStatus::Dependency(dependency_id, err) => { + VMVerificationStatus::Dependency(dependency_id.clone(), err.into()) + } } } } @@ -745,8 +752,11 @@ impl From<&VMErrorKind> for VMStatus { VMErrorKind::ValueSerializerError => ExecutionStatus::ValueSerializationError, VMErrorKind::ValueDeserializerError => ExecutionStatus::ValueDeserializationError, VMErrorKind::DuplicateModuleName => ExecutionStatus::DuplicateModuleName, + // The below errors already have top-level VMStatus variants associated with them, so + // return those. VMErrorKind::CodeSerializerError(err) => return VMStatus::from(err), VMErrorKind::CodeDeserializerError(err) => return VMStatus::from(err), + VMErrorKind::Verification(statuses) => return statuses.iter().collect(), }; VMStatus::Execution(err) } diff --git a/language/vm/vm_runtime/Cargo.toml b/language/vm/vm_runtime/Cargo.toml index 5edd67983084..9e5a22ab29e3 100644 --- a/language/vm/vm_runtime/Cargo.toml +++ b/language/vm/vm_runtime/Cargo.toml @@ -27,6 +27,7 @@ config = { path = "../../../config"} logger = { path = "../../../common/logger" } [dev-dependencies] +assert_matches = "1.3.0" compiler = { path = "../../compiler"} [dependencies.prometheus] diff --git a/language/vm/vm_runtime/src/code_cache/module_cache.rs b/language/vm/vm_runtime/src/code_cache/module_cache.rs index df94b6c0a46e..d74159b3e40e 100644 --- a/language/vm/vm_runtime/src/code_cache/module_cache.rs +++ b/language/vm/vm_runtime/src/code_cache/module_cache.rs @@ -12,6 +12,7 @@ use crate::{ types::Type, }, }; +use bytecode_verifier::VerifiedModule; use std::marker::PhantomData; use types::language_storage::ModuleId; use vm::{ @@ -32,25 +33,30 @@ mod module_cache_tests; /// Trait that describe a cache for modules. The idea is that this trait will in charge of /// loading resolving all dependencies of needed module from the storage. pub trait ModuleCache<'alloc> { - /// Given a function handle index, resolve that handle into an internal representation of - /// move function. Return value can be one of the three following cases: - /// 1. `Ok(Some(FunctionRef))` if such function exists. - /// 2. `Ok(None)` if such function doesn't exists. - /// 3. `Err` if the module we are referring to has some internal consistency error + /// Given a function handle index, resolves that handle into an internal representation of + /// move function. + /// + /// Returns: + /// + /// * `Ok(Ok(Some(FunctionRef)))` if such function exists. + /// * `Ok(Ok(None))` if such function doesn't exists. + /// * `Ok(Err(...))` or `Err(...)` for a verification issue in a resolved dependency. + /// * `Err(...)` for a VM invariant violation. fn resolve_function_ref( &self, caller_module: &LoadedModule, idx: FunctionHandleIndex, - ) -> Result>, VMInvariantViolation>; + ) -> VMResult>>; /// Resolve a StructDefinitionIndex into a StructDef. This process will be recursive so we may - /// charge gas on each recursive step. Return value can be one of the following cases: - /// 1. `Ok(Some(StructDef))` if such struct exists. - /// 2. `Ok(None)` if such function doesn't exists. - /// 4. `Err(VMInvariantViolation)` if the module we are referring to has some internal - /// consistency error - /// 5. `Err(LinkerError)` if some fields contains an unknown struct. - /// 6. `Err(OutOfGas)` if the recursive resolution is costing too much gas + /// charge gas on each recursive step. + /// + /// Returns: + /// + /// * `Ok(Ok(Some(StructDef)))` if such struct exists. + /// * `Ok(Ok(None))` if such function doesn't exists. + /// * `Ok(Err(...))` for a verification or other issue in a resolved dependency, or out of gas. + /// * `Err(...)` for a VM invariant violation. fn resolve_struct_def( &self, module: &LoadedModule, @@ -58,15 +64,15 @@ pub trait ModuleCache<'alloc> { gas_meter: &GasMeter, ) -> VMResult>; - /// Resolve a ModuleId into a LoadedModule if the module has been cached already. Return value - /// can be one of the three following cases: - /// 1. `Ok(Some(LoadedModule))` if such module exists. - /// 2. `Ok(None)` if such module doesn't exists. - /// 3. `Err` if the module we are referring to has some internal consistency error - fn get_loaded_module( - &self, - id: &ModuleId, - ) -> Result, VMInvariantViolation>; + /// Resolve a ModuleId into a LoadedModule if the module has been cached already. + /// + /// Returns: + /// + /// * `Ok(Ok(Some(LoadedModule)))` if such module exists. + /// * `Ok(Ok(None))` if such module doesn't exists. + /// * `Ok(Err(...))` for a verification issue in the module. + /// * `Err(...)` for a VM invariant violation. + fn get_loaded_module(&self, id: &ModuleId) -> VMResult>; fn cache_module(&self, module: CompiledModule); @@ -84,7 +90,7 @@ where &self, caller_module: &LoadedModule, idx: FunctionHandleIndex, - ) -> Result>, VMInvariantViolation> { + ) -> VMResult>> { (*self).resolve_function_ref(caller_module, idx) } @@ -97,10 +103,7 @@ where (*self).resolve_struct_def(module, idx, gas_meter) } - fn get_loaded_module( - &self, - id: &ModuleId, - ) -> Result, VMInvariantViolation> { + fn get_loaded_module(&self, id: &ModuleId) -> VMResult> { (*self).get_loaded_module(id) } @@ -138,17 +141,36 @@ impl<'alloc> VMModuleCache<'alloc> { &self, id: &ModuleId, fetcher: &F, - ) -> Result, VMInvariantViolation> { + ) -> VMRuntimeResult> { // Currently it is still possible for a script to invoke a nonsense module id function. // However, once we have the verifier that checks the well-formedness of the all the linked // module id, we should get rid of that ok_or case here. if let Some(m) = self.map.get(id) { return Ok(Some(&*m)); } - Ok(fetcher - .get_module(id) - .map(LoadedModule::new) - .map(|m| self.map.or_insert(id.clone(), m))) + let module = match fetcher.get_module(id) { + Some(module) => module, + None => return Ok(None), + }; + + // Verify the module before using it. + let module = match VerifiedModule::new(module) { + Ok(module) => module.into_inner(), + Err((_, errors)) => { + return Err(VMRuntimeError { + loc: Location::new(), + err: VMErrorKind::Verification( + errors + .into_iter() + .map(|error| VerificationStatus::Dependency(id.clone(), error)) + .collect(), + ), + }) + } + }; + + let loaded_module = LoadedModule::new(module); + Ok(Some(self.map.or_insert(id.clone(), loaded_module))) } #[cfg(test)] @@ -170,25 +192,25 @@ impl<'alloc> VMModuleCache<'alloc> { caller_module: &LoadedModule, idx: FunctionHandleIndex, fetcher: &F, - ) -> Result>, VMInvariantViolation> + ) -> VMResult>> where F: ModuleFetcher, { let function_handle = caller_module.function_handle_at(idx); let callee_name = caller_module.string_at(function_handle.name); let callee_module_id = FunctionHandleView::new(caller_module, function_handle).module_id(); - self.get_loaded_module_with_fetcher(&callee_module_id, fetcher) - .and_then(|callee_module_opt| { - if let Some(callee_module) = callee_module_opt { - let callee_func_id = callee_module - .function_defs_table - .get(callee_name) - .ok_or(VMInvariantViolation::LinkerError)?; - Ok(Some(FunctionRef::new(callee_module, *callee_func_id))) - } else { - Ok(None) - } - }) + + match self.get_loaded_module_with_fetcher(&callee_module_id, fetcher) { + Ok(Some(callee_module)) => { + let callee_func_id = callee_module + .function_defs_table + .get(callee_name) + .ok_or(VMInvariantViolation::LinkerError)?; + Ok(Ok(Some(FunctionRef::new(callee_module, *callee_func_id)))) + } + Ok(None) => Ok(Ok(None)), + Err(errors) => Ok(Err(errors)), + } } /// Resolve a StructHandle into a StructDef recursively in either the cache or the `fetcher`. @@ -202,15 +224,16 @@ impl<'alloc> VMModuleCache<'alloc> { let struct_handle = module.struct_handle_at(idx); let struct_name = module.string_at(struct_handle.name); let struct_def_module_id = StructHandleView::new(module, struct_handle).module_id(); - let defined_module = self.get_loaded_module_with_fetcher(&struct_def_module_id, fetcher)?; - if let Some(m) = defined_module { - let struct_def_idx = m - .struct_defs_table - .get(struct_name) - .ok_or(VMInvariantViolation::LinkerError)?; - self.resolve_struct_def_with_fetcher(m, *struct_def_idx, gas_meter, fetcher) - } else { - Ok(Ok(None)) + match self.get_loaded_module_with_fetcher(&struct_def_module_id, fetcher) { + Ok(Some(module)) => { + let struct_def_idx = module + .struct_defs_table + .get(struct_name) + .ok_or(VMInvariantViolation::LinkerError)?; + self.resolve_struct_def_with_fetcher(module, *struct_def_idx, gas_meter, fetcher) + } + Ok(None) => Ok(Ok(None)), + Err(errors) => Ok(Err(errors)), } } @@ -292,7 +315,7 @@ impl<'alloc> ModuleCache<'alloc> for VMModuleCache<'alloc> { &self, caller_module: &LoadedModule, idx: FunctionHandleIndex, - ) -> Result>, VMInvariantViolation> { + ) -> VMResult>> { self.resolve_function_ref_with_fetcher(caller_module, idx, &NullFetcher()) } @@ -305,14 +328,11 @@ impl<'alloc> ModuleCache<'alloc> for VMModuleCache<'alloc> { self.resolve_struct_def_with_fetcher(module, idx, gas_meter, &NullFetcher()) } - fn get_loaded_module( - &self, - id: &ModuleId, - ) -> Result, VMInvariantViolation> { + fn get_loaded_module(&self, id: &ModuleId) -> VMResult> { // Currently it is still possible for a script to invoke a nonsense module id function. // However, once we have the verifier that checks the well-formedness of the all the linked // module id, we should get rid of that ok_or case here. - Ok(self.map.get(id)) + Ok(Ok(self.map.get(id))) } fn cache_module(&self, module: CompiledModule) { @@ -362,7 +382,7 @@ impl<'alloc, 'blk, F: ModuleFetcher> ModuleCache<'alloc> for BlockModuleCache<'a &self, caller_module: &LoadedModule, idx: FunctionHandleIndex, - ) -> Result>, VMInvariantViolation> { + ) -> VMResult>> { self.vm_cache .resolve_function_ref_with_fetcher(caller_module, idx, &self.storage) } @@ -377,12 +397,10 @@ impl<'alloc, 'blk, F: ModuleFetcher> ModuleCache<'alloc> for BlockModuleCache<'a .resolve_struct_def_with_fetcher(module, idx, gas_meter, &self.storage) } - fn get_loaded_module( - &self, - id: &ModuleId, - ) -> Result, VMInvariantViolation> { - self.vm_cache - .get_loaded_module_with_fetcher(id, &self.storage) + fn get_loaded_module(&self, id: &ModuleId) -> VMResult> { + Ok(self + .vm_cache + .get_loaded_module_with_fetcher(id, &self.storage)) } fn cache_module(&self, module: CompiledModule) { @@ -432,9 +450,9 @@ where &self, caller_module: &LoadedModule, idx: FunctionHandleIndex, - ) -> Result>, VMInvariantViolation> { - if let Some(f) = self.local_cache.resolve_function_ref(caller_module, idx)? { - Ok(Some(f)) + ) -> VMResult>> { + if let Some(f) = try_runtime!(self.local_cache.resolve_function_ref(caller_module, idx)) { + Ok(Ok(Some(f))) } else { self.block_cache.resolve_function_ref(caller_module, idx) } @@ -453,12 +471,9 @@ where } } - fn get_loaded_module( - &self, - id: &ModuleId, - ) -> Result, VMInvariantViolation> { - if let Some(m) = self.local_cache.get_loaded_module(id)? { - Ok(Some(m)) + fn get_loaded_module(&self, id: &ModuleId) -> VMResult> { + if let Some(m) = try_runtime!(self.local_cache.get_loaded_module(id)) { + Ok(Ok(Some(m))) } else { self.block_cache.get_loaded_module(id) } diff --git a/language/vm/vm_runtime/src/gas_meter.rs b/language/vm/vm_runtime/src/gas_meter.rs index d08888064c78..df7f582eb1f9 100644 --- a/language/vm/vm_runtime/src/gas_meter.rs +++ b/language/vm/vm_runtime/src/gas_meter.rs @@ -187,10 +187,10 @@ impl GasMeter { } Bytecode::Call(call_idx) => { let self_module = &stk.top_frame()?.module(); - let function_ref = stk + let function_ref = try_runtime!(stk .module_cache - .resolve_function_ref(self_module, *call_idx)? - .ok_or(VMInvariantViolation::LinkerError)?; + .resolve_function_ref(self_module, *call_idx)) + .ok_or(VMInvariantViolation::LinkerError)?; if function_ref.is_native() { 0 // This will be costed at the call site/by the native function } else { diff --git a/language/vm/vm_runtime/src/process_txn/execute.rs b/language/vm/vm_runtime/src/process_txn/execute.rs index 5b77d29fcee2..7dd3383dac73 100644 --- a/language/vm/vm_runtime/src/process_txn/execute.rs +++ b/language/vm/vm_runtime/src/process_txn/execute.rs @@ -84,15 +84,17 @@ where // will read through the cache to fetch the module from the global storage // if it is not already cached. match txn_executor.module_cache().get_loaded_module(&module_id) { - Ok(None) => (), // No module with this name exists. safe to publish one - Ok(Some(_)) => { - // A module with this name already exists. It is not safe to publish - // another one; it would clobber the old module. This would break - // code that links against the module and make published resources - // from the old module inaccessible (or worse, accessible and not + Ok(Ok(None)) => (), // No module with this name exists. safe to publish one + Ok(Ok(Some(_))) | Ok(Err(_)) => { + // A module with this name already exists (the error case is when the module + // couldn't be verified, but it still exists so we should fail similarly). + // It is not safe to publish another one; it would clobber the old module. + // This would break code that links against the module and make published + // resources from the old module inaccessible (or worse, accessible and not // typesafe). - // We are currently developing a versioning scheme for safe updates - // of modules and resources. + // + // We are currently developing a versioning scheme for safe updates of + // modules and resources. warn!("[VM] VM error duplicate module {:?}", module_id); return txn_executor.failed_transaction_cleanup(Ok(Err(VMRuntimeError { loc: Location::default(), @@ -101,7 +103,7 @@ where } Err(err) => { error!( - "[VM] VM internal error verifying module {:?}: {:?}", + "[VM] VM internal error while checking for duplicate module {:?}: {:?}", module_id, err ); return ExecutedTransaction::discard_error_output(&err); diff --git a/language/vm/vm_runtime/src/txn_executor.rs b/language/vm/vm_runtime/src/txn_executor.rs index c93235e7bbc5..e781baa0fc2e 100644 --- a/language/vm/vm_runtime/src/txn_executor.rs +++ b/language/vm/vm_runtime/src/txn_executor.rs @@ -240,11 +240,11 @@ where } Bytecode::Call(idx) => { let self_module = &self.execution_stack.top_frame()?.module(); - let callee_function_ref = self + let callee_function_ref = try_runtime!(self .execution_stack .module_cache - .resolve_function_ref(self_module, idx)? - .ok_or(VMInvariantViolation::LinkerError)?; + .resolve_function_ref(self_module, idx)) + .ok_or(VMInvariantViolation::LinkerError)?; if callee_function_ref.is_native() { let module_name: &str = callee_function_ref.module().name(); @@ -609,11 +609,11 @@ where /// Create an account on the blockchain by calling into `CREATE_ACCOUNT_NAME` function stored /// in the `ACCOUNT_MODULE` on chain. pub fn create_account(&mut self, addr: AccountAddress) -> VMResult<()> { - let account_module = self + let account_module = try_runtime!(self .execution_stack .module_cache - .get_loaded_module(&ACCOUNT_MODULE)? - .ok_or(VMInvariantViolation::LinkerError)?; + .get_loaded_module(&ACCOUNT_MODULE)) + .ok_or(VMInvariantViolation::LinkerError)?; // Address will be used as the initial authentication key. try_runtime!(self.execute_function( @@ -747,11 +747,11 @@ where function_name: &str, args: Vec, ) -> VMResult<()> { - let loaded_module = self - .execution_stack - .module_cache - .get_loaded_module(module)? - .ok_or(VMInvariantViolation::LinkerError)?; + let loaded_module = + match try_runtime!(self.execution_stack.module_cache.get_loaded_module(module)) { + Some(module) => module, + None => return Err(VMInvariantViolation::LinkerError), + }; let func_idx = loaded_module .function_defs_table .get(function_name) diff --git a/language/vm/vm_runtime/src/unit_tests/module_cache_tests.rs b/language/vm/vm_runtime/src/unit_tests/module_cache_tests.rs index 6e616964897f..b4b1a963cdd7 100644 --- a/language/vm/vm_runtime/src/unit_tests/module_cache_tests.rs +++ b/language/vm/vm_runtime/src/unit_tests/module_cache_tests.rs @@ -7,6 +7,7 @@ use crate::{ loaded_data::function::{FunctionRef, FunctionReference}, }; use ::compiler::{compiler, parser::parse_program}; +use assert_matches::assert_matches; use hex; use types::account_address::AccountAddress; use vm::file_format::*; @@ -41,7 +42,7 @@ fn test_module(name: String) -> CompiledModule { code: CodeUnit { max_stack_size: 10, locals: LocalsSignatureIndex::new(0), - code: vec![Bytecode::Add], + code: vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret], }, }, FunctionDefinition { @@ -144,16 +145,22 @@ fn test_loader_one_module() { let allocator = Arena::new(); let loaded_program = VMModuleCache::new(&allocator); loaded_program.cache_module(module); - let module_ref = loaded_program.get_loaded_module(&mod_id).unwrap().unwrap(); + let module_ref = loaded_program + .get_loaded_module(&mod_id) + .unwrap() + .unwrap() + .unwrap(); // Get the function reference of the first two function handles. let func1_ref = loaded_program .resolve_function_ref(module_ref, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); let func2_ref = loaded_program .resolve_function_ref(module_ref, FunctionHandleIndex::new(1)) .unwrap() + .unwrap() .unwrap(); // The two references should refer to the same module @@ -164,7 +171,10 @@ fn test_loader_one_module() { assert_eq!(func1_ref.arg_count(), 0); assert_eq!(func1_ref.return_count(), 0); - assert_eq!(func1_ref.code_definition(), vec![Bytecode::Add].as_slice()); + assert_eq!( + func1_ref.code_definition(), + vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret].as_slice() + ); assert_eq!(func2_ref.arg_count(), 1); assert_eq!(func2_ref.return_count(), 0); @@ -187,10 +197,12 @@ fn test_loader_cross_modules() { let func1 = loaded_program .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); let func2 = loaded_program .resolve_function_ref(entry_module, FunctionHandleIndex::new(1)) .unwrap() + .unwrap() .unwrap(); assert_eq!( @@ -200,7 +212,10 @@ fn test_loader_cross_modules() { assert_eq!(func1.arg_count(), 0); assert_eq!(func1.return_count(), 0); - assert_eq!(func1.code_definition(), vec![Bytecode::Add].as_slice()); + assert_eq!( + func1.code_definition(), + vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret].as_slice() + ); assert_eq!(func2.arg_count(), 1); assert_eq!(func2.return_count(), 0); @@ -222,6 +237,7 @@ fn test_cache_with_storage() { assert!(vm_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .is_none()); { @@ -232,10 +248,12 @@ fn test_cache_with_storage() { let func1 = block_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); let func2 = block_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(1)) .unwrap() + .unwrap() .unwrap(); assert_eq!( @@ -245,7 +263,10 @@ fn test_cache_with_storage() { assert_eq!(func1.arg_count(), 0); assert_eq!(func1.return_count(), 0); - assert_eq!(func1.code_definition(), vec![Bytecode::Add].as_slice()); + assert_eq!( + func1.code_definition(), + vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret].as_slice() + ); assert_eq!(func2.arg_count(), 1); assert_eq!(func2.return_count(), 0); @@ -257,10 +278,12 @@ fn test_cache_with_storage() { let func1 = block_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); let func2 = block_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(1)) .unwrap() + .unwrap() .unwrap(); assert_eq!( @@ -270,7 +293,10 @@ fn test_cache_with_storage() { assert_eq!(func1.arg_count(), 0); assert_eq!(func1.return_count(), 0); - assert_eq!(func1.code_definition(), vec![Bytecode::Add].as_slice()); + assert_eq!( + func1.code_definition(), + vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret].as_slice() + ); assert_eq!(func2.arg_count(), 1); assert_eq!(func2.return_count(), 0); @@ -282,10 +308,12 @@ fn test_cache_with_storage() { let func1 = vm_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); let func2 = vm_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(1)) .unwrap() + .unwrap() .unwrap(); assert_eq!( @@ -295,7 +323,10 @@ fn test_cache_with_storage() { assert_eq!(func1.arg_count(), 0); assert_eq!(func1.return_count(), 0); - assert_eq!(func1.code_definition(), vec![Bytecode::Add].as_slice()); + assert_eq!( + func1.code_definition(), + vec![Bytecode::LdTrue, Bytecode::Pop, Bytecode::Ret].as_slice() + ); assert_eq!(func2.arg_count(), 1); assert_eq!(func2.return_count(), 0); @@ -407,10 +438,12 @@ fn test_multi_level_cache_write_back() { assert!(vm_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .is_none()); let func2_txn_ref = txn_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); assert_eq!(func2_txn_ref.arg_count(), 1); assert_eq!(func2_txn_ref.return_count(), 0); @@ -428,6 +461,7 @@ fn test_multi_level_cache_write_back() { let func2_ref = vm_cache .resolve_function_ref(entry_module, FunctionHandleIndex::new(0)) .unwrap() + .unwrap() .unwrap(); assert_eq!(func2_ref.arg_count(), 1); assert_eq!(func2_ref.return_count(), 0); @@ -464,7 +498,11 @@ fn test_same_module_struct_resolution() { let block_cache = BlockModuleCache::new(&vm_cache, fetcher); { let module_id = ModuleId::new(AccountAddress::default(), "M1".to_string()); - let module_ref = block_cache.get_loaded_module(&module_id).unwrap().unwrap(); + let module_ref = block_cache + .get_loaded_module(&module_id) + .unwrap() + .unwrap() + .unwrap(); let gas = GasMeter::new(100_000_000); let struct_x = block_cache .resolve_struct_def(module_ref, StructDefinitionIndex::new(0), &gas) @@ -515,6 +553,7 @@ fn test_multi_module_struct_resolution() { let module2_ref = block_cache .get_loaded_module(&module_id_2) .unwrap() + .unwrap() .unwrap(); let gas = GasMeter::new(100_000_000); @@ -552,7 +591,11 @@ fn test_field_offset_resolution() { let block_cache = BlockModuleCache::new(&vm_cache, fetcher); { let module_id = ModuleId::new(AccountAddress::default(), "M1".to_string()); - let module_ref = block_cache.get_loaded_module(&module_id).unwrap().unwrap(); + let module_ref = block_cache + .get_loaded_module(&module_id) + .unwrap() + .unwrap() + .unwrap(); let f_idx = module_ref.field_defs_table.get("f").unwrap(); assert_eq!(module_ref.get_field_offset(*f_idx).unwrap(), 0); @@ -570,3 +613,51 @@ fn test_field_offset_resolution() { assert_eq!(module_ref.get_field_offset(*y_idx).unwrap(), 2); } } + +#[test] +fn test_dependency_fails_verification() { + let allocator = Arena::new(); + let vm_cache = VMModuleCache::new(&allocator); + + // This module has a struct inside a resource, which should fail verification. But assume that + // it made its way onto the chain somehow (e.g. there was a bug in an older version of the + // bytecode verifier). + let code = " + modules: + module Test { + resource R1 { } + struct S1 { r1: R#Self.R1 } + + public new_S1(): V#Self.S1 { + let s: V#Self.S1; + let r: R#Self.R1; + r = R1 {}; + s = S1 { r1: move(r) }; + return move(s); + } + } + + script: + main() { + } + "; + + let module = parse_and_compile_modules(code); + let fetcher = FakeFetcher::new(module); + let block_cache = BlockModuleCache::new(&vm_cache, fetcher); + + let module_id = ModuleId::new(AccountAddress::default(), "Test".to_string()); + let VMRuntimeError { err, .. } = block_cache + .get_loaded_module(&module_id) + .unwrap() + .unwrap_err(); + let errors = match err { + VMErrorKind::Verification(errors) => errors, + other => panic!("Unexpected error: {:?}", other), + }; + assert_matches!( + &errors[0], + VerificationStatus::Dependency(module_id, _) + if module_id.address() == &AccountAddress::default() && module_id.name() == "Test" + ); +} diff --git a/language/vm/vm_runtime/src/unit_tests/runtime_tests.rs b/language/vm/vm_runtime/src/unit_tests/runtime_tests.rs index 0005afb75fec..0d8c04c62a3d 100644 --- a/language/vm/vm_runtime/src/unit_tests/runtime_tests.rs +++ b/language/vm/vm_runtime/src/unit_tests/runtime_tests.rs @@ -634,11 +634,16 @@ fn test_call() { let allocator = Arena::new(); let module_cache = VMModuleCache::new_from_module(module, &allocator).unwrap(); let fake_func = { - let fake_mod_entry = module_cache.get_loaded_module(&mod_id).unwrap().unwrap(); + let fake_mod_entry = module_cache + .get_loaded_module(&mod_id) + .unwrap() + .unwrap() + .unwrap(); module_cache .resolve_function_ref(fake_mod_entry, FunctionHandleIndex::new(0)) .unwrap() .unwrap() + .unwrap() }; let data_cache = FakeDataCache::new(); let mut vm = diff --git a/language/vm/vm_runtime/vm_runtime_tests/src/data_store.rs b/language/vm/vm_runtime/vm_runtime_tests/src/data_store.rs index 470ec10fc943..27be3ae28cbb 100644 --- a/language/vm/vm_runtime/vm_runtime_tests/src/data_store.rs +++ b/language/vm/vm_runtime/vm_runtime_tests/src/data_store.rs @@ -12,10 +12,11 @@ use state_view::StateView; use std::{collections::HashMap, fs::File, io::prelude::*, path::PathBuf}; use types::{ access_path::AccessPath, + language_storage::ModuleId, transaction::{SignedTransaction, TransactionPayload}, write_set::{WriteOp, WriteSet}, }; -use vm::errors::*; +use vm::{errors::*, CompiledModule}; use vm_runtime::data_cache::RemoteCache; lazy_static! { @@ -89,6 +90,18 @@ impl FakeDataStore { None => panic!("can't create Account data"), } } + + /// Adds a [`CompiledModule`] to this data store. + /// + /// Does not do any sort of verification on the module. + pub fn add_module(&mut self, module_id: &ModuleId, module: &CompiledModule) { + let access_path = AccessPath::from(module_id); + let mut blob = vec![]; + module + .serialize(&mut blob) + .expect("serializing this module should work"); + self.set(access_path, blob); + } } // This is used by the `execute_block` API. diff --git a/language/vm/vm_runtime/vm_runtime_tests/src/executor.rs b/language/vm/vm_runtime/vm_runtime_tests/src/executor.rs index a59785f9901f..8edb17e07262 100644 --- a/language/vm/vm_runtime/vm_runtime_tests/src/executor.rs +++ b/language/vm/vm_runtime/vm_runtime_tests/src/executor.rs @@ -11,10 +11,12 @@ use config::config::{NodeConfig, NodeConfigHelpers, VMPublishingOption}; use state_view::StateView; use types::{ access_path::AccessPath, + language_storage::ModuleId, transaction::{SignedTransaction, TransactionOutput}, vm_error::VMStatus, write_set::WriteSet, }; +use vm::CompiledModule; use vm_runtime::{ loaded_data::{struct_def::StructDef, types::Type}, value::Value, @@ -92,6 +94,13 @@ impl FakeExecutor { self.data_store.add_account_data(account_data) } + /// Adds a module to this executor's data store. + /// + /// Does not do any sort of verification on the module. + pub fn add_module(&mut self, module_id: &ModuleId, module: &CompiledModule) { + self.data_store.add_module(module_id, module) + } + /// Reads the resource [`Value`] for an account from this executor's data store. pub fn read_account_resource(&self, account: &Account) -> Option { let ap = account.make_access_path(); diff --git a/language/vm/vm_runtime/vm_runtime_tests/src/tests/verify_txn.rs b/language/vm/vm_runtime/vm_runtime_tests/src/tests/verify_txn.rs index edfa3d409377..8690f699c03f 100644 --- a/language/vm/vm_runtime/vm_runtime_tests/src/tests/verify_txn.rs +++ b/language/vm/vm_runtime/vm_runtime_tests/src/tests/verify_txn.rs @@ -5,7 +5,7 @@ use crate::{ account::AccountData, assert_prologue_disparity, assert_prologue_parity, common_transactions::*, - compile::{compile_program_with_address, compile_script}, + compile::{compile_program_with_address, compile_script, Compiler}, executor::FakeExecutor, }; use assert_matches::assert_matches; @@ -14,6 +14,7 @@ use crypto::signing::KeyPair; use std::collections::HashSet; use tiny_keccak::Keccak; use types::{ + account_address::AccountAddress, test_helpers::transaction_test_helpers, transaction::{ TransactionArgument, TransactionStatus, MAX_TRANSACTION_SIZE_IN_BYTES, SCRIPT_HASH_LENGTH, @@ -551,3 +552,72 @@ pub fn test_open_publishing() { &TransactionStatus::Keep(VMStatus::Execution(ExecutionStatus::Executed)) ); } + +#[test] +fn test_dependency_fails_verification() { + let mut executor = FakeExecutor::from_genesis_with_options(VMPublishingOption::Open); + + // Get a module that fails verification into the store. + let bad_module_code = " + modules: + module Test { + resource R1 { } + struct S1 { r1: R#Self.R1 } + + public new_S1(): V#Self.S1 { + let s: V#Self.S1; + let r: R#Self.R1; + r = R1 {}; + s = S1 { r1: move(r) }; + return move(s); + } + } + + script: + main() { + } + "; + let compiler = Compiler { + code: bad_module_code, + ..Compiler::default() + }; + let mut modules = compiler.into_compiled_program().modules; + let module = modules.swap_remove(0); + executor.add_module(&module.self_id(), &module); + + // Create a transaction that tries to use that module. + let sender = AccountData::new(1_000_000, 10); + executor.add_account_data(&sender); + + let code = " + import 0x0.Test; + + main() { + let x: V#Test.S1; + x = Test.new_S1(); + return; + } + "; + + let compiler = Compiler { + code, + address: *sender.address(), + extra_deps: vec![module], + ..Compiler::default() + }; + let program = compiler.into_program(vec![]); + let txn = sender + .account() + .create_signed_txn_impl(*sender.address(), program, 10, 10_000, 1); + // As of now, we don't verify dependencies in verify_transaction. + assert_eq!(executor.verify_transaction(txn.clone()), None); + let errors = match executor.execute_transaction(txn).status() { + TransactionStatus::Discard(VMStatus::Verification(errors)) => errors.to_vec(), + other => panic!("Unexpected status: {:?}", other), + }; + assert_matches!( + &errors[0], + VMVerificationStatus::Dependency(module_id, _) + if module_id.address() == &AccountAddress::default() && module_id.name() == "Test" + ); +} diff --git a/types/Cargo.toml b/types/Cargo.toml index 5b8067b3b4ac..02ba89dc1035 100644 --- a/types/Cargo.toml +++ b/types/Cargo.toml @@ -26,6 +26,7 @@ tiny-keccak = "1.4.2" canonical_serialization = { path = "../common/canonical_serialization"} crypto = { path = "../crypto/legacy_crypto" } failure = { path = "../common/failure_ext", package = "failure_ext" } +proptest_helpers = { path = "../common/proptest_helpers" } proto_conv = { path = "../common/proto_conv", features = ["derive"] } [build-dependencies] diff --git a/types/src/proto/vm_errors.proto b/types/src/proto/vm_errors.proto index b29d32e00420..767d80ba2405 100644 --- a/types/src/proto/vm_errors.proto +++ b/types/src/proto/vm_errors.proto @@ -5,6 +5,8 @@ syntax = "proto3"; package types; +import "language_storage.proto"; + // The statuses and errors produced by the VM can be categorized into a // couple different types: // 1. Validation Statuses: all the errors that can (/should) be @@ -83,12 +85,15 @@ message VMVerificationStatus { enum StatusKind { SCRIPT = 0; MODULE = 1; + DEPENDENCY = 2; } StatusKind status_kind = 1; - // For StatusKind::SCRIPT this is ignored. + // For StatusKind::SCRIPT and DEPENDENCY this is ignored. uint32 module_idx = 2; VMVerificationErrorKind error_kind = 3; string message = 4; + // For StatusKind::SCRIPT and MODULE this is ignored. + ModuleId dependency_id = 5; } // When a code module/script is published it is verified. These are the diff --git a/types/src/unit_tests/vm_error_proto_conversion_test.rs b/types/src/unit_tests/vm_error_proto_conversion_test.rs index 1c41761f292d..d8826883d206 100644 --- a/types/src/unit_tests/vm_error_proto_conversion_test.rs +++ b/types/src/unit_tests/vm_error_proto_conversion_test.rs @@ -7,6 +7,7 @@ use crate::vm_error::{ VMVerificationStatus, }; use proptest::prelude::*; +use proptest_helpers::with_stack_size; use proto_conv::test_helper::assert_protobuf_encode_decode; proptest! { @@ -49,9 +50,14 @@ proptest! { fn execution_status_roundtrip(execution_status in any::()) { assert_protobuf_encode_decode(&execution_status); } +} - #[test] - fn test_vm_status(vm_status in any::()) { - assert_protobuf_encode_decode(&vm_status); - } +#[test] +fn test_vm_status_roundtrip() { + with_stack_size(4 * 1024 * 1024, || { + proptest!(|(vm_status in any::())| { + assert_protobuf_encode_decode(&vm_status); + }) + }) + .unwrap(); } diff --git a/types/src/vm_error.rs b/types/src/vm_error.rs index 064252db028d..4529fe86f919 100644 --- a/types/src/vm_error.rs +++ b/types/src/vm_error.rs @@ -3,6 +3,7 @@ #![allow(clippy::unit_arg)] +use crate::language_storage::ModuleId; use failure::prelude::*; use proptest::{collection::vec, prelude::*}; use proptest_derive::Arbitrary; @@ -112,6 +113,8 @@ pub enum VMVerificationStatus { /// Verification error in a module -- the first element is the index of the module with the /// error. Module(u16, VMVerificationError), + /// Verification error in a dependent module. + Dependency(ModuleId, VMVerificationError), } #[derive(Arbitrary, Clone, Copy, PartialEq, Eq, Debug, Hash)] @@ -743,6 +746,11 @@ impl IntoProto for VMVerificationStatus { proto_status.set_module_idx(u32::from(module_idx)); error.into_proto() } + VMVerificationStatus::Dependency(dependency_id, error) => { + proto_status.set_status_kind(ProtoStatusKind::DEPENDENCY); + proto_status.set_dependency_id(dependency_id.into_proto()); + error.into_proto() + } }; proto_status.set_error_kind(kind); proto_status.set_message(message); @@ -770,6 +778,10 @@ impl FromProto for VMVerificationStatus { } Ok(VMVerificationStatus::Module(module_idx as u16, err)) } + ProtoStatusKind::DEPENDENCY => { + let dependency_id = ModuleId::from_proto(proto_status.take_dependency_id())?; + Ok(VMVerificationStatus::Dependency(dependency_id, err)) + } } } }