Refactor sp-sandbox; make sure both sandbox executors are always tested (#10173)

* sp-sandbox: convert executors into normal `mod`s instead of using `include!`

* sp-sandbox: run `cargo fmt` on `host_executor.rs`

* sp-sandbox: abstract away the executors behind traits

* sp_sandbox: always compile both executors when possible

* sc-executor: make sure all sandbox tests run on both sandbox executors

* sc-executor: fix brainfart: actually call into the sandbox through the trait

* sc-runtime-test: fix cargo fmt

* sc-runtime-test: deduplicate executor-specific sandbox test entrypoints

* sc-executor: test each sandbox executor in a separate test

* cargo fmt (Github's conflict resolving thingy broke indentation)

substrate/primitives/sandbox/src/embedded_executor.rs

@@ -15,6 +15,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//! An embedded WASM executor utilizing `wasmi`.
+
 use super::{Error, HostError, HostFuncType, ReturnValue, Value, TARGET};
 use alloc::string::String;
 use log::debug;
@@ -27,13 +29,14 @@ use wasmi::{
 	RuntimeArgs, RuntimeValue, Signature, TableDescriptor, TableRef, Trap, TrapKind,
 };
 
+/// The linear memory used by the sandbox.
 #[derive(Clone)]
 pub struct Memory {
 	memref: MemoryRef,
 }
 
-impl Memory {
-	pub fn new(initial: u32, maximum: Option<u32>) -> Result<Memory, Error> {
+impl super::SandboxMemory for Memory {
+	fn new(initial: u32, maximum: Option<u32>) -> Result<Memory, Error> {
 		Ok(Memory {
 			memref: MemoryInstance::alloc(
 				Pages(initial as usize),
@@ -43,12 +46,12 @@ impl Memory {
 		})
 	}
 
-	pub fn get(&self, ptr: u32, buf: &mut [u8]) -> Result<(), Error> {
+	fn get(&self, ptr: u32, buf: &mut [u8]) -> Result<(), Error> {
 		self.memref.get_into(ptr, buf).map_err(|_| Error::OutOfBounds)?;
 		Ok(())
 	}
 
-	pub fn set(&self, ptr: u32, value: &[u8]) -> Result<(), Error> {
+	fn set(&self, ptr: u32, value: &[u8]) -> Result<(), Error> {
 		self.memref.set(ptr, value).map_err(|_| Error::OutOfBounds)?;
 		Ok(())
 	}
@@ -118,20 +121,21 @@ enum ExternVal {
 	Memory(Memory),
 }
 
+/// A builder for the environment of the sandboxed WASM module.
 pub struct EnvironmentDefinitionBuilder<T> {
 	map: BTreeMap<(Vec<u8>, Vec<u8>), ExternVal>,
 	defined_host_functions: DefinedHostFunctions<T>,
 }
 
-impl<T> EnvironmentDefinitionBuilder<T> {
-	pub fn new() -> EnvironmentDefinitionBuilder<T> {
+impl<T> super::SandboxEnvironmentBuilder<T, Memory> for EnvironmentDefinitionBuilder<T> {
+	fn new() -> EnvironmentDefinitionBuilder<T> {
 		EnvironmentDefinitionBuilder {
 			map: BTreeMap::new(),
 			defined_host_functions: DefinedHostFunctions::new(),
 		}
 	}
 
-	pub fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<T>)
+	fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<T>)
 	where
 		N1: Into<Vec<u8>>,
 		N2: Into<Vec<u8>>,
@@ -140,7 +144,7 @@ impl<T> EnvironmentDefinitionBuilder<T> {
 		self.map.insert((module.into(), field.into()), ExternVal::HostFunc(idx));
 	}
 
-	pub fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
+	fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
 	where
 		N1: Into<Vec<u8>>,
 		N2: Into<Vec<u8>>,
@@ -213,14 +217,18 @@ impl<T> ImportResolver for EnvironmentDefinitionBuilder<T> {
 	}
 }
 
+/// Sandboxed instance of a WASM module.
 pub struct Instance<T> {
 	instance: ModuleRef,
 	defined_host_functions: DefinedHostFunctions<T>,
 	_marker: PhantomData<T>,
 }
 
-impl<T> Instance<T> {
-	pub fn new(
+impl<T> super::SandboxInstance<T> for Instance<T> {
+	type Memory = Memory;
+	type EnvironmentBuilder = EnvironmentDefinitionBuilder<T>;
+
+	fn new(
 		code: &[u8],
 		env_def_builder: &EnvironmentDefinitionBuilder<T>,
 		state: &mut T,
@@ -241,12 +249,7 @@ impl<T> Instance<T> {
 		Ok(Instance { instance, defined_host_functions, _marker: PhantomData::<T> })
 	}
 
-	pub fn invoke(
-		&mut self,
-		name: &str,
-		args: &[Value],
-		state: &mut T,
-	) -> Result<ReturnValue, Error> {
+	fn invoke(&mut self, name: &str, args: &[Value], state: &mut T) -> Result<ReturnValue, Error> {
 		let args = args.iter().cloned().map(to_wasmi).collect::<Vec<_>>();
 
 		let mut externals =
@@ -260,7 +263,7 @@ impl<T> Instance<T> {
 		}
 	}
 
-	pub fn get_global_val(&self, name: &str) -> Option<Value> {
+	fn get_global_val(&self, name: &str) -> Option<Value> {
 		let global = self.instance.export_by_name(name)?.as_global()?.get();
 
 		Some(to_interface(global))
@@ -289,7 +292,8 @@ fn to_interface(value: RuntimeValue) -> Value {
 
 #[cfg(test)]
 mod tests {
-	use crate::{EnvironmentDefinitionBuilder, Error, HostError, Instance, ReturnValue, Value};
+	use super::{EnvironmentDefinitionBuilder, Instance};
+	use crate::{Error, HostError, ReturnValue, SandboxEnvironmentBuilder, SandboxInstance, Value};
 	use assert_matches::assert_matches;
 
 	fn execute_sandboxed(code: &[u8], args: &[Value]) -> Result<ReturnValue, HostError> {

substrate/primitives/sandbox/src/host_executor.rs

@@ -15,15 +15,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//! A WASM executor utilizing the sandbox runtime interface of the host.
+
+use super::{Error, HostFuncType, ReturnValue, Value};
 use codec::{Decode, Encode};
 use sp_core::sandbox as sandbox_primitives;
 use sp_io::sandbox;
-use sp_std::{prelude::*, slice, marker, mem, vec, rc::Rc};
-use super::{Error, Value, ReturnValue, HostFuncType};
+use sp_std::{marker, mem, prelude::*, rc::Rc, slice, vec};
 
 mod ffi {
-	use sp_std::mem;
 	use super::HostFuncType;
+	use sp_std::mem;
 
 	/// Index into the default table that points to a `HostFuncType`.
 	pub type HostFuncIndex = usize;
@@ -38,8 +40,9 @@ mod ffi {
 	pub unsafe fn coerce_host_index_to_func<T>(idx: HostFuncIndex) -> HostFuncType<T> {
 		// We need to ensure that sizes of a callable function pointer and host function index is
 		// indeed equal.
-		// We can't use `static_assertions` create because it makes compiler panic, fallback to runtime assert.
-		// const_assert!(mem::size_of::<HostFuncIndex>() == mem::size_of::<HostFuncType<T>>());
+		// We can't use `static_assertions` create because it makes compiler panic, fallback to
+		// runtime assert. const_assert!(mem::size_of::<HostFuncIndex>() ==
+		// mem::size_of::<HostFuncType<T>>());
 		assert!(mem::size_of::<HostFuncIndex>() == mem::size_of::<HostFuncType<T>>());
 		mem::transmute::<HostFuncIndex, HostFuncType<T>>(idx)
 	}
@@ -55,6 +58,7 @@ impl Drop for MemoryHandle {
 	}
 }
 
+/// The linear memory used by the sandbox.
 #[derive(Clone)]
 pub struct Memory {
 	// Handle to memory instance is wrapped to add reference-counting semantics
@@ -62,29 +66,20 @@ pub struct Memory {
 	handle: Rc<MemoryHandle>,
 }
 
-impl Memory {
-	pub fn new(initial: u32, maximum: Option<u32>) -> Result<Memory, Error> {
-		let maximum = if let Some(maximum) = maximum {
-			maximum
-		} else {
-			sandbox_primitives::MEM_UNLIMITED
-		};
+impl super::SandboxMemory for Memory {
+	fn new(initial: u32, maximum: Option<u32>) -> Result<Memory, Error> {
+		let maximum =
+			if let Some(maximum) = maximum { maximum } else { sandbox_primitives::MEM_UNLIMITED };
 
 		match sandbox::memory_new(initial, maximum) {
 			sandbox_primitives::ERR_MODULE => Err(Error::Module),
-			memory_idx => Ok(Memory {
-				handle: Rc::new(MemoryHandle { 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 = sandbox::memory_get(
-			self.handle.memory_idx,
-			offset,
-			buf.as_mut_ptr(),
-			buf.len() as u32,
-		);
+	fn get(&self, offset: u32, buf: &mut [u8]) -> Result<(), Error> {
+		let result =
+			sandbox::memory_get(self.handle.memory_idx, offset, buf.as_mut_ptr(), buf.len() as u32);
 		match result {
 			sandbox_primitives::ERR_OK => Ok(()),
 			sandbox_primitives::ERR_OUT_OF_BOUNDS => Err(Error::OutOfBounds),
@@ -92,11 +87,11 @@ impl Memory {
 		}
 	}
 
-	pub fn set(&self, offset: u32, val: &[u8]) -> Result<(), Error> {
+	fn set(&self, offset: u32, val: &[u8]) -> Result<(), Error> {
 		let result = sandbox::memory_set(
 			self.handle.memory_idx,
 			offset,
-			val.as_ptr() as _ ,
+			val.as_ptr() as _,
 			val.len() as u32,
 		);
 		match result {
@@ -107,6 +102,7 @@ impl Memory {
 	}
 }
 
+/// A builder for the environment of the sandboxed WASM module.
 pub struct EnvironmentDefinitionBuilder<T> {
 	env_def: sandbox_primitives::EnvironmentDefinition,
 	retained_memories: Vec<Memory>,
@@ -114,16 +110,6 @@ pub struct EnvironmentDefinitionBuilder<T> {
 }
 
 impl<T> EnvironmentDefinitionBuilder<T> {
-	pub fn new() -> EnvironmentDefinitionBuilder<T> {
-		EnvironmentDefinitionBuilder {
-			env_def: sandbox_primitives::EnvironmentDefinition {
-				entries: Vec::new(),
-			},
-			retained_memories: Vec::new(),
-			_marker: marker::PhantomData::<T>,
-		}
-	}
-
 	fn add_entry<N1, N2>(
 		&mut self,
 		module: N1,
@@ -140,8 +126,18 @@ impl<T> EnvironmentDefinitionBuilder<T> {
 		};
 		self.env_def.entries.push(entry);
 	}
+}
 
-	pub fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<T>)
+impl<T> super::SandboxEnvironmentBuilder<T, Memory> for EnvironmentDefinitionBuilder<T> {
+	fn new() -> EnvironmentDefinitionBuilder<T> {
+		EnvironmentDefinitionBuilder {
+			env_def: sandbox_primitives::EnvironmentDefinition { entries: Vec::new() },
+			retained_memories: Vec::new(),
+			_marker: marker::PhantomData::<T>,
+		}
+	}
+
+	fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<T>)
 	where
 		N1: Into<Vec<u8>>,
 		N2: Into<Vec<u8>>,
@@ -150,7 +146,7 @@ impl<T> EnvironmentDefinitionBuilder<T> {
 		self.add_entry(module, field, f);
 	}
 
-	pub fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
+	fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
 	where
 		N1: Into<Vec<u8>>,
 		N2: Into<Vec<u8>>,
@@ -163,6 +159,7 @@ impl<T> EnvironmentDefinitionBuilder<T> {
 	}
 }
 
+/// Sandboxed instance of a WASM module.
 pub struct Instance<T> {
 	instance_idx: u32,
 	_retained_memories: Vec<Memory>,
@@ -211,8 +208,11 @@ extern "C" fn dispatch_thunk<T>(
 	}
 }
 
-impl<T> Instance<T> {
-	pub fn new(
+impl<T> super::SandboxInstance<T> for Instance<T> {
+	type Memory = Memory;
+	type EnvironmentBuilder = EnvironmentDefinitionBuilder<T>;
+
+	fn new(
 		code: &[u8],
 		env_def_builder: &EnvironmentDefinitionBuilder<T>,
 		state: &mut T,
@@ -242,12 +242,7 @@ impl<T> Instance<T> {
 		})
 	}
 
-	pub fn invoke(
-		&mut self,
-		name: &str,
-		args: &[Value],
-		state: &mut T,
-	) -> Result<ReturnValue, Error> {
+	fn invoke(&mut self, name: &str, args: &[Value], state: &mut T) -> Result<ReturnValue, Error> {
 		let serialized_args = args.to_vec().encode();
 		let mut return_val = vec![0u8; ReturnValue::ENCODED_MAX_SIZE];
 
@@ -262,16 +257,16 @@ impl<T> Instance<T> {
 
 		match result {
 			sandbox_primitives::ERR_OK => {
-				let return_val = ReturnValue::decode(&mut &return_val[..])
-					.map_err(|_| Error::Execution)?;
+				let return_val =
+					ReturnValue::decode(&mut &return_val[..]).map_err(|_| Error::Execution)?;
 				Ok(return_val)
-			}
+			},
 			sandbox_primitives::ERR_EXECUTION => Err(Error::Execution),
 			_ => unreachable!(),
 		}
 	}
 
-	pub fn get_global_val(&self, name: &str) -> Option<Value> {
+	fn get_global_val(&self, name: &str) -> Option<Value> {
 		sandbox::get_global_val(self.instance_idx, name)
 	}
 }

substrate/primitives/sandbox/src/lib.rs

@@ -48,13 +48,15 @@ pub use sp_wasm_interface::{ReturnValue, Value};
 /// The target used for logging.
 const TARGET: &str = "runtime::sandbox";
 
-mod imp {
-	#[cfg(all(feature = "wasmer-sandbox", not(feature = "std")))]
-	include!("../host_executor.rs");
+pub mod embedded_executor;
+#[cfg(not(feature = "std"))]
+pub mod host_executor;
 
-	#[cfg(not(all(feature = "wasmer-sandbox", not(feature = "std"))))]
-	include!("../embedded_executor.rs");
-}
+#[cfg(all(feature = "wasmer-sandbox", not(feature = "std")))]
+pub use host_executor as default_executor;
+
+#[cfg(not(all(feature = "wasmer-sandbox", not(feature = "std"))))]
+pub use embedded_executor as default_executor;
 
 /// Error that can occur while using this crate.
 #[derive(sp_core::RuntimeDebug)]
@@ -88,12 +90,7 @@ pub type HostFuncType<T> = fn(&mut T, &[Value]) -> Result<ReturnValue, HostError
 ///
 /// The memory can't be directly accessed by supervisor, but only
 /// through designated functions [`get`](Memory::get) and [`set`](Memory::set).
-#[derive(Clone)]
-pub struct Memory {
-	inner: imp::Memory,
-}
-
-impl Memory {
+pub trait SandboxMemory: Sized + Clone {
 	/// Construct a new linear memory instance.
 	///
 	/// The memory allocated with initial number of pages specified by `initial`.
@@ -104,38 +101,26 @@ impl Memory {
 	/// `maximum`. If not specified, this memory instance would be able to allocate up to 4GiB.
 	///
 	/// Allocated memory is always zeroed.
-	pub fn new(initial: u32, maximum: Option<u32>) -> Result<Memory, Error> {
-		Ok(Memory { inner: imp::Memory::new(initial, maximum)? })
-	}
+	fn new(initial: u32, maximum: Option<u32>) -> Result<Self, Error>;
 
 	/// Read a memory area at the address `ptr` with the size of the provided slice `buf`.
 	///
 	/// Returns `Err` if the range is out-of-bounds.
-	pub fn get(&self, ptr: u32, buf: &mut [u8]) -> Result<(), Error> {
-		self.inner.get(ptr, buf)
-	}
+	fn get(&self, ptr: u32, buf: &mut [u8]) -> Result<(), Error>;
 
 	/// Write a memory area at the address `ptr` with contents of the provided slice `buf`.
 	///
 	/// Returns `Err` if the range is out-of-bounds.
-	pub fn set(&self, ptr: u32, value: &[u8]) -> Result<(), Error> {
-		self.inner.set(ptr, value)
-	}
+	fn set(&self, ptr: u32, value: &[u8]) -> Result<(), Error>;
 }
 
 /// Struct that can be used for defining an environment for a sandboxed module.
 ///
 /// The sandboxed module can access only the entities which were defined and passed
 /// to the module at the instantiation time.
-pub struct EnvironmentDefinitionBuilder<T> {
-	inner: imp::EnvironmentDefinitionBuilder<T>,
-}
-
-impl<T> EnvironmentDefinitionBuilder<T> {
+pub trait SandboxEnvironmentBuilder<State, Memory>: Sized {
 	/// Construct a new `EnvironmentDefinitionBuilder`.
-	pub fn new() -> EnvironmentDefinitionBuilder<T> {
-		EnvironmentDefinitionBuilder { inner: imp::EnvironmentDefinitionBuilder::new() }
-	}
+	fn new() -> Self;
 
 	/// Register a host function in this environment definition.
 	///
@@ -143,32 +128,28 @@ impl<T> EnvironmentDefinitionBuilder<T> {
 	/// can import function passed here with any signature it wants. It can even import
 	/// the same function (i.e. with same `module` and `field`) several times. It's up to
 	/// the user code to check or constrain the types of signatures.
-	pub fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<T>)
+	fn add_host_func<N1, N2>(&mut self, module: N1, field: N2, f: HostFuncType<State>)
 	where
 		N1: Into<Vec<u8>>,
-		N2: Into<Vec<u8>>,
-	{
-		self.inner.add_host_func(module, field, f);
-	}
+		N2: Into<Vec<u8>>;
 
 	/// Register a memory in this environment definition.
-	pub fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
+	fn add_memory<N1, N2>(&mut self, module: N1, field: N2, mem: Memory)
 	where
 		N1: Into<Vec<u8>>,
-		N2: Into<Vec<u8>>,
-	{
-		self.inner.add_memory(module, field, mem.inner);
-	}
+		N2: Into<Vec<u8>>;
 }
 
 /// Sandboxed instance of a wasm module.
 ///
 /// This instance can be used for invoking exported functions.
-pub struct Instance<T> {
-	inner: imp::Instance<T>,
-}
+pub trait SandboxInstance<State>: Sized {
+	/// The memory type used for this sandbox.
+	type Memory: SandboxMemory;
+
+	/// The environment builder used to construct this sandbox.
+	type EnvironmentBuilder: SandboxEnvironmentBuilder<State, Self::Memory>;
 
-impl<T> Instance<T> {
 	/// Instantiate a module with the given [`EnvironmentDefinitionBuilder`]. It will
 	/// run the `start` function (if it is present in the module) with the given `state`.
 	///
@@ -177,13 +158,11 @@ impl<T> Instance<T> {
 	/// will be returned.
 	///
 	/// [`EnvironmentDefinitionBuilder`]: struct.EnvironmentDefinitionBuilder.html
-	pub fn new(
+	fn new(
 		code: &[u8],
-		env_def_builder: &EnvironmentDefinitionBuilder<T>,
-		state: &mut T,
-	) -> Result<Instance<T>, Error> {
-		Ok(Instance { inner: imp::Instance::new(code, &env_def_builder.inner, state)? })
-	}
+		env_def_builder: &Self::EnvironmentBuilder,
+		state: &mut State,
+	) -> Result<Self, Error>;
 
 	/// Invoke an exported function with the given name.
 	///
@@ -196,19 +175,15 @@ impl<T> Instance<T> {
 	/// - If types of the arguments passed to the function doesn't match function signature then
 	///   trap occurs (as if the exported function was called via call_indirect),
 	/// - Trap occurred at the execution time.
-	pub fn invoke(
+	fn invoke(
 		&mut self,
 		name: &str,
 		args: &[Value],
-		state: &mut T,
-	) -> Result<ReturnValue, Error> {
-		self.inner.invoke(name, args, state)
-	}
+		state: &mut State,
+	) -> Result<ReturnValue, Error>;
 
 	/// Get the value from a global with the given `name`.
 	///
 	/// Returns `Some(_)` if the global could be found.
-	pub fn get_global_val(&self, name: &str) -> Option<Value> {
-		self.inner.get_global_val(name)
-	}
+	fn get_global_val(&self, name: &str) -> Option<Value>;
 }