diff --git a/Cargo.lock b/Cargo.lock index 6f896a99c8969..bd75ac9b32357 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5323,6 +5323,7 @@ dependencies = [ "sp-std", "wasm-instrument", "wasmi 0.18.1", + "wasmparser-nostd", "wat", ] diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 59a91827e40f8..fa67c86a976d6 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -26,6 +26,7 @@ smallvec = { version = "1", default-features = false, features = [ "const_generics", ] } wasmi = { version = "0.18", default-features = false } +wasmparser = { package = "wasmparser-nostd", version = "0.91" } impl-trait-for-tuples = "0.2" # Only used in benchmarking to generate random contract code diff --git a/frame/contracts/fixtures/invalid_contract.wat b/frame/contracts/fixtures/invalid_contract.wat new file mode 100644 index 0000000000000..085569000c559 --- /dev/null +++ b/frame/contracts/fixtures/invalid_contract.wat @@ -0,0 +1,4 @@ +;; Valid module but missing the call function +(module + (func (export "deploy")) +) diff --git a/frame/contracts/fixtures/invalid_import.wat b/frame/contracts/fixtures/invalid_import.wat deleted file mode 100644 index 011f1a40e76d7..0000000000000 --- a/frame/contracts/fixtures/invalid_import.wat +++ /dev/null @@ -1,6 +0,0 @@ -;; A valid contract which does nothing at all but imports an invalid function -(module - (import "invalid" "invalid_88_99" (func (param i32 i32 i32))) - (func (export "deploy")) - (func (export "call")) -) diff --git a/frame/contracts/fixtures/invalid_module.wat b/frame/contracts/fixtures/invalid_module.wat new file mode 100644 index 0000000000000..e4a72f74273f9 --- /dev/null +++ b/frame/contracts/fixtures/invalid_module.wat @@ -0,0 +1,8 @@ +;; An invalid module +(module + (func (export "deploy")) + (func (export "call") + ;; imbalanced stack + (i32.const 7) + ) +) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index d48a71b85e9fe..608421af4f94f 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -102,7 +102,7 @@ use crate::{ exec::{AccountIdOf, ExecError, Executable, Stack as ExecStack}, gas::GasMeter, storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract, Storage}, - wasm::{OwnerInfo, PrefabWasmModule}, + wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate}, weights::WeightInfo, }; use codec::{Codec, Encode, HasCompact}; @@ -997,7 +997,8 @@ where ) -> CodeUploadResult, BalanceOf> { let schedule = T::Schedule::get(); let module = - PrefabWasmModule::from_code(code, &schedule, origin).map_err(|(err, _)| err)?; + PrefabWasmModule::from_code(code, &schedule, origin, TryInstantiate::Instantiate) + .map_err(|(err, _)| err)?; let deposit = module.open_deposit(); if let Some(storage_deposit_limit) = storage_deposit_limit { ensure!(storage_deposit_limit >= deposit, >::StorageDepositLimitExhausted); @@ -1115,11 +1116,17 @@ where let schedule = T::Schedule::get(); let (extra_deposit, executable) = match code { Code::Upload(binary) => { - let executable = PrefabWasmModule::from_code(binary, &schedule, origin.clone()) - .map_err(|(err, msg)| { - debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes())); - err - })?; + let executable = PrefabWasmModule::from_code( + binary, + &schedule, + origin.clone(), + TryInstantiate::Skip, + ) + .map_err(|(err, msg)| { + println!("found error"); + debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes())); + err + })?; // The open deposit will be charged during execution when the // uploaded module does not already exist. This deposit is not part of the // storage meter because it is not transferred to the contract but diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 993a089c0c176..e4e49476ca2fd 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -3546,10 +3546,31 @@ fn contract_reverted() { #[test] fn code_rejected_error_works() { - let (wasm, _) = compile_module::("invalid_import").unwrap(); ExtBuilder::default().existential_deposit(200).build().execute_with(|| { let _ = Balances::deposit_creating(&ALICE, 1_000_000); + let (wasm, _) = compile_module::("invalid_module").unwrap(); + assert_noop!( + Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm.clone(), None), + >::CodeRejected, + ); + let result = Contracts::bare_instantiate( + ALICE, + 0, + GAS_LIMIT, + None, + Code::Upload(wasm), + vec![], + vec![], + true, + ); + assert_err!(result.result, >::CodeRejected); + assert_eq!( + std::str::from_utf8(&result.debug_message).unwrap(), + "validation of new code failed" + ); + + let (wasm, _) = compile_module::("invalid_contract").unwrap(); assert_noop!( Contracts::upload_code(RuntimeOrigin::signed(ALICE), wasm.clone(), None), >::CodeRejected, @@ -3566,7 +3587,10 @@ fn code_rejected_error_works() { true, ); assert_err!(result.result, >::CodeRejected); - assert_eq!(std::str::from_utf8(&result.debug_message).unwrap(), "validation failed"); + assert_eq!( + std::str::from_utf8(&result.debug_message).unwrap(), + "call function isn't exported" + ); }); } diff --git a/frame/contracts/src/wasm/code_cache.rs b/frame/contracts/src/wasm/code_cache.rs index 8e1bbc2e1cd45..4cd72c7b12e7c 100644 --- a/frame/contracts/src/wasm/code_cache.rs +++ b/frame/contracts/src/wasm/code_cache.rs @@ -204,10 +204,8 @@ where // as the contract is already deployed and every change in size would be the result // of changes in the instrumentation algorithm controlled by the chain authors. prefab_module.code = WeakBoundedVec::force_from( - prepare::check_and_instrument::(&original_code, schedule) - .map(|(code, _)| code) - .map_err(|_| >::CodeRejected)?, - Some("Contract exceeds limit after re-instrumentation."), + prepare::reinstrument::(&original_code, schedule)?, + Some("Contract exceeds size limit after re-instrumentation."), ); prefab_module.instruction_weights_version = schedule.instruction_weights.version; >::insert(&prefab_module.code_hash, &*prefab_module); diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 0ffbdf34aa4db..c5d721dbfaa79 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -28,6 +28,7 @@ mod runtime; pub use crate::wasm::code_cache::reinstrument; pub use crate::wasm::{ env_def::Environment, + prepare::TryInstantiate, runtime::{CallFlags, ReturnCode, Runtime, RuntimeCosts}, }; use crate::{ @@ -126,11 +127,13 @@ where original_code: Vec, schedule: &Schedule, owner: AccountIdOf, + try_instantiate: TryInstantiate, ) -> Result { - let module = prepare::prepare_contract::( + let module = prepare::prepare::( original_code.try_into().map_err(|_| (>::CodeTooLarge.into(), ""))?, schedule, owner, + try_instantiate, )?; Ok(module) } @@ -206,7 +209,7 @@ where schedule: &Schedule, owner: T::AccountId, ) -> DispatchResult { - let executable = prepare::benchmarking::prepare_contract(original_code, schedule, owner) + let executable = prepare::benchmarking::prepare(original_code, schedule, owner) .map_err::(Into::into)?; code_cache::store(executable, false) } @@ -259,7 +262,7 @@ where (self.initial, self.maximum), ) .map_err(|msg| { - log::error!(target: "runtime::contracts", "existing code rejected: {}", msg); + log::debug!(target: "runtime::contracts", "failed to instantiate code: {}", msg); Error::::CodeRejected })?; store.state_mut().set_memory(memory); @@ -582,8 +585,13 @@ mod tests { fn execute>(wat: &str, input_data: Vec, mut ext: E) -> ExecResult { let wasm = wat::parse_str(wat).unwrap(); let schedule = crate::Schedule::default(); - let executable = PrefabWasmModule::<::T>::from_code(wasm, &schedule, ALICE) - .map_err(|err| err.0)?; + let executable = PrefabWasmModule::<::T>::from_code( + wasm, + &schedule, + ALICE, + TryInstantiate::Skip, + ) + .map_err(|err| err.0)?; executable.execute(ext.borrow_mut(), &ExportedFunction::Call, input_data) } diff --git a/frame/contracts/src/wasm/prepare.rs b/frame/contracts/src/wasm/prepare.rs index 89c6f41e0b19e..b35d5449eb2e0 100644 --- a/frame/contracts/src/wasm/prepare.rs +++ b/frame/contracts/src/wasm/prepare.rs @@ -37,6 +37,21 @@ use wasm_instrument::parity_wasm::elements::{ /// compiler toolchains might not support specifying other modules than "env" for memory imports. pub const IMPORT_MODULE_MEMORY: &str = "env"; +/// Determines whether a module should be instantiated during preparation. +pub enum TryInstantiate { + /// Do the instantation to make sure that the module is valid. + /// + /// This should be used if a module is only uploaded but not executed. We need + /// to make sure that it can be actually instantiated. + Instantiate, + /// Skip the instantation during preparation. + /// + /// This makes sense when the preparation takes place as part of an instantation. Then + /// this instantiation would fail the whole transaction and an extra check is not + /// necessary. + Skip, +} + struct ContractModule<'a, T: Config> { /// A deserialized module. The module is valid (this is Guaranteed by `new` method). module: elements::Module, @@ -364,17 +379,81 @@ fn get_memory_limits( /// - all imported functions from the external environment matches defined by `env` module, /// /// The preprocessing includes injecting code for gas metering and metering the height of stack. -pub fn prepare_contract( +pub fn prepare( original_code: CodeVec, schedule: &Schedule, owner: AccountIdOf, + try_instantiate: TryInstantiate, ) -> Result, (DispatchError, &'static str)> where E: Environment<()>, T::AccountId: UncheckedFrom + AsRef<[u8]>, { - let (code, (initial, maximum)) = check_and_instrument::(original_code.as_ref(), schedule) - .map_err(|msg| (>::CodeRejected.into(), msg))?; + use wasmparser::{Validator, WasmFeatures}; + Validator::new_with_features(WasmFeatures { + relaxed_simd: false, + threads: false, + tail_call: false, + multi_memory: false, + exceptions: false, + memory64: false, + extended_const: false, + component_model: false, + // we disallow floats later on + deterministic_only: false, + mutable_global: false, + saturating_float_to_int: false, + sign_extension: false, + bulk_memory: false, + multi_value: false, + reference_types: false, + simd: false, + }) + .validate_all(original_code.as_ref()) + .map_err(|err| { + log::debug!(target: "runtime::contracts", "{}", err); + (Error::::CodeRejected.into(), "validation of new code failed") + })?; + + let (code, (initial, maximum)) = (|| { + let contract_module = ContractModule::new(original_code.as_ref(), schedule)?; + contract_module.scan_exports()?; + contract_module.ensure_no_internal_memory()?; + contract_module.ensure_table_size_limit(schedule.limits.table_size)?; + contract_module.ensure_global_variable_limit(schedule.limits.globals)?; + contract_module.ensure_no_floating_types()?; + contract_module.ensure_parameter_limit(schedule.limits.parameters)?; + contract_module.ensure_br_table_size_limit(schedule.limits.br_table_size)?; + + // We disallow importing `gas` function here since it is treated as implementation detail. + let disallowed_imports = [b"gas".as_ref()]; + let memory_limits = + get_memory_limits(contract_module.scan_imports(&disallowed_imports)?, schedule)?; + + let code = contract_module + .inject_gas_metering()? + .inject_stack_height_metering()? + .into_wasm_code()?; + + Ok((code, memory_limits)) + })() + .map_err(|msg: &str| { + log::debug!(target: "runtime::contracts", "new code rejected: {}", msg); + (Error::::CodeRejected.into(), msg) + })?; + + // This will make sure that the module can be actually run within wasmi: + // + // - Doesn't use any unknown imports. + // - Doesn't explode the wasmi bytecode generation. + if matches!(try_instantiate, TryInstantiate::Instantiate) { + PrefabWasmModule::::instantiate::(original_code.as_ref(), (), (initial, maximum)) + .map_err(|err| { + log::debug!(target: "runtime::contracts", "{}", err); + (Error::::CodeRejected.into(), "new code rejected after instrumentation") + })?; + } + let original_code_len = original_code.len(); let mut module = PrefabWasmModule { @@ -402,57 +481,29 @@ where Ok(module) } -/// The same as [`prepare_contract`] but without constructing a new [`PrefabWasmModule`] -/// -/// # Note +/// Just run the instrumentation without checking the code. /// -/// Use this when an existing contract should be re-instrumented with a newer schedule version. -pub fn check_and_instrument( +/// Only run this on already uploaded code which is already validated. This also doesn't +/// check new newly instrumented code as there is nothing we can do on a failure here +/// anyways: The code is already deployed and it doesn't matter if it fails here or when +/// actually running the code. +pub fn reinstrument( original_code: &[u8], schedule: &Schedule, -) -> Result<(Vec, (u32, u32)), &'static str> +) -> Result, &'static str> where E: Environment<()>, T::AccountId: UncheckedFrom + AsRef<[u8]>, { - let result = (|| { - let contract_module = ContractModule::new(original_code, schedule)?; - contract_module.scan_exports()?; - contract_module.ensure_no_internal_memory()?; - contract_module.ensure_table_size_limit(schedule.limits.table_size)?; - contract_module.ensure_global_variable_limit(schedule.limits.globals)?; - contract_module.ensure_no_floating_types()?; - contract_module.ensure_parameter_limit(schedule.limits.parameters)?; - contract_module.ensure_br_table_size_limit(schedule.limits.br_table_size)?; - - // We disallow importing `gas` function here since it is treated as implementation detail. - let disallowed_imports = [b"gas".as_ref()]; - let memory_limits = - get_memory_limits(contract_module.scan_imports(&disallowed_imports)?, schedule)?; - - // We validate the module by instantiating it with an bogus host state object - // This will validate the wasm and check that all imported objects are provided - // We want to do that before the instrumentation as this requires a valid module - PrefabWasmModule::::instantiate::(original_code, (), memory_limits).map_err( - |msg| { - log::debug!(target: "runtime::contracts", "validation failed: {}", msg); - "validation failed" - }, - )?; - - let code = contract_module - .inject_gas_metering()? - .inject_stack_height_metering()? - .into_wasm_code()?; - - Ok((code, memory_limits)) - })(); - - if let Err(msg) = &result { - log::debug!(target: "runtime::contracts", "CodeRejected: {}", msg); - } - - result + let contract_module = ContractModule::new(original_code, schedule)?; + contract_module + .inject_gas_metering() + .and_then(|module| module.inject_stack_height_metering()) + .and_then(|module| module.into_wasm_code()) + .map_err(|msg| { + log::error!(target: "runtime::contracts", "CodeRejected during reinstrument: {}", msg); + Error::::CodeRejected.into() + }) } /// Alternate (possibly unsafe) preparation functions used only for benchmarking. @@ -466,7 +517,7 @@ pub mod benchmarking { use super::*; /// Prepare function that neither checks nor instruments the passed in code. - pub fn prepare_contract( + pub fn prepare( original_code: Vec, schedule: &Schedule, owner: AccountIdOf, @@ -557,7 +608,7 @@ mod tests { }, .. Default::default() }; - let r = prepare_contract::(wasm, &schedule, ALICE); + let r = prepare::(wasm, &schedule, ALICE, TryInstantiate::Instantiate); assert_matches::assert_matches!(r.map_err(|(_, msg)| msg), $($expected)*); } }; @@ -693,7 +744,7 @@ mod tests { (func (export "deploy")) ) "#, - Err("Requested initial number of pages should not exceed the requested maximum") + Err("validation of new code failed") ); prepare_test!( @@ -759,7 +810,7 @@ mod tests { (func (export "deploy")) ) "#, - Err("Multiple memory imports defined") + Err("validation of new code failed") ); prepare_test!( @@ -953,7 +1004,7 @@ mod tests { (func (export "deploy")) ) "#, - Err("validation failed") + Err("new code rejected after instrumentation") ); }