Cursed implementation of allowing missing imports on wasmtime (#4730)

* Quick and dirty impl.

* Clean up

* Check the signatures.

* Fix tests.

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Ignore non function members.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/executor/src/integration_tests/mod.rs

@@ -31,48 +31,6 @@ use sp_trie::{TrieConfiguration, trie_types::Layout};
 use crate::WasmExecutionMethod;
 
 pub type TestExternalities = CoreTestExternalities<Blake2Hasher, u64>;
-
-#[cfg(feature = "wasmtime")]
-mod wasmtime_missing_externals {
-	use sp_wasm_interface::{Function, FunctionContext, HostFunctions, Result, Signature, Value};
-
-	pub struct WasmtimeHostFunctions;
-
-	impl HostFunctions for WasmtimeHostFunctions {
-		fn host_functions() -> Vec<&'static dyn Function> {
-			vec![MISSING_EXTERNAL_FUNCTION, YET_ANOTHER_MISSING_EXTERNAL_FUNCTION]
-		}
-	}
-
-	struct MissingExternalFunction(&'static str);
-
-	impl Function for MissingExternalFunction {
-		fn name(&self) -> &str { self.0 }
-
-		fn signature(&self) -> Signature {
-			Signature::new(vec![], None)
-		}
-
-		fn execute(
-			&self,
-			_context: &mut dyn FunctionContext,
-			_args: &mut dyn Iterator<Item = Value>,
-		) -> Result<Option<Value>> {
-			panic!("should not be called");
-		}
-	}
-
-	static MISSING_EXTERNAL_FUNCTION: &'static MissingExternalFunction =
-		&MissingExternalFunction("missing_external");
-	static YET_ANOTHER_MISSING_EXTERNAL_FUNCTION: &'static MissingExternalFunction =
-		&MissingExternalFunction("yet_another_missing_external");
-}
-
-#[cfg(feature = "wasmtime")]
-type HostFunctions =
-	(wasmtime_missing_externals::WasmtimeHostFunctions, sp_io::SubstrateHostFunctions);
-
-#[cfg(not(feature = "wasmtime"))]
 type HostFunctions = sp_io::SubstrateHostFunctions;
 
 fn call_in_wasm<E: Externalities>(
@@ -114,40 +72,66 @@ fn returning_should_work(wasm_method: WasmExecutionMethod) {
 
 #[test_case(WasmExecutionMethod::Interpreted)]
 #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
-#[should_panic(expected = "Function `missing_external` is only a stub. Calling a stub is not allowed.")]
-#[cfg(not(feature = "wasmtime"))]
 fn call_not_existing_function(wasm_method: WasmExecutionMethod) {
-	let mut ext = TestExternalities::default();
-	let mut ext = ext.ext();
-	let test_code = WASM_BINARY;
+    let mut ext = TestExternalities::default();
+    let mut ext = ext.ext();
+    let test_code = WASM_BINARY;
 
-	call_in_wasm(
+    match call_in_wasm(
 		"test_calling_missing_external",
 		&[],
 		wasm_method,
 		&mut ext,
 		&test_code[..],
 		8,
-	).unwrap();
+	) {
+		Ok(_) => panic!("was expected an `Err`"),
+		Err(e) => {
+			match wasm_method {
+				WasmExecutionMethod::Interpreted => assert_eq!(
+					&format!("{:?}", e),
+					"Wasmi(Trap(Trap { kind: Host(Other(\"Function `missing_external` is only a stub. Calling a stub is not allowed.\")) }))"
+				),
+				#[cfg(feature = "wasmtime")]
+				WasmExecutionMethod::Compiled => assert_eq!(
+					&format!("{:?}", e),
+					"Other(\"call to undefined external function with index 67\")"
+				),
+			}
+		}
+	}
 }
 
 #[test_case(WasmExecutionMethod::Interpreted)]
 #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))]
-#[should_panic(expected = "Function `yet_another_missing_external` is only a stub. Calling a stub is not allowed.")]
-#[cfg(not(feature = "wasmtime"))]
 fn call_yet_another_not_existing_function(wasm_method: WasmExecutionMethod) {
 	let mut ext = TestExternalities::default();
 	let mut ext = ext.ext();
 	let test_code = WASM_BINARY;
 
-	call_in_wasm(
+	match call_in_wasm(
 		"test_calling_yet_another_missing_external",
 		&[],
 		wasm_method,
 		&mut ext,
 		&test_code[..],
 		8,
-	).unwrap();
+	) {
+		Ok(_) => panic!("was expected an `Err`"),
+		Err(e) => {
+			match wasm_method {
+				WasmExecutionMethod::Interpreted => assert_eq!(
+					&format!("{:?}", e),
+					"Wasmi(Trap(Trap { kind: Host(Other(\"Function `yet_another_missing_external` is only a stub. Calling a stub is not allowed.\")) }))"
+				),
+				#[cfg(feature = "wasmtime")]
+				WasmExecutionMethod::Compiled => assert_eq!(
+					&format!("{:?}", e),
+					"Other(\"call to undefined external function with index 68\")"
+				),
+			}
+		}
+	}
 }
 
 #[test_case(WasmExecutionMethod::Interpreted)]

substrate/client/executor/src/lib.rs

@@ -68,7 +68,7 @@ pub fn call_in_wasm<HF: sp_wasm_interface::HostFunctions>(
 	ext: &mut dyn Externalities,
 	code: &[u8],
 	heap_pages: u64,
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 ) -> error::Result<Vec<u8>> {
 	call_in_wasm_with_host_functions(
 		function,
@@ -78,7 +78,7 @@ pub fn call_in_wasm<HF: sp_wasm_interface::HostFunctions>(
 		code,
 		heap_pages,
 		HF::host_functions(),
-		allow_missing_imports,
+		allow_missing_func_imports,
 	)
 }
 
@@ -92,14 +92,14 @@ pub fn call_in_wasm_with_host_functions(
 	code: &[u8],
 	heap_pages: u64,
 	host_functions: Vec<&'static dyn sp_wasm_interface::Function>,
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 ) -> error::Result<Vec<u8>> {
 	let instance = wasm_runtime::create_wasm_runtime_with_code(
 		execution_method,
 		heap_pages,
 		code,
 		host_functions,
-		allow_missing_imports,
+		allow_missing_func_imports,
 	)?;
 
 	// It is safe, as we delete the instance afterwards.

substrate/client/executor/src/wasm_runtime.rs

@@ -191,15 +191,15 @@ pub fn create_wasm_runtime_with_code(
 	heap_pages: u64,
 	code: &[u8],
 	host_functions: Vec<&'static dyn Function>,
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 ) -> Result<Box<dyn WasmRuntime>, WasmError> {
 	match wasm_method {
 		WasmExecutionMethod::Interpreted =>
-			sc_executor_wasmi::create_instance(code, heap_pages, host_functions, allow_missing_imports)
+			sc_executor_wasmi::create_instance(code, heap_pages, host_functions, allow_missing_func_imports)
 				.map(|runtime| -> Box<dyn WasmRuntime> { Box::new(runtime) }),
 		#[cfg(feature = "wasmtime")]
 		WasmExecutionMethod::Compiled =>
-			sc_executor_wasmtime::create_instance(code, heap_pages, host_functions)
+			sc_executor_wasmtime::create_instance(code, heap_pages, host_functions, allow_missing_func_imports)
 				.map(|runtime| -> Box<dyn WasmRuntime> { Box::new(runtime) }),
 	}
 }

substrate/client/executor/wasmi/src/lib.rs

@@ -38,7 +38,7 @@ struct FunctionExecutor<'a> {
 	memory: MemoryRef,
 	table: Option<TableRef>,
 	host_functions: &'a [&'static dyn Function],
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 	missing_functions: &'a [String],
 }
 
@@ -48,7 +48,7 @@ impl<'a> FunctionExecutor<'a> {
 		heap_base: u32,
 		t: Option<TableRef>,
 		host_functions: &'a [&'static dyn Function],
-		allow_missing_imports: bool,
+		allow_missing_func_imports: bool,
 		missing_functions: &'a [String],
 	) -> Result<Self, Error> {
 		Ok(FunctionExecutor {
@@ -57,7 +57,7 @@ impl<'a> FunctionExecutor<'a> {
 			memory: m,
 			table: t,
 			host_functions,
-			allow_missing_imports,
+			allow_missing_func_imports,
 			missing_functions,
 		})
 	}
@@ -273,15 +273,15 @@ impl<'a> Sandbox for FunctionExecutor<'a> {
 
 struct Resolver<'a> {
 	host_functions: &'a[&'static dyn Function],
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 	missing_functions: RefCell<Vec<String>>,
 }
 
 impl<'a> Resolver<'a> {
-	fn new(host_functions: &'a[&'static dyn Function], allow_missing_imports: bool) -> Resolver<'a> {
+	fn new(host_functions: &'a[&'static dyn Function], allow_missing_func_imports: bool) -> Resolver<'a> {
 		Resolver {
 			host_functions,
-			allow_missing_imports,
+			allow_missing_func_imports,
 			missing_functions: RefCell::new(Vec::new()),
 		}
 	}
