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

@@ -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],
+		);
+	}
 }

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)?;

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()
 	}
 );
 

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> {

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
 		};
 

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 {

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)?;
+	// 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),
+		};
 
-	let run_result = instance.invoke(b"call", &[], &mut runtime);
-
-	to_execution_result(runtime, run_result.err())
+	to_execution_result(runtime, exec_error)
 }
 
 // TODO: Extract it to the root of the crate