From 8070ea9ba8b8c28dda57ed88c1d715b39d355b8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rain=20=E2=81=A3?= Date: Mon, 24 Jun 2019 16:45:31 -0700 Subject: [PATCH] [vm_runtime] verify modules while loading them from the store Modules loaded from the store weren't being verified previously. This diff allows that to happen. The work involves threading verification errors through to the protobuf error messages. This is a blocker to making `LoadedModule` use `VerifiedModule`. This found an interesting bug in one of our tests, where a test module we had defined was failing to verify. --- common/proptest_helpers/Cargo.toml | 1 + common/proptest_helpers/src/lib.rs | 32 ++++ execution/execution_proto/Cargo.toml | 1 + .../src/protobuf_conversion_test.rs | 16 +- .../vm/cost_synthesis/src/stack_generator.rs | 6 +- language/vm/cost_synthesis/src/vm_runner.rs | 3 +- language/vm/src/errors.rs | 10 ++ language/vm/vm_runtime/Cargo.toml | 1 + .../vm_runtime/src/code_cache/module_cache.rs | 167 ++++++++++-------- language/vm/vm_runtime/src/gas_meter.rs | 6 +- .../vm/vm_runtime/src/process_txn/execute.rs | 20 ++- language/vm/vm_runtime/src/txn_executor.rs | 22 +-- .../src/unit_tests/module_cache_tests.rs | 109 +++++++++++- .../src/unit_tests/runtime_tests.rs | 7 +- .../vm_runtime_tests/src/data_store.rs | 15 +- .../vm_runtime_tests/src/executor.rs | 9 + .../vm_runtime_tests/src/tests/verify_txn.rs | 72 +++++++- types/Cargo.toml | 1 + types/src/proto/vm_errors.proto | 7 +- .../vm_error_proto_conversion_test.rs | 14 +- types/src/vm_error.rs | 12 ++ 21 files changed, 407 insertions(+), 124 deletions(-) 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)) + } } } }