From 488830e81abad4df55de65ab78892b57e2ccd4b7 Mon Sep 17 00:00:00 2001
From: Sergey Pepyakin <s.pepyakin@gmail.com>
Date: Wed, 19 Sep 2018 11:01:25 +0300
Subject: [PATCH] Fix error handling in sandboxing/contracts modules (#744)
* Fix error handling in sandboxing/contracts modules
* Add some docs.
* Add some tests.
* grammar
---
substrate/core/executor/src/sandbox.rs | 115 +++++++++++++++++--
substrate/core/executor/src/wasm_executor.rs | 18 ++-
substrate/core/executor/wasm/src/lib.rs | 10 ++
substrate/core/sr-sandbox/src/lib.rs | 7 +-
substrate/core/sr-sandbox/with_std.rs | 2 +-
substrate/core/sr-sandbox/without_std.rs | 1 +
substrate/srml/contract/src/vm/mod.rs | 24 ++--
7 files changed, 155 insertions(+), 22 deletions(-)
diff --git a/substrate/core/executor/src/sandbox.rs b/substrate/core/executor/src/sandbox.rs
index 4d5fa7620f9..4d79b373ea0 100644
--- a/substrate/core/executor/src/sandbox.rs
+++ b/substrate/core/executor/src/sandbox.rs
@@ -335,11 +335,27 @@ impl SandboxInstance {
}
}
+/// Error occured during instantiation of a sandboxed module.
+pub enum InstantiationError {
+ /// Something wrong with the environment definition. It either can't
+ /// be decoded, have a reference to a non-existent or torn down memory instance.
+ EnvironmentDefintionCorrupted,
+ /// Provided module isn't recognized as a valid webassembly binary.
+ ModuleDecoding,
+ /// Module is a well-formed webassembly binary but could not be instantiated. This could
+ /// happen because, e.g. the module imports entries not provided by the environment.
+ Instantiation,
+ /// Module is well-formed, instantiated and linked, but while executing the start function
+ /// a trap was generated.
+ StartTrapped,
+}
+
fn decode_environment_definition(
raw_env_def: &[u8],
memories: &[Option<MemoryRef>],
-) -> Result<(Imports, GuestToSupervisorFunctionMapping), UserError> {
- let env_def = sandbox_primitives::EnvironmentDefinition::decode(&mut &raw_env_def[..]).ok_or_else(|| UserError("Sandbox error"))?;
+) -> Result<(Imports, GuestToSupervisorFunctionMapping), InstantiationError> {
+ let env_def = sandbox_primitives::EnvironmentDefinition::decode(&mut &raw_env_def[..])
+ .ok_or_else(|| InstantiationError::EnvironmentDefintionCorrupted)?;
let mut func_map = HashMap::new();
let mut memories_map = HashMap::new();
@@ -359,8 +375,8 @@ fn decode_environment_definition(
let memory_ref = memories
.get(memory_idx as usize)
.cloned()
- .ok_or_else(|| UserError("Sandbox error"))?
- .ok_or_else(|| UserError("Sandbox error"))?;
+ .ok_or_else(|| InstantiationError::EnvironmentDefintionCorrupted)?
+ .ok_or_else(|| InstantiationError::EnvironmentDefintionCorrupted)?;
memories_map.insert((module, field), memory_ref);
}
}
@@ -395,12 +411,12 @@ pub fn instantiate<FE: SandboxCapabilities + Externals>(
wasm: &[u8],
raw_env_def: &[u8],
state: u32,
-) -> Result<u32, UserError> {
+) -> Result<u32, InstantiationError> {
let (imports, guest_to_supervisor_mapping) =
decode_environment_definition(raw_env_def, &supervisor_externals.store().memories)?;
- let module = Module::from_buffer(wasm).map_err(|_| UserError("Sandbox error"))?;
- let instance = ModuleInstance::new(&module, &imports).map_err(|_| UserError("Sandbox error"))?;
+ let module = Module::from_buffer(wasm).map_err(|_| InstantiationError::ModuleDecoding)?;
+ let instance = ModuleInstance::new(&module, &imports).map_err(|_| InstantiationError::Instantiation)?;
let sandbox_instance = Rc::new(SandboxInstance {
// In general, it's not a very good idea to use `.not_started_instance()` for anything
@@ -418,14 +434,14 @@ pub fn instantiate<FE: SandboxCapabilities + Externals>(
|guest_externals| {
instance
.run_start(guest_externals)
- .map_err(|_| UserError("Sandbox error"))
+ .map_err(|_| InstantiationError::StartTrapped)
},
)?;
+ // At last, register the instance.
let instance_idx = supervisor_externals
.store_mut()
.register_sandbox_instance(sandbox_instance);
-
Ok(instance_idx)
}
@@ -674,4 +690,85 @@ mod tests {
vec![1],
);
}
+
+ #[test]
+ fn unlinkable_module() {
+ let mut ext = TestExternalities::default();
+ let test_code = include_bytes!("../wasm/target/wasm32-unknown-unknown/release/runtime_test.compact.wasm");
+
+ let code = wabt::wat2wasm(r#"
+ (module
+ (import "env" "non-existent" (func))
+
+ (func (export "call")
+ )
+ )
+ "#).unwrap();
+
+ assert_eq!(
+ WasmExecutor::new().call(&mut ext, 8, &test_code[..], "test_sandbox_instantiate", &code).unwrap(),
+ vec![1],
+ );
+ }
+
+ #[test]
+ fn corrupted_module() {
+ let mut ext = TestExternalities::default();
+ let test_code = include_bytes!("../wasm/target/wasm32-unknown-unknown/release/runtime_test.compact.wasm");
+
+ // Corrupted wasm file
+ let code = &[0, 0, 0, 0, 1, 0, 0, 0];
+
+ assert_eq!(
+ WasmExecutor::new().call(&mut ext, 8, &test_code[..], "test_sandbox_instantiate", code).unwrap(),
+ vec![1],
+ );
+ }
+
+ #[test]
+ fn start_fn_ok() {
+ let mut ext = TestExternalities::default();
+ let test_code = include_bytes!("../wasm/target/wasm32-unknown-unknown/release/runtime_test.compact.wasm");
+
+ let code = wabt::wat2wasm(r#"
+ (module
+ (func (export "call")
+ )
+
+ (func $start
+ )
+
+ (start $start)
+ )
+ "#).unwrap();
+
+ assert_eq!(
+ WasmExecutor::new().call(&mut ext, 8, &test_code[..], "test_sandbox_instantiate", &code).unwrap(),
+ vec![0],
+ );
+ }
+
+ #[test]
+ fn start_fn_traps() {
+ let mut ext = TestExternalities::default();
+ let test_code = include_bytes!("../wasm/target/wasm32-unknown-unknown/release/runtime_test.compact.wasm");
+
+ let code = wabt::wat2wasm(r#"
+ (module
+ (func (export "call")
+ )
+
+ (func $start
+ unreachable
+ )
+
+ (start $start)
+ )
+ "#).unwrap();
+
+ assert_eq!(
+ WasmExecutor::new().call(&mut ext, 8, &test_code[..], "test_sandbox_instantiate", &code).unwrap(),
+ vec![2],
+ );
+ }
}
diff --git a/substrate/core/executor/src/wasm_executor.rs b/substrate/core/executor/src/wasm_executor.rs
index 19f680d6635..e0d8da80d7c 100644
--- a/substrate/core/executor/src/wasm_executor.rs
+++ b/substrate/core/executor/src/wasm_executor.rs
@@ -344,7 +344,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
5
})
},
- ext_sandbox_instantiate(dispatch_thunk_idx: usize, wasm_ptr: *const u8, wasm_len: usize, imports_ptr: *const u8, imports_len: usize, state: usize) -> u32 => {
+ ext_sandbox_instantiate(
+ dispatch_thunk_idx: usize,
+ wasm_ptr: *const u8,
+ wasm_len: usize,
+ imports_ptr: *const u8,
+ imports_len: usize,
+ state: usize
+ ) -> u32 => {
let wasm = this.memory.get(wasm_ptr, wasm_len as usize).map_err(|_| UserError("Sandbox error"))?;
let raw_env_def = this.memory.get(imports_ptr, imports_len as usize).map_err(|_| UserError("Sandbox error"))?;
@@ -357,9 +364,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>,
.clone()
};
- let instance_idx = sandbox::instantiate(this, dispatch_thunk, &wasm, &raw_env_def, state)?;
+ let instance_idx_or_err_code =
+ match sandbox::instantiate(this, dispatch_thunk, &wasm, &raw_env_def, state) {
+ Ok(instance_idx) => instance_idx,
+ Err(sandbox::InstantiationError::StartTrapped) => sandbox_primitives::ERR_EXECUTION,
+ Err(_) => sandbox_primitives::ERR_MODULE,
+ };
- Ok(instance_idx as u32)
+ Ok(instance_idx_or_err_code as u32)
},
ext_sandbox_instance_teardown(instance_idx: u32) => {
this.sandbox_store.instance_teardown(instance_idx)?;
diff --git a/substrate/core/executor/wasm/src/lib.rs b/substrate/core/executor/wasm/src/lib.rs
index 3bec626091f..58759b2a469 100644
--- a/substrate/core/executor/wasm/src/lib.rs
+++ b/substrate/core/executor/wasm/src/lib.rs
@@ -81,6 +81,16 @@ impl_stubs!(
);
let ok = if let Ok(sandbox::ReturnValue::Value(sandbox::TypedValue::I32(0x1337))) = result { true } else { false };
[ok as u8].to_vec()
+ },
+ test_sandbox_instantiate NO_DECODE => |code: &[u8]| {
+ let env_builder = sandbox::EnvironmentDefinitionBuilder::new();
+ let code = match sandbox::Instance::new(code, &env_builder, &mut ()) {
+ Ok(_) => 0,
+ Err(sandbox::Error::Module) => 1,
+ Err(sandbox::Error::Execution) => 2,
+ Err(sandbox::Error::OutOfBounds) => 3,
+ };
+ [code].to_vec()
}
);
diff --git a/substrate/core/sr-sandbox/src/lib.rs b/substrate/core/sr-sandbox/src/lib.rs
index ca94edf4e41..49d18c13c99 100755
--- a/substrate/core/sr-sandbox/src/lib.rs
+++ b/substrate/core/sr-sandbox/src/lib.rs
@@ -190,7 +190,12 @@ pub struct Instance<T> {
}
impl<T> Instance<T> {
- /// Instantiate a module with the given [`EnvironmentDefinitionBuilder`].
+ /// Instantiate a module with the given [`EnvironmentDefinitionBuilder`]. It will
+ /// run the `start` function with the given `state`.
+ ///
+ /// Returns `Err(Error::Module)` if this module can't be instantiated with the given
+ /// environment. If execution of `start` function generated a trap, then `Err(Error::Execution)` will
+ /// be returned.
///
/// [`EnvironmentDefinitionBuilder`]: struct.EnvironmentDefinitionBuilder.html
pub fn new(code: &[u8], env_def_builder: &EnvironmentDefinitionBuilder<T>, state: &mut T) -> Result<Instance<T>, Error> {
diff --git a/substrate/core/sr-sandbox/with_std.rs b/substrate/core/sr-sandbox/with_std.rs
index e4e4aa131ea..d6a60694894 100755
--- a/substrate/core/sr-sandbox/with_std.rs
+++ b/substrate/core/sr-sandbox/with_std.rs
@@ -271,7 +271,7 @@ impl<T> Instance<T> {
state,
defined_host_functions: &defined_host_functions,
};
- let instance = not_started_instance.run_start(&mut externals).map_err(|_| Error::Module)?;
+ let instance = not_started_instance.run_start(&mut externals).map_err(|_| Error::Execution)?;
instance
};
diff --git a/substrate/core/sr-sandbox/without_std.rs b/substrate/core/sr-sandbox/without_std.rs
index f7feb466a09..6a14a198439 100755
--- a/substrate/core/sr-sandbox/without_std.rs
+++ b/substrate/core/sr-sandbox/without_std.rs
@@ -253,6 +253,7 @@ impl<T> Instance<T> {
};
let instance_idx = match result {
sandbox_primitives::ERR_MODULE => return Err(Error::Module),
+ sandbox_primitives::ERR_EXECUTION => return Err(Error::Execution),
instance_idx => instance_idx,
};
Ok(Instance {
diff --git a/substrate/srml/contract/src/vm/mod.rs b/substrate/srml/contract/src/vm/mod.rs
index 3bec1b40afa..53d7f82f7af 100644
--- a/substrate/srml/contract/src/vm/mod.rs
+++ b/substrate/srml/contract/src/vm/mod.rs
@@ -138,11 +138,11 @@ impl<'a, 'data, E: Ext + 'a> Runtime<'a, 'data, E> {
fn to_execution_result<E: Ext>(
runtime: Runtime<E>,
- run_err: Option<sandbox::Error>,
+ sandbox_err: Option<sandbox::Error>,
) -> Result<(), Error> {
// Check the exact type of the error. It could be plain trap or
// special runtime trap the we must recognize.
- match (run_err, runtime.special_trap) {
+ match (sandbox_err, runtime.special_trap) {
// No traps were generated. Proceed normally.
(None, None) => Ok(()),
// Special case. The trap was the result of the execution `return` host function.
@@ -188,12 +188,20 @@ pub fn execute<'a, E: Ext>(
special_trap: None,
};
- let mut instance = sandbox::Instance::new(&instrumented_code, &imports, &mut runtime)
- .map_err(|_| Error::Instantiate)?;
-
- let run_result = instance.invoke(b"call", &[], &mut runtime);
-
- to_execution_result(runtime, run_result.err())
+ // Instantiate the instance from the instrumented module code.
+ let exec_error: Option<sandbox::Error> =
+ match sandbox::Instance::new(&instrumented_code, &imports, &mut runtime) {
+ // No errors or traps were generated on instantiation! That
+ // means we can now invoke the contract entrypoint.
+ Ok(mut instance) => instance.invoke(b"call", &[], &mut runtime).err(),
+ // `start` function trapped.
+ Err(err @ sandbox::Error::Execution) => Some(err),
+ // Other instantiation errors.
+ // Return without executing anything.
+ Err(_) => return Err(Error::Instantiate),
+ };
+
+ to_execution_result(runtime, exec_error)
}
// TODO: Extract it to the root of the crate
--
GitLab