From 15a0bfb0f64aed88502ecd13110ee42ba152a707 Mon Sep 17 00:00:00 2001
From: Koute <koute@users.noreply.github.com>
Date: Thu, 18 Nov 2021 20:16:38 +0900
Subject: [PATCH] Clear WASM linear memory on other OSes besides Linux too
(#10291)
---
.../executor/src/integration_tests/mod.rs | 78 +++++++++++++++++++
.../executor/wasmtime/src/instance_wrapper.rs | 16 ++--
.../client/executor/wasmtime/src/runtime.rs | 8 +-
3 files changed, 91 insertions(+), 11 deletions(-)
diff --git a/substrate/client/executor/src/integration_tests/mod.rs b/substrate/client/executor/src/integration_tests/mod.rs
index 7aa02a61dba..01c040687dd 100644
--- a/substrate/client/executor/src/integration_tests/mod.rs
+++ b/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());
+}
diff --git a/substrate/client/executor/wasmtime/src/instance_wrapper.rs b/substrate/client/executor/wasmtime/src/instance_wrapper.rs
index 2b8508ee2b0..1d40563d0a9 100644
--- a/substrate/client/executor/wasmtime/src/instance_wrapper.rs
+++ b/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);
}
}
diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs
index bd113c33838..4d107862173 100644
--- a/substrate/client/executor/wasmtime/src/runtime.rs
+++ b/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`].
--
GitLab