Clear WASM linear memory on other OSes besides Linux too (#10291)

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

@@ -699,3 +699,81 @@ fn panic_in_spawned_instance_panics_on_joining_its_result(wasm_method: WasmExecu
 
 	assert!(format!("{}", error_result).contains("Spawned task"));
 }
+
+test_wasm_execution!(memory_is_cleared_between_invocations);
+fn memory_is_cleared_between_invocations(wasm_method: WasmExecutionMethod) {
+	// This is based on the code generated by compiling a runtime *without*
+	// the `-C link-arg=--import-memory` using the following code and then
+	// disassembling the resulting blob with `wasm-dis`:
+	//
+	// ```
+	// #[no_mangle]
+	// #[cfg(not(feature = "std"))]
+	// pub fn returns_no_bss_mutable_static(_: *mut u8, _: usize) -> u64 {
+	//     static mut COUNTER: usize = 0;
+	//     let output = unsafe {
+	//        COUNTER += 1;
+	//        COUNTER as u64
+	//     };
+	//     sp_core::to_substrate_wasm_fn_return_value(&output)
+	// }
+	// ```
+	//
+	// This results in the BSS section to *not* be emitted, hence the executor has no way
+	// of knowing about the `static` variable's existence, so this test will fail if the linear
+	// memory is not properly cleared between invocations.
+	let binary = wat::parse_str(r#"
+	(module
+	 (type $i32_=>_i32 (func (param i32) (result i32)))
+	 (type $i32_i32_=>_i64 (func (param i32 i32) (result i64)))
+	 (import "env" "ext_allocator_malloc_version_1" (func $ext_allocator_malloc_version_1 (param i32) (result i32)))
+	 (global $__stack_pointer (mut i32) (i32.const 1048576))
+	 (global $global$1 i32 (i32.const 1048580))
+	 (global $global$2 i32 (i32.const 1048592))
+	 (memory $0 17)
+	 (export "memory" (memory $0))
+	 (export "returns_no_bss_mutable_static" (func $returns_no_bss_mutable_static))
+	 (export "__data_end" (global $global$1))
+	 (export "__heap_base" (global $global$2))
+	 (func $returns_no_bss_mutable_static (param $0 i32) (param $1 i32) (result i64)
+	  (local $2 i32)
+	  (local $3 i32)
+	  (i32.store offset=1048576
+	   (i32.const 0)
+	   (local.tee $2
+	    (i32.add
+	     (i32.load offset=1048576 (i32.const 0))
+	     (i32.const 1)
+	    )
+	   )
+	  )
+	  (i64.store
+	   (local.tee $3
+	    (call $ext_allocator_malloc_version_1 (i32.const 8))
+	   )
+	   (i64.extend_i32_u (local.get $2))
+	  )
+	  (i64.or
+	   (i64.extend_i32_u (local.get $3))
+	   (i64.const 34359738368)
+	  )
+	 )
+	)"#).unwrap();
+
+	let runtime = crate::wasm_runtime::create_wasm_runtime_with_code(
+		wasm_method,
+		1024,
+		RuntimeBlob::uncompress_if_needed(&binary[..]).unwrap(),
+		HostFunctions::host_functions(),
+		true,
+		None,
+	)
+	.unwrap();
+
+	let mut instance = runtime.new_instance().unwrap();
+	let res = instance.call_export("returns_no_bss_mutable_static", &[0]).unwrap();
+	assert_eq!(1, u64::decode(&mut &res[..]).unwrap());
+
+	let res = instance.call_export("returns_no_bss_mutable_static", &[0]).unwrap();
+	assert_eq!(1, u64::decode(&mut &res[..]).unwrap());
+}

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

@@ -403,10 +403,10 @@ impl InstanceWrapper {
 		self.memory.data_ptr(ctx)
 	}
 
-	/// Removes physical backing from the allocated linear memory. This leads to returning the
-	/// memory back to the system. While the memory is zeroed this is considered as a side-effect
-	/// and is not relied upon. Thus this function acts as a hint.
-	pub fn decommit(&self, ctx: impl AsContext) {
+	/// If possible removes physical backing from the allocated linear memory which
+	/// leads to returning the memory back to the system; this also zeroes the memory
+	/// as a side-effect.
+	pub fn decommit(&self, mut ctx: impl AsContextMut) {
 		if self.memory.data_size(&ctx) == 0 {
 			return
 		}
@@ -417,7 +417,7 @@ impl InstanceWrapper {
 
 				unsafe {
 					let ptr = self.memory.data_ptr(&ctx);
-					let len = self.memory.data_size(ctx);
+					let len = self.memory.data_size(&ctx);
 
 					// Linux handles MADV_DONTNEED reliably. The result is that the given area
 					// is unmapped and will be zeroed on the next pagefault.
@@ -429,9 +429,15 @@ impl InstanceWrapper {
 								std::io::Error::last_os_error(),
 							);
 						});
+					} else {
+						return;
 					}
 				}
 			}
 		}
+
+		// If we're on an unsupported OS or the memory couldn't have been
+		// decommited for some reason then just manually zero it out.
+		self.memory.data_mut(ctx.as_context_mut()).fill(0);
 	}
 }

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

@@ -237,7 +237,7 @@ impl WasmInstance for WasmtimeInstance {
 
 				// Signal to the OS that we are done with the linear memory and that it can be
 				// reclaimed.
-				instance_wrapper.decommit(&store);
+				instance_wrapper.decommit(store);
 
 				result
 			},
@@ -415,11 +415,7 @@ pub struct Semantics {
 	///
 	/// Primarily this is achieved by not recreating the instance for each call and performing a
 	/// bare minimum clean up: reapplying the data segments and restoring the values for global
-	/// variables. The vast majority of the linear memory is not restored, meaning that effects
-	/// of previous executions on the same [`WasmInstance`] can be observed there.
-	///
-	/// This is not a problem for a standard substrate runtime execution because it's up to the
-	/// runtime itself to make sure that it doesn't involve any non-determinism.
+	/// variables.
 	///
 	/// Since this feature depends on instrumentation, it can be set only if runtime is
 	/// instantiated using the runtime blob, e.g. using [`create_runtime`].