-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Wasm executor should provide stubs for unknown externs (wasmi) #4550
Changes from 12 commits
e10bddd
86b6bc5
2e61dca
ce243f4
e19948e
cdd65ee
935f976
9ec344c
5f86a84
335038b
2333994
0ef2746
1374979
79f4a2a
997bb60
f7109d1
c2f0e0d
797bfb6
d889cf7
cfd9b3b
b4b42d9
c67be22
0e926a8
4c80f58
36d8be2
6786bf8
623787f
000116e
131b7d4
0b67996
2cb0469
a5633f3
cd81107
cc1a4a8
7289892
77fcaa5
3785028
e78e4aa
63dc3ce
04a3db6
cc844f5
199a60e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,26 @@ fn call_in_wasm<E: Externalities>( | |
| ext, | ||
| code, | ||
| heap_pages, | ||
| false, | ||
| ) | ||
| } | ||
|
|
||
| fn call_in_wasm_with_stub<E: Externalities>( | ||
| function: &str, | ||
| call_data: &[u8], | ||
| execution_method: WasmExecutionMethod, | ||
| ext: &mut E, | ||
| code: &[u8], | ||
| heap_pages: u64, | ||
| ) -> crate::error::Result<Vec<u8>> { | ||
| crate::call_in_wasm::<E, sp_io::SubstrateHostFunctions>( | ||
| function, | ||
| call_data, | ||
| execution_method, | ||
| ext, | ||
| code, | ||
| heap_pages, | ||
| true, | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -68,6 +88,60 @@ fn returning_should_work(wasm_method: WasmExecutionMethod) { | |
| assert_eq!(output, vec![0u8; 0]); | ||
| } | ||
|
|
||
| #[test_case(WasmExecutionMethod::Interpreted)] | ||
| #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))] | ||
| #[should_panic(expected = "function missing_external does not exist")] | ||
| fn call_not_existing_function_with_stub_enabled(wasm_method: WasmExecutionMethod) { | ||
| let mut ext = TestExternalities::default(); | ||
| let mut ext = ext.ext(); | ||
| let test_code = WASM_BINARY; | ||
|
|
||
| call_in_wasm_with_stub( | ||
| "test_calling_missing_external", | ||
| &[], | ||
| wasm_method, | ||
| &mut ext, | ||
| &test_code[..], | ||
| 8, | ||
| ).unwrap(); | ||
| } | ||
|
|
||
| #[test_case(WasmExecutionMethod::Interpreted)] | ||
| #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))] | ||
| #[should_panic(expected = "function yet_another_missing_external does not exist")] | ||
| fn call_yet_another_not_existing_function_with_stub_enabled(wasm_method: WasmExecutionMethod) { | ||
| let mut ext = TestExternalities::default(); | ||
| let mut ext = ext.ext(); | ||
| let test_code = WASM_BINARY; | ||
|
|
||
| call_in_wasm_with_stub( | ||
| "test_calling_yet_another_missing_external", | ||
| &[], | ||
| wasm_method, | ||
| &mut ext, | ||
| &test_code[..], | ||
| 8, | ||
| ).unwrap(); | ||
| } | ||
|
|
||
| #[test_case(WasmExecutionMethod::Interpreted)] | ||
| #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))] | ||
| fn call_not_existing_function_without_stub_enabled(wasm_method: WasmExecutionMethod) { | ||
| let mut ext = TestExternalities::default(); | ||
| let mut ext = ext.ext(); | ||
| let test_code = WASM_BINARY; | ||
|
|
||
| let output = call_in_wasm( | ||
| "test_calling_missing_external", | ||
| &[], | ||
| wasm_method, | ||
| &mut ext, | ||
| &test_code[..], | ||
| 8, | ||
| ); | ||
| assert!(output.is_err()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check for the exact error you expect. Otherwise it could happen that the error type returned changes and the test still finishes successfully.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done but I removed the test entirely as we test only with stub enabled |
||
| } | ||
|
|
||
| #[test_case(WasmExecutionMethod::Interpreted)] | ||
| #[cfg_attr(feature = "wasmtime", test_case(WasmExecutionMethod::Compiled))] | ||
| fn panicking_should_work(wasm_method: WasmExecutionMethod) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ use sc_executor_common::{ | |
| allocator, | ||
| }; | ||
| use std::{str, mem}; | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| use std::cell::RefCell; | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| use std::collections::HashMap; | ||
| use wasmi::{ | ||
| Module, ModuleInstance, MemoryInstance, MemoryRef, TableRef, ImportsBuilder, ModuleRef, | ||
| memory_units::Pages, RuntimeValue::{I32, I64, self}, | ||
|
|
@@ -42,6 +44,8 @@ struct FunctionExecutor<'a> { | |
| memory: MemoryRef, | ||
| table: Option<TableRef>, | ||
| host_functions: &'a [&'static dyn Function], | ||
| enable_stub: bool, | ||
| missing_functions: &'a HashMap<usize, String>, | ||
| } | ||
|
|
||
| impl<'a> FunctionExecutor<'a> { | ||
|
|
@@ -50,13 +54,17 @@ impl<'a> FunctionExecutor<'a> { | |
| heap_base: u32, | ||
| t: Option<TableRef>, | ||
| host_functions: &'a [&'static dyn Function], | ||
| enable_stub: bool, | ||
| missing_functions: &'a HashMap<usize, String>, | ||
| ) -> Result<Self, Error> { | ||
| Ok(FunctionExecutor { | ||
| sandbox_store: sandbox::Store::new(), | ||
| heap: allocator::FreeingBumpHeapAllocator::new(heap_base), | ||
| memory: m, | ||
| table: t, | ||
| host_functions, | ||
| enable_stub, | ||
| missing_functions, | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -269,14 +277,30 @@ impl<'a> Sandbox for FunctionExecutor<'a> { | |
| } | ||
| } | ||
|
|
||
| struct Resolver<'a>(&'a[&'static dyn Function]); | ||
| struct Resolver<'a> { | ||
| host_functions: &'a[&'static dyn Function], | ||
| enable_stub: bool, | ||
| missing_functions: RefCell<HashMap<usize, String>>, | ||
| missing_function_id: RefCell<usize>, | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| impl<'a> Resolver<'a> { | ||
| fn new(host_functions: &'a[&'static dyn Function], enable_stub: bool) -> Resolver<'a> { | ||
| Resolver { | ||
| host_functions, | ||
| enable_stub, | ||
| missing_functions: RefCell::new(HashMap::new()), | ||
| missing_function_id: RefCell::new(host_functions.len()), | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a> wasmi::ModuleImportResolver for Resolver<'a> { | ||
| fn resolve_func(&self, name: &str, signature: &wasmi::Signature) | ||
| -> std::result::Result<wasmi::FuncRef, wasmi::Error> | ||
| { | ||
| let signature = sp_wasm_interface::Signature::from(signature); | ||
| for (function_index, function) in self.0.iter().enumerate() { | ||
| for (function_index, function) in self.host_functions.iter().enumerate() { | ||
| if name == function.name() { | ||
| if signature == function.signature() { | ||
| return Ok( | ||
|
|
@@ -295,9 +319,19 @@ impl<'a> wasmi::ModuleImportResolver for Resolver<'a> { | |
| } | ||
| } | ||
|
|
||
| Err(wasmi::Error::Instantiation( | ||
| format!("Export {} not found", name), | ||
| )) | ||
| if self.enable_stub { | ||
| trace!("Could not find function {}, a stub will be provided instead.", name); | ||
|
cecton marked this conversation as resolved.
Outdated
cecton marked this conversation as resolved.
Outdated
|
||
| let id = self.missing_function_id.borrow().clone(); | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| self.missing_functions.borrow_mut().insert(id, name.to_string()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We actually just require The index for the function will be computed as: (before adding the new missing function name!) And in invoke we do the following
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's what I implemented before as you can see here: 2333994 But I told you this looks way too clever and I should probably use a hashmap and you said you would have use a hashmap. I don't mind reverting it but I'm not in favor of this because I find that it doesn't look like it brings performance improvement, maybe memory, but it certainly make the code harder to understand. Now that I shared my humble opinion. What do you prefer? 😁
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I wanted to use a hashmap because I did not thought about this solution 😅
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| *self.missing_function_id.borrow_mut() += 1; | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
|
|
||
| // NOTE: provide purposedly an invalid index of the function | ||
| Ok(wasmi::FuncInstance::alloc_host(signature.into(), id)) | ||
| } else { | ||
| Err(wasmi::Error::Instantiation( | ||
| format!("Export {} not found", name), | ||
| )) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -306,16 +340,19 @@ impl<'a> wasmi::Externals for FunctionExecutor<'a> { | |
| -> Result<Option<wasmi::RuntimeValue>, wasmi::Trap> | ||
| { | ||
| let mut args = args.as_ref().iter().copied().map(Into::into); | ||
| let function = self.host_functions.get(index).ok_or_else(|| | ||
| Error::from( | ||
| format!("Could not find host function with index: {}", index), | ||
| ) | ||
| )?; | ||
|
|
||
| function.execute(self, &mut args) | ||
| .map_err(|msg| Error::FunctionExecution(function.name().to_string(), msg)) | ||
| .map_err(wasmi::Trap::from) | ||
| .map(|v| v.map(Into::into)) | ||
|
|
||
| if let Some(function) = self.host_functions.get(index) { | ||
| function.execute(self, &mut args) | ||
| .map_err(|msg| Error::FunctionExecution(function.name().to_string(), msg)) | ||
| .map_err(wasmi::Trap::from) | ||
| .map(|v| v.map(Into::into)) | ||
| } else if self.enable_stub { | ||
| Err(Error::from( | ||
| format!("function {} does not exist", self.missing_functions[&index]) | ||
| ).into()) | ||
| } else { | ||
| Err(Error::from(format!("Could not find host function with index: {}", index)).into()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -351,6 +388,8 @@ fn call_in_wasm_module( | |
| method: &str, | ||
| data: &[u8], | ||
| host_functions: &[&'static dyn Function], | ||
| enable_stub: bool, | ||
| missing_functions: &HashMap<usize, String>, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| // extract a reference to a linear memory, optional reference to a table | ||
| // and then initialize FunctionExecutor. | ||
|
|
@@ -360,7 +399,8 @@ fn call_in_wasm_module( | |
| .and_then(|e| e.as_table().cloned()); | ||
| let heap_base = get_heap_base(module_instance)?; | ||
|
|
||
| let mut fec = FunctionExecutor::new(memory.clone(), heap_base, table, host_functions)?; | ||
| let mut fec = FunctionExecutor::new( | ||
| memory.clone(), heap_base, table, host_functions, enable_stub, missing_functions)?; | ||
|
|
||
| // Write the call data | ||
| let offset = fec.allocate_memory(data.len() as u32)?; | ||
|
|
@@ -397,8 +437,9 @@ fn instantiate_module( | |
| heap_pages: usize, | ||
| module: &Module, | ||
| host_functions: &[&'static dyn Function], | ||
| ) -> Result<ModuleRef, Error> { | ||
| let resolver = Resolver(host_functions); | ||
| enable_stub: bool, | ||
| ) -> Result<(ModuleRef, HashMap<usize, String>), Error> { | ||
| let resolver = Resolver::new(host_functions, enable_stub); | ||
| // start module instantiation. Don't run 'start' function yet. | ||
| let intermediate_instance = ModuleInstance::new( | ||
| module, | ||
|
|
@@ -416,7 +457,7 @@ fn instantiate_module( | |
| // Runtime is not allowed to have the `start` function. | ||
| Err(Error::RuntimeHasStartFn) | ||
| } else { | ||
| Ok(intermediate_instance.assert_no_start()) | ||
| Ok((intermediate_instance.assert_no_start(), resolver.missing_functions.into_inner())) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -536,6 +577,10 @@ pub struct WasmiRuntime { | |
| state_snapshot: StateSnapshot, | ||
| /// The host functions registered for this instance. | ||
| host_functions: Vec<&'static dyn Function>, | ||
| /// Enable STUB for function called that are missing | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| enable_stub: bool, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of Aside, expanding the role of this field in the doc would be great!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine by me. @bkchr any remark?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like @pepyakin idea :) Sounds much better!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| /// List of missing functions detected during function resolution | ||
| missing_functions: HashMap<usize, String>, | ||
| } | ||
|
|
||
| impl WasmRuntime for WasmiRuntime { | ||
|
|
@@ -561,14 +606,23 @@ impl WasmRuntime for WasmiRuntime { | |
| error!(target: "wasm-executor", "snapshot restoration failed: {}", e); | ||
| e | ||
| })?; | ||
| call_in_wasm_module(ext, &self.instance, method, data, &self.host_functions) | ||
| call_in_wasm_module( | ||
| ext, | ||
| &self.instance, | ||
| method, | ||
| data, | ||
| &self.host_functions, | ||
| self.enable_stub, | ||
| &self.missing_functions, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| pub fn create_instance( | ||
| code: &[u8], | ||
| heap_pages: u64, | ||
| host_functions: Vec<&'static dyn Function>, | ||
| enable_stub: bool, | ||
| ) -> Result<WasmiRuntime, WasmError> { | ||
| let module = Module::from_buffer(&code).map_err(|_| WasmError::InvalidModule)?; | ||
|
|
||
|
|
@@ -579,7 +633,8 @@ pub fn create_instance( | |
| let data_segments = extract_data_segments(&code)?; | ||
|
|
||
| // Instantiate this module. | ||
| let instance = instantiate_module(heap_pages as usize, &module, &host_functions) | ||
| let (instance, missing_functions) = instantiate_module( | ||
| heap_pages as usize, &module, &host_functions, enable_stub) | ||
| .map_err(|e| WasmError::Instantiation(e.to_string()))?; | ||
|
bkchr marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Take state snapshot before executing anything. | ||
|
|
@@ -595,6 +650,8 @@ pub fn create_instance( | |
| instance, | ||
| state_snapshot, | ||
| host_functions, | ||
| enable_stub, | ||
| missing_functions, | ||
| }) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't the other tests fails because of missing imports? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (but I think wasmtime is failing at instantiation because of the missing function. I will fix that)