@@ -311,7 +311,7 @@ impl<'a> wasmi::ModuleImportResolver for Resolver<'a> {
 			}
 		}
 
-		if self.allow_missing_imports {
+		if self.allow_missing_func_imports {
 			trace!(target: "wasm-executor", "Could not find function `{}`, a stub will be provided instead.", name);
 			let id = self.missing_functions.borrow().len() + self.host_functions.len();
 			self.missing_functions.borrow_mut().push(name.to_string());
@@ -336,7 +336,7 @@ impl<'a> wasmi::Externals for FunctionExecutor<'a> {
 				.map_err(|msg| Error::FunctionExecution(function.name().to_string(), msg))
 				.map_err(wasmi::Trap::from)
 				.map(|v| v.map(Into::into))
-		} else if self.allow_missing_imports
+		} else if self.allow_missing_func_imports
 			&& index >= self.host_functions.len()
 			&& index < self.host_functions.len() + self.missing_functions.len()
 		{
@@ -381,7 +381,7 @@ fn call_in_wasm_module(
 	method: &str,
 	data: &[u8],
 	host_functions: &[&'static dyn Function],
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 	missing_functions: &Vec<String>,
 ) -> Result<Vec<u8>, Error> {
 	// extract a reference to a linear memory, optional reference to a table
@@ -397,7 +397,7 @@ fn call_in_wasm_module(
 		heap_base,
 		table,
 		host_functions,
-		allow_missing_imports,
+		allow_missing_func_imports,
 		missing_functions,
 	)?;
 
@@ -433,9 +433,9 @@ fn instantiate_module(
 	heap_pages: usize,
 	module: &Module,
 	host_functions: &[&'static dyn Function],
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 ) -> Result<(ModuleRef, Vec<String>), Error> {
-	let resolver = Resolver::new(host_functions, allow_missing_imports);
+	let resolver = Resolver::new(host_functions, allow_missing_func_imports);
 	// start module instantiation. Don't run 'start' function yet.
 	let intermediate_instance = ModuleInstance::new(
 		module,
@@ -575,7 +575,7 @@ pub struct WasmiRuntime {
 	host_functions: Vec<&'static dyn Function>,
 	/// Enable stub generation for functions that are not available in `host_functions`.
 	/// These stubs will error when the wasm blob tries to call them.
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 	/// List of missing functions detected during function resolution
 	missing_functions: Vec<String>,
 }
@@ -607,7 +607,7 @@ impl WasmRuntime for WasmiRuntime {
 			method,
 			data,
 			&self.host_functions,
-			self.allow_missing_imports,
+			self.allow_missing_func_imports,
 			&self.missing_functions,
 		)
 	}
@@ -617,7 +617,7 @@ pub fn create_instance(
 	code: &[u8],
 	heap_pages: u64,
 	host_functions: Vec<&'static dyn Function>,
-	allow_missing_imports: bool,
+	allow_missing_func_imports: bool,
 ) -> Result<WasmiRuntime, WasmError> {
 	let module = Module::from_buffer(&code).map_err(|_| WasmError::InvalidModule)?;
 
@@ -632,7 +632,7 @@ pub fn create_instance(
 		heap_pages as usize,
 		&module,
 		&host_functions,
-		allow_missing_imports,
+		allow_missing_func_imports,
 	).map_err(|e| WasmError::Instantiation(e.to_string()))?;
 
 	// Take state snapshot before executing anything.
@@ -648,7 +648,7 @@ pub fn create_instance(
 		instance,
 		state_snapshot,
 		host_functions,
-		allow_missing_imports,
+		allow_missing_func_imports,
 		missing_functions,
 	})
 }

substrate/client/executor/wasmtime/src/runtime.rs

@@ -18,7 +18,12 @@
 
 use crate::function_executor::FunctionExecutorState;
 use crate::trampoline::{EnvState, make_trampoline};
-use crate::util::{cranelift_ir_signature, read_memory_into, write_memory_from};
+use crate::util::{
+	cranelift_ir_signature,
+	convert_parity_wasm_signature,
+	read_memory_into,
+	write_memory_from
+};
 
 use sc_executor_common::{
 	error::{Error, Result, WasmError},
@@ -86,8 +91,9 @@ pub fn create_instance(
 	code: &[u8],
 	heap_pages: u64,
 	host_functions: Vec<&'static dyn Function>,
+	allow_missing_func_imports: bool,
 ) -> std::result::Result<WasmtimeRuntime, WasmError> {
-	let (compiled_module, context) = create_compiled_unit(code, &host_functions)?;
+	let (compiled_module, context) = create_compiled_unit(code, &host_functions, allow_missing_func_imports)?;
 
 	// Inspect the module for the min and max memory sizes.
 	let (min_memory_size, max_memory_size) = {
@@ -115,9 +121,95 @@ pub fn create_instance(
 	})
 }
 
+#[derive(Debug)]
+struct MissingFunction {
+	name: String,
+	sig: cranelift_codegen::ir::Signature,
+}
+
+#[derive(Debug)]
+struct MissingFunctionStubs {
+	stubs: HashMap<String, Vec<MissingFunction>>,
+}
+
+impl MissingFunctionStubs {
+	fn new() -> Self {
+		Self {
+			stubs: HashMap::new(),
+		}
+	}
+
+	fn insert(&mut self, module: String, name: String, sig: cranelift_codegen::ir::Signature) {
+		self.stubs.entry(module).or_insert_with(Vec::new).push(MissingFunction {
+			name,
+			sig,
+		});
+	}
+}
+
+fn scan_missing_functions(
+	code: &[u8],
+	host_functions: &[&'static dyn Function],
+) -> std::result::Result<MissingFunctionStubs, WasmError> {
+	let isa = target_isa()?;
+	let call_conv = isa.default_call_conv();
+
+	let module = parity_wasm::elements::Module::from_bytes(code)
+		.map_err(|e| WasmError::Other(format!("cannot deserialize error: {}", e)))?;
+
+	let types = module.type_section().map(|ts| ts.types()).unwrap_or(&[]);
+	let import_entries = module
+		.import_section()
+		.map(|is| is.entries())
+		.unwrap_or(&[]);
+
+	let mut missing_functions = MissingFunctionStubs::new();
+	for import_entry in import_entries {
+		let func_ty = match import_entry.external() {
+			parity_wasm::elements::External::Function(func_ty_idx) => {
+				let parity_wasm::elements::Type::Function(ref func_ty) =
+					types.get(*func_ty_idx as usize).ok_or_else(|| {
+						WasmError::Other(format!("corrupted module, type out of bounds"))
+					})?;
+				func_ty
+			}
+			_ => {
+				// We are looking only for missing **functions** here. Any other items, be they
+				// missing or not, will be handled at the resolution stage later.
+				continue;
+			}
+		};
+		let signature = convert_parity_wasm_signature(func_ty);
+
+		if import_entry.module() == "env" {
+			if let Some(hf) = host_functions
+				.iter()
+				.find(|hf| hf.name() == import_entry.field())
+			{
+				if signature == hf.signature() {
+					continue;
+				}
+			}
+		}
+
+		// This function is either not from the env module, or doesn't have a corresponding host
+		// function, or the signature is not matching. Add it to the list.
+		let sig = cranelift_ir_signature(signature, &call_conv);
+
+		missing_functions.insert(
+			import_entry.module().to_string(),
+			import_entry.field().to_string(),
+			sig,
+		);
+	}
+
+	Ok(missing_functions)
+}
+
 fn create_compiled_unit(
 	code: &[u8],
 	host_functions: &[&'static dyn Function],
+	allow_missing_func_imports: bool,
 ) -> std::result::Result<(CompiledModule, Context), WasmError> {
 	let compilation_strategy = CompilationStrategy::Cranelift;
 
@@ -130,8 +222,27 @@ fn create_compiled_unit(
 	// Instantiate and link the env module.
 	let global_exports = context.get_global_exports();
 	let compiler = new_compiler(compilation_strategy)?;
-	let env_module = instantiate_env_module(global_exports, compiler, host_functions)?;
-	context.name_instance("env".to_owned(), env_module);
+
+	let mut missing_functions_stubs = if allow_missing_func_imports {
+		scan_missing_functions(code, host_functions)?
+	} else {
+		// If there are in fact missing functions they will be detected at the instantiation time
+		// and the module will be rejected.
+		MissingFunctionStubs::new()
+	};
+
+	let env_missing_functions = missing_functions_stubs.stubs.remove("env").unwrap_or_else(|| Vec::new());
+	context.name_instance(
+		"env".to_owned(),
+		instantiate_env_module(global_exports, compiler, host_functions, env_missing_functions)?,
+	);
+
+	for (module, missing_functions_stubs) in missing_functions_stubs.stubs {
+		let compiler = new_compiler(compilation_strategy)?;
+		let global_exports = context.get_global_exports();
+		let instance = instantiate_env_module(global_exports, compiler, &[], missing_functions_stubs)?;
+		context.name_instance(module, instance);
+	}
 
 	// Compile the wasm module.
 	let module = context.compile_module(&code)
@@ -199,6 +310,7 @@ fn instantiate_env_module(
 	global_exports: Rc<RefCell<HashMap<String, Option<Export>>>>,
 	compiler: Compiler,
 	host_functions: &[&'static dyn Function],
+	missing_functions_stubs: Vec<MissingFunction>,
 ) -> std::result::Result<InstanceHandle, WasmError>
 {
 	let isa = target_isa()?;
@@ -231,6 +343,26 @@ fn instantiate_env_module(
 		finished_functions.push(trampoline);
 	}
 
+	for MissingFunction { name, sig } in missing_functions_stubs {
+		let sig = translate_signature(
+			sig,
+			pointer_type,
+		);
+		let sig_id = module.signatures.push(sig.clone());
+		let func_id = module.functions.push(sig_id);
+		module
+			.exports
+			.insert(name, wasmtime_environ::Export::Function(func_id));
+		let trampoline = make_trampoline(
+			isa.as_ref(),
+			&mut code_memory,
+			&mut fn_builder_ctx,
+			func_id.index() as u32,
+			&sig,
+		)?;
+		finished_functions.push(trampoline);
+	}
+
 	code_memory.publish();
 
 	let imports = Imports::none();

substrate/client/executor/wasmtime/src/util.rs

@@ -51,6 +51,24 @@ pub fn checked_range(offset: usize, len: usize, max: usize) -> Option<Range<usiz
 	}
 }
 
+/// Convert from a parity wasm FunctionType to wasm interface's Signature.
+pub fn convert_parity_wasm_signature(func_ty: &parity_wasm::elements::FunctionType) -> Signature {
+	fn convert_value_type(val_ty: parity_wasm::elements::ValueType) -> ValueType {
+		use parity_wasm::elements::ValueType::*;
+		match val_ty {
+			I32 => ValueType::I32,
+			I64 => ValueType::I64,
+			F32 => ValueType::F32,
+			F64 => ValueType::F64,
+		}
+	}
+
+	Signature::new(
+		func_ty.params().iter().cloned().map(convert_value_type).collect::<Vec<_>>(),
+		func_ty.return_type().map(convert_value_type),
+	)
+}
+
 /// Convert a wasm_interface Signature into a cranelift_codegen Signature.
 pub fn cranelift_ir_signature(signature: Signature, call_conv: &isa::CallConv) -> ir::Signature {
 	ir::Signature {