From 2414ffdac07c7dcccbdbb6bd0f2b2f383a7aeca4 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Sun, 30 Sep 2018 17:23:32 +0100 Subject: [PATCH] Fix `sandbox::Memory` lifecycle + sandox get memory function for no_std env (#845) * Fix `sandbox::Memory` lifecycle for no_std env * Retain memories in env_def builder and instance * Add scoped memory creation to test RC semantics * Add deploying_wasm_contract_should_work test. * Fix sandboxed memory set function. --- substrate/Cargo.lock | 2 + substrate/core/executor/src/wasm_executor.rs | 4 +- substrate/core/executor/wasm/src/lib.rs | 18 +- substrate/core/sr-sandbox/without_std.rs | 44 +++-- substrate/node/executor/Cargo.toml | 2 + substrate/node/executor/src/lib.rs | 193 ++++++++++++++++++- 6 files changed, 238 insertions(+), 25 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index dbdaae9709..18b5ff35b9 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -1611,6 +1611,7 @@ dependencies = [ "sr-primitives 0.1.0", "srml-balances 0.1.0", "srml-consensus 0.1.0", + "srml-contract 0.1.0", "srml-session 0.1.0", "srml-staking 0.1.0", "srml-support 0.1.0", @@ -1623,6 +1624,7 @@ dependencies = [ "substrate-state-machine 0.1.0", "substrate-trie 0.4.0", "trie-root 0.9.0 (git+https://github.com/paritytech/trie)", + "wabt 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/substrate/core/executor/src/wasm_executor.rs b/substrate/core/executor/src/wasm_executor.rs index 03dce4b5e1..be0711df25 100644 --- a/substrate/core/executor/src/wasm_executor.rs +++ b/substrate/core/executor/src/wasm_executor.rs @@ -438,11 +438,11 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ext_sandbox_memory_set(memory_idx: u32, offset: u32, val_ptr: *const u8, val_len: usize) -> u32 => { let dst_memory = this.sandbox_store.memory(memory_idx)?; - let data = match this.memory.get(offset, val_len as usize) { + let data = match this.memory.get(val_ptr, val_len as usize) { Ok(data) => data, Err(_) => return Ok(sandbox_primitives::ERR_OUT_OF_BOUNDS), }; - match dst_memory.set(val_ptr, &data) { + match dst_memory.set(offset, &data) { Err(_) => return Ok(sandbox_primitives::ERR_OUT_OF_BOUNDS), _ => {}, } diff --git a/substrate/core/executor/wasm/src/lib.rs b/substrate/core/executor/wasm/src/lib.rs index b2e795182c..fc4a1aa6ca 100644 --- a/substrate/core/executor/wasm/src/lib.rs +++ b/substrate/core/executor/wasm/src/lib.rs @@ -120,9 +120,21 @@ fn execute_sandboxed(code: &[u8], args: &[sandbox::TypedValue]) -> Result m, + Err(_) => unreachable!(" + Memory::new() can return Err only if parameters are borked; \ + We passing params here explicitly and they're correct; \ + Memory::new() can't return a Error qed" + ), + }; + env_builder.add_memory("env", "memory", memory.clone()); + env_builder + }; let mut instance = sandbox::Instance::new(code, &env_builder, &mut state)?; let result = instance.invoke(b"call", args, &mut state); diff --git a/substrate/core/sr-sandbox/without_std.rs b/substrate/core/sr-sandbox/without_std.rs index 6a14a19843..d75168f1b0 100755 --- a/substrate/core/sr-sandbox/without_std.rs +++ b/substrate/core/sr-sandbox/without_std.rs @@ -16,6 +16,7 @@ use rstd::prelude::*; use rstd::{slice, marker, mem}; +use rstd::rc::Rc; use codec::{Decode, Encode}; use primitives::sandbox as sandbox_primitives; use super::{Error, TypedValue, ReturnValue, HostFuncType}; @@ -89,9 +90,23 @@ mod ffi { } } +struct MemoryHandle { + memory_idx: u32, +} + +impl Drop for MemoryHandle { + fn drop(&mut self) { + unsafe { + ffi::ext_sandbox_memory_teardown(self.memory_idx); + } + } +} + #[derive(Clone)] pub struct Memory { - memory_idx: u32, + // Handle to memory instance is wrapped to add reference-counting semantics + // to `Memory`. + handle: Rc, } impl Memory { @@ -106,12 +121,14 @@ impl Memory { }; match result { sandbox_primitives::ERR_MODULE => Err(Error::Module), - memory_idx => Ok(Memory { memory_idx }), + memory_idx => Ok(Memory { + handle: Rc::new(MemoryHandle { memory_idx, }), + }), } } pub fn get(&self, offset: u32, buf: &mut [u8]) -> Result<(), Error> { - let result = unsafe { ffi::ext_sandbox_memory_get(self.memory_idx, offset, buf.as_mut_ptr(), buf.len()) }; + let result = unsafe { ffi::ext_sandbox_memory_get(self.handle.memory_idx, offset, buf.as_mut_ptr(), buf.len()) }; match result { sandbox_primitives::ERR_OK => Ok(()), sandbox_primitives::ERR_OUT_OF_BOUNDS => Err(Error::OutOfBounds), @@ -120,7 +137,7 @@ impl Memory { } pub fn set(&self, offset: u32, val: &[u8]) -> Result<(), Error> { - let result = unsafe { ffi::ext_sandbox_memory_set(self.memory_idx, offset, val.as_ptr(), val.len()) }; + let result = unsafe { ffi::ext_sandbox_memory_set(self.handle.memory_idx, offset, val.as_ptr(), val.len()) }; match result { sandbox_primitives::ERR_OK => Ok(()), sandbox_primitives::ERR_OUT_OF_BOUNDS => Err(Error::OutOfBounds), @@ -129,16 +146,9 @@ impl Memory { } } -impl Drop for Memory { - fn drop(&mut self) { - unsafe { - ffi::ext_sandbox_memory_teardown(self.memory_idx); - } - } -} - pub struct EnvironmentDefinitionBuilder { env_def: sandbox_primitives::EnvironmentDefinition, + retained_memories: Vec, _marker: marker::PhantomData, } @@ -148,6 +158,7 @@ impl EnvironmentDefinitionBuilder { env_def: sandbox_primitives::EnvironmentDefinition { entries: Vec::new(), }, + retained_memories: Vec::new(), _marker: marker::PhantomData::, } } @@ -183,13 +194,17 @@ impl EnvironmentDefinitionBuilder { N1: Into>, N2: Into>, { - let mem = sandbox_primitives::ExternEntity::Memory(mem.memory_idx as u32); + // We need to retain memory to keep it alive while the EnvironmentDefinitionBuilder alive. + self.retained_memories.push(mem.clone()); + + let mem = sandbox_primitives::ExternEntity::Memory(mem.handle.memory_idx as u32); self.add_entry(module, field, mem); } } pub struct Instance { instance_idx: u32, + _retained_memories: Vec, _marker: marker::PhantomData, } @@ -256,8 +271,11 @@ impl Instance { sandbox_primitives::ERR_EXECUTION => return Err(Error::Execution), instance_idx => instance_idx, }; + // We need to retain memories to keep them alive while the Instance is alive. + let retained_memories = env_def_builder.retained_memories.clone(); Ok(Instance { instance_idx, + _retained_memories: retained_memories, _marker: marker::PhantomData::, }) } diff --git a/substrate/node/executor/Cargo.toml b/substrate/node/executor/Cargo.toml index 96488d5ee6..3e0b4cf30c 100644 --- a/substrate/node/executor/Cargo.toml +++ b/substrate/node/executor/Cargo.toml @@ -27,3 +27,5 @@ srml-system = { path = "../../srml/system" } srml-consensus = { path = "../../srml/consensus" } srml-timestamp = { path = "../../srml/timestamp" } srml-treasury = { path = "../../srml/treasury" } +srml-contract = { path = "../../srml/contract" } +wabt = "0.4" diff --git a/substrate/node/executor/src/lib.rs b/substrate/node/executor/src/lib.rs index 5fbf9ff7fc..96eb1a863f 100644 --- a/substrate/node/executor/src/lib.rs +++ b/substrate/node/executor/src/lib.rs @@ -31,12 +31,14 @@ extern crate substrate_primitives as primitives; #[cfg(test)] extern crate srml_consensus as consensus; #[cfg(test)] extern crate srml_timestamp as timestamp; #[cfg(test)] extern crate srml_treasury as treasury; +#[cfg(test)] extern crate srml_contract as contract; #[cfg(test)] extern crate node_primitives; #[cfg(test)] extern crate parity_codec as codec; #[cfg(test)] extern crate sr_io as runtime_io; #[cfg(test)] extern crate substrate_trie as trie; #[cfg(test)] extern crate substrate_state_machine as state_machine; #[cfg(test)] #[macro_use] extern crate hex_literal; +#[cfg(test)] extern crate wabt; pub use substrate_executor::NativeExecutor; native_executor_instance!(pub Executor, node_runtime::api::dispatch, node_runtime::native_version, include_bytes!("../../runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm")); @@ -55,11 +57,13 @@ mod tests { use node_primitives::{Hash, BlockNumber, AccountId}; use runtime_primitives::traits::{Header as HeaderT, Digest as DigestT}; use runtime_primitives::{generic, generic::Era, ApplyOutcome, ApplyError, ApplyResult, Perbill}; - use {balances, staking, session, system, consensus, timestamp, treasury}; + use {balances, staking, session, system, consensus, timestamp, treasury, contract}; + use contract::ContractAddressFor; use system::{EventRecord, Phase}; use node_runtime::{Header, Block, UncheckedExtrinsic, CheckedExtrinsic, Call, Runtime, Balances, BuildStorage, GenesisConfig, BalancesConfig, SessionConfig, StakingConfig, System, SystemConfig, Event, Log}; + use wabt; const BLOATY_CODE: &[u8] = include_bytes!("../../runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.wasm"); const COMPACT_CODE: &[u8] = include_bytes!("../../runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm"); @@ -80,6 +84,10 @@ mod tests { AccountId::from(Keyring::Bob.to_raw_public()) } + fn charlie() -> AccountId { + AccountId::from(Keyring::Charlie.to_raw_public()) + } + fn sign(xt: CheckedExtrinsic) -> UncheckedExtrinsic { match xt.signed { Some((signed, index)) => { @@ -219,7 +227,10 @@ mod tests { ..Default::default() }), balances: Some(BalancesConfig { - balances: vec![(alice(), 111)], + balances: vec![ + (alice(), 111), + (charlie(), 100_000_000), + ], transaction_base_fee: 1, transaction_byte_fee: 0, existential_deposit: 0, @@ -286,9 +297,9 @@ mod tests { 1, GENESIS_HASH.into(), if support_changes_trie { - hex!("60548dd56fae6f47e3c9fde94e3612f1386c88791799c44239b473b6ecfeae2b").into() + hex!("9a7fe864529934ebb5915a855b0270f729803510bc12763c5c6fb87d3e231c17").into() } else { - hex!("4bdc21d39b43830969d61b451c7e6c4c81270e824b1c0bc92b52f9994745403a").into() + hex!("ee335055cb0c93b108ceed0431129d93b11796ab88d89efe81a29e8c6a1f270f").into() }, if support_changes_trie { Some(hex!("1f8f44dcae8982350c14dee720d34b147e73279f5a2ce1f9781195a991970978").into()) @@ -312,7 +323,7 @@ mod tests { construct_block( 2, block1(false).1, - hex!("aad6a6c83ede7c2da40700cd4754aebd9b50ab07e9580899581e1d5ab9fc4095").into(), + hex!("1db5d091b6efdf41dfd5294aa7a3ceb616fc1e0b95d3ee614f4d4af16e841195").into(), None, vec![ CheckedExtrinsic { @@ -335,7 +346,7 @@ mod tests { construct_block( 1, GENESIS_HASH.into(), - hex!("9b508cc4088b08eb72edea6ae9b2873fe6d75a1292d25bf18d93b89f19e39e39").into(), + hex!("6408d8740fbd31b234032678e9f91608fcccf83b0f900c63d52390dcbcd601a0").into(), None, vec![ CheckedExtrinsic { @@ -366,7 +377,7 @@ mod tests { }, EventRecord { phase: Phase::ApplyExtrinsic(1), - event: Event::balances(balances::RawEvent::NewAccount(bob(), 1, balances::NewAccountOutcome::NoHint)) + event: Event::balances(balances::RawEvent::NewAccount(bob(), 2, balances::NewAccountOutcome::NoHint)) }, EventRecord { phase: Phase::ApplyExtrinsic(1), @@ -479,6 +490,174 @@ mod tests { }); } + const CODE_TRANSFER: &str = r#" +(module + ;; ext_call( + ;; callee_ptr: u32, + ;; callee_len: u32, + ;; gas: u64, + ;; value_ptr: u32, + ;; value_len: u32, + ;; input_data_ptr: u32, + ;; input_data_len: u32 + ;; ) -> u32 + (import "env" "ext_call" (func $ext_call (param i32 i32 i64 i32 i32 i32 i32) (result i32))) + (import "env" "ext_input_size" (func $ext_input_size (result i32))) + (import "env" "ext_input_copy" (func $ext_input_copy (param i32 i32 i32))) + (import "env" "memory" (memory 1 1)) + (func (export "call") + (block $fail + ;; fail if ext_input_size != 4 + (br_if $fail + (i32.ne + (i32.const 4) + (call $ext_input_size) + ) + ) + + (call $ext_input_copy + (i32.const 0) + (i32.const 0) + (i32.const 4) + ) + + + (br_if $fail + (i32.ne + (i32.load8_u (i32.const 0)) + (i32.const 0) + ) + ) + (br_if $fail + (i32.ne + (i32.load8_u (i32.const 1)) + (i32.const 1) + ) + ) + (br_if $fail + (i32.ne + (i32.load8_u (i32.const 2)) + (i32.const 2) + ) + ) + (br_if $fail + (i32.ne + (i32.load8_u (i32.const 3)) + (i32.const 3) + ) + ) + + (drop + (call $ext_call + (i32.const 4) ;; Pointer to "callee" address. + (i32.const 32) ;; Length of "callee" address. + (i64.const 0) ;; How much gas to devote for the execution. 0 = all. + (i32.const 36) ;; Pointer to the buffer with value to transfer + (i32.const 16) ;; Length of the buffer with value to transfer. + (i32.const 0) ;; Pointer to input data buffer address + (i32.const 0) ;; Length of input data buffer + ) + ) + + (return) + ) + unreachable + ) + ;; Destination AccountId to transfer the funds. + ;; Represented by H256 (32 bytes long) in little endian. + (data (i32.const 4) "\09\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00") + ;; Amount of value to transfer. + ;; Represented by u128 (16 bytes long) in little endian. + (data (i32.const 36) "\06\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00") +) +"#; + + /// Convert a byte slice to a string with hex values. + /// Convert a byte slice to a string with hex values. + /// + /// Each value is preceeded with a `\` character. + fn escaped_bytestring(bytes: &[u8]) -> String { + use std::fmt::Write; + let mut result = String::new(); + for b in bytes { + write!(result, "\\{:02x}", b).unwrap(); + } + result + } + + /// Create a constructor for the specified code. + /// + /// When constructor is executed, it will call `ext_return` with code that + /// specified in `child_bytecode`. + fn code_ctor(child_bytecode: &[u8]) -> String { + format!( + r#" + (module + ;; ext_return(data_ptr: u32, data_len: u32) -> ! + (import "env" "ext_return" (func $ext_return (param i32 i32))) + (import "env" "memory" (memory 1 1)) + (func (export "call") + (call $ext_return + (i32.const 4) + (i32.const {code_len}) + ) + ;; ext_return is diverging, i.e. doesn't return. + unreachable + ) + (data (i32.const 4) "{escaped_bytecode}") + ) + "#, + escaped_bytecode = escaped_bytestring(child_bytecode), + code_len = child_bytecode.len(), + ) + } + + #[test] + fn deploying_wasm_contract_should_work() { + let mut t = new_test_ext(false); + + let code_transfer = wabt::wat2wasm(CODE_TRANSFER).unwrap(); + let code_ctor_transfer = wabt::wat2wasm(&code_ctor(&code_transfer)).unwrap(); + + let addr = ::DetermineContractAddress::contract_address_for( + &code_ctor_transfer, + &[], + &charlie(), + ); + + let b = construct_block( + 1, + GENESIS_HASH.into(), + hex!("062f87b5ba0e96e4a7dcc41afe56a2df7f65a975415a6c1b0e8e92c330124f8f").into(), + None, + vec![ + CheckedExtrinsic { + signed: None, + function: Call::Timestamp(timestamp::Call::set(42)), + }, + CheckedExtrinsic { + signed: Some((charlie(), 0)), + function: Call::Contract( + contract::Call::create::(10, 10_000, code_ctor_transfer, Vec::new()) + ), + }, + CheckedExtrinsic { + signed: Some((charlie(), 1)), + function: Call::Contract( + contract::Call::call::(addr, 10, 10_000, vec![0x00, 0x01, 0x02, 0x03]) + ), + }, + ] + ); + + WasmExecutor::new().call(&mut t, 8, COMPACT_CODE, "execute_block", &b.0).unwrap(); + + runtime_io::with_externalities(&mut t, || { + // Verify that the contract constructor worked well and code of TRANSFER contract is actually deployed. + assert_eq!(&contract::CodeOf::::get(addr), &code_transfer); + }); + } + #[test] fn wasm_big_block_import_fails() { let mut t = new_test_ext(false);