From bf741630e638c6c0902910651f9a8f7172b4eb83 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Mon, 8 Aug 2022 13:24:14 +0000 Subject: [PATCH 1/4] Restore `wasmtime`'s default stack size limit to 1MB --- client/executor/wasmtime/src/runtime.rs | 15 ++++--- client/executor/wasmtime/src/tests.rs | 60 +++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index 2feb9d0eea502..88991bc71970a 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -324,13 +324,14 @@ fn common_config(semantics: &Semantics) -> std::result::Result native_stack_max, + None => 1024 * 1024, + }; + + config + .max_wasm_stack(native_stack_max as usize) + .map_err(|e| WasmError::Other(format!("cannot set max wasm stack: {:#}", e)))?; config.parallel_compilation(semantics.parallel_compilation); diff --git a/client/executor/wasmtime/src/tests.rs b/client/executor/wasmtime/src/tests.rs index e0fd9fbce0c57..d2c19ab297a6d 100644 --- a/client/executor/wasmtime/src/tests.rs +++ b/client/executor/wasmtime/src/tests.rs @@ -174,6 +174,66 @@ impl RuntimeBuilder { } } +fn deep_call_stack_wat(depth: usize) -> String { + format!( + r#" + (module + (memory $0 32) + (export "memory" (memory $0)) + (global (export "__heap_base") i32 (i32.const 0)) + (func (export "overflow") call 0) + + (func $overflow (param $0 i32) + (block $label$1 + (br_if $label$1 + (i32.ge_u + (local.get $0) + (i32.const {depth}) + ) + ) + (call $overflow + (i32.add + (local.get $0) + (i32.const 1) + ) + ) + ) + ) + + (func (export "main") + (param i32 i32) (result i64) + (call $overflow (i32.const 0)) + (i64.const 0) + ) + ) + "# + ) +} + +test_wasm_execution!(test_consume_under_1mb_of_stack_does_not_trap); +fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: InstantiationStrategy) { + let wat = deep_call_stack_wat(65478); + let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); + let runtime = builder.build(); + let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); + instance.call_export("main", &[]).unwrap(); +} + +test_wasm_execution!(test_consume_over_1mb_of_stack_does_trap); +fn test_consume_over_1mb_of_stack_does_trap(instantiation_strategy: InstantiationStrategy) { + let wat = deep_call_stack_wat(65479); + let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); + let runtime = builder.build(); + let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); + match instance.call_export("main", &[]).unwrap_err() { + Error::AbortedDueToTrap(error) => { + let expected = "wasm trap: call stack exhausted"; + assert_eq!(error.message, expected); + }, + error => panic!("unexpected error: {:?}", error), + } +} + test_wasm_execution!(test_nan_canonicalization); fn test_nan_canonicalization(instantiation_strategy: InstantiationStrategy) { let mut builder = RuntimeBuilder::new(instantiation_strategy).canonicalize_nans(true); From 749a4478a644383efd14982280aa2d72dc152010 Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Tue, 9 Aug 2022 07:39:14 +0000 Subject: [PATCH 2/4] Add extra comments --- client/executor/wasmtime/src/runtime.rs | 5 +++++ client/executor/wasmtime/src/tests.rs | 14 ++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index 88991bc71970a..871de4e2300d2 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -326,6 +326,11 @@ fn common_config(semantics: &Semantics) -> std::result::Result native_stack_max, + + // In `wasmtime` 0.35 the default stack size limit was changed from 1MB to 512KB. + // + // This broke at least one parachain which depended on the original 1MB limit, + // so here we restore it to what it was originally. None => 1024 * 1024, }; diff --git a/client/executor/wasmtime/src/tests.rs b/client/executor/wasmtime/src/tests.rs index d2c19ab297a6d..6fc58e4020439 100644 --- a/client/executor/wasmtime/src/tests.rs +++ b/client/executor/wasmtime/src/tests.rs @@ -210,6 +210,20 @@ fn deep_call_stack_wat(depth: usize) -> String { ) } +// These two tests ensure that the `wasmtime`'s stack size limit and the amount of +// stack space used by a single stack frame doesn't suddenly change without us noticing. +// +// If they do (e.g. because we've pulled in a new version of `wasmtime`) we want to know +// that it did, regardless of how small the change was. +// +// If these tests starting failing it doesn't necessarily mean that something is broken; +// what it means is that one (or multiple) of the following has to be done: +// a) the tests may need to be updated for the new call depth, +// b) the stack limit may need to be changed to maintain backwards compatibility, +// c) the root cause of the new call depth limit determined, and potentially fixed, +// d) the new call depth limit may need to be validated to ensure it doesn't prevent any +// existing chain from syncing (if it was effectively decreased) + test_wasm_execution!(test_consume_under_1mb_of_stack_does_not_trap); fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: InstantiationStrategy) { let wat = deep_call_stack_wat(65478); From f72855013b5fa374bf590497d4f037ca7923560f Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Tue, 9 Aug 2022 08:20:31 +0000 Subject: [PATCH 3/4] Enforce different maximum call depth in release mode --- client/executor/wasmtime/src/tests.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/client/executor/wasmtime/src/tests.rs b/client/executor/wasmtime/src/tests.rs index 6fc58e4020439..840072c107443 100644 --- a/client/executor/wasmtime/src/tests.rs +++ b/client/executor/wasmtime/src/tests.rs @@ -224,9 +224,15 @@ fn deep_call_stack_wat(depth: usize) -> String { // d) the new call depth limit may need to be validated to ensure it doesn't prevent any // existing chain from syncing (if it was effectively decreased) +#[cfg(debug_assertions)] +const CALL_DEPTH_LIMIT: usize = 65478; + +#[cfg(not(debug_assertions))] +const CALL_DEPTH_LIMIT: usize = 65514; + test_wasm_execution!(test_consume_under_1mb_of_stack_does_not_trap); fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: InstantiationStrategy) { - let wat = deep_call_stack_wat(65478); + let wat = deep_call_stack_wat(CALL_DEPTH_LIMIT); let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); let runtime = builder.build(); let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); @@ -235,7 +241,7 @@ fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: Instant test_wasm_execution!(test_consume_over_1mb_of_stack_does_trap); fn test_consume_over_1mb_of_stack_does_trap(instantiation_strategy: InstantiationStrategy) { - let wat = deep_call_stack_wat(65479); + let wat = deep_call_stack_wat(CALL_DEPTH_LIMIT + 1); let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); let runtime = builder.build(); let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); From b05972518bb47057d7dd7a20a3566be222980b8c Mon Sep 17 00:00:00 2001 From: Jan Bujak Date: Tue, 9 Aug 2022 08:48:26 +0000 Subject: [PATCH 4/4] Split the call depth limit in two --- client/executor/wasmtime/src/tests.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/client/executor/wasmtime/src/tests.rs b/client/executor/wasmtime/src/tests.rs index 840072c107443..1ca5dde4c2b93 100644 --- a/client/executor/wasmtime/src/tests.rs +++ b/client/executor/wasmtime/src/tests.rs @@ -224,15 +224,14 @@ fn deep_call_stack_wat(depth: usize) -> String { // d) the new call depth limit may need to be validated to ensure it doesn't prevent any // existing chain from syncing (if it was effectively decreased) -#[cfg(debug_assertions)] -const CALL_DEPTH_LIMIT: usize = 65478; - -#[cfg(not(debug_assertions))] -const CALL_DEPTH_LIMIT: usize = 65514; +// We need two limits here since depending on whether the code is compiled in debug +// or in release mode the maximum call depth is slightly different. +const CALL_DEPTH_LOWER_LIMIT: usize = 65478; +const CALL_DEPTH_UPPER_LIMIT: usize = 65514; test_wasm_execution!(test_consume_under_1mb_of_stack_does_not_trap); fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: InstantiationStrategy) { - let wat = deep_call_stack_wat(CALL_DEPTH_LIMIT); + let wat = deep_call_stack_wat(CALL_DEPTH_LOWER_LIMIT); let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); let runtime = builder.build(); let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); @@ -241,7 +240,7 @@ fn test_consume_under_1mb_of_stack_does_not_trap(instantiation_strategy: Instant test_wasm_execution!(test_consume_over_1mb_of_stack_does_trap); fn test_consume_over_1mb_of_stack_does_trap(instantiation_strategy: InstantiationStrategy) { - let wat = deep_call_stack_wat(CALL_DEPTH_LIMIT + 1); + let wat = deep_call_stack_wat(CALL_DEPTH_UPPER_LIMIT + 1); let mut builder = RuntimeBuilder::new(instantiation_strategy).use_wat(wat); let runtime = builder.build(); let mut instance = runtime.new_instance().expect("failed to instantiate a runtime");