Skip to content

Commit

Permalink
fix(windows): Precise stack height limit (#3462)
Browse files Browse the repository at this point in the history
  • Loading branch information
ark0f authored Nov 15, 2023
1 parent a461819 commit bce05ca
Show file tree
Hide file tree
Showing 18 changed files with 325 additions and 110 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/CI.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ jobs:
- name: "Build: Node"
run: ./scripts/gear.sh build node --release --locked

- name: "Check: Stack height limit"
run: cargo run -p calc-stack-height --release --locked

- name: "Test: gsdk tests"
run: ./scripts/gear.sh test gsdk --release

Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/build-macos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ jobs:

- name: "Build: Node"
run: cargo build -p gear-cli

- name: "Check: Stack height limit"
run: cargo run -p calc-stack-height --release --locked

- name: "Test: Lazy pages"
run: >-
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ jobs:
- name: "Check: Vara runtime imports"
run: ./target/release/wasm-proc --check-runtime-imports target/release/wbuild/vara-runtime/vara_runtime.compact.wasm

- name: "Check: Stack height limit"
run: cargo run -p calc-stack-height --release --locked

- name: "Test: Gear workspace"
run: ./scripts/gear.sh test gear --exclude gclient --exclude gcli --exclude gsdk --release --locked

Expand Down Expand Up @@ -228,6 +231,11 @@ jobs:
env:
CARGO_BUILD_TARGET: x86_64-pc-windows-msvc

- name: "Check: Stack height limit"
run: cargo xwin run -p calc-stack-height --release --locked
env:
CARGO_BUILD_TARGET: x86_64-pc-windows-msvc

# These tests randomly stops responding

#- name: "Test: Client tests"
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
node_modules/
target/
target-no-lazy/
target-xwin/
log/
.binpath
.vscode
Expand Down
14 changes: 14 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ gear-wasm-gen = { path = "utils/wasm-gen" }
gear-wasm-instrument = { path = "utils/wasm-instrument", default-features = false }
junit-common = { path = "utils/junit-common" }
actor-system-error = { path = "utils/actor-system-error" }
calc-stack-height = { path = "utils/calc-stack-height" }
pallet-gear = { path = "pallets/gear", default-features = false }
pallet-gear-debug = { path = "pallets/gear-debug", default-features = false }
pallet-gear-gas = { path = "pallets/gas", default-features = false }
Expand All @@ -240,6 +241,10 @@ testing = { package = "gear-node-testing", path = "node/testing" }
vara-runtime = { path = "runtime/vara", default-features = false }
wasm-smith = { version = "0.12.21", git = "https://github.com/gear-tech/wasm-tools.git", branch = "gear-stable" }

# Common executors between `sandbox-host` and `calc-stack-height`
sandbox-wasmer = { package = "wasmer", version = "2.2", features = ["singlepass"] }
sandbox-wasmer-types = { package = "wasmer-types", version = "2.2" }

# Substrate deps
frame-benchmarking = { version = "4.0.0-dev", git = "https://github.com/gear-tech/substrate.git", branch = "gear-polkadot-v0.9.41-canary-no-sandbox-revert-oom-changes", default-features = false }
frame-benchmarking-cli = { version = "4.0.0-dev", git = "https://github.com/gear-tech/substrate.git", branch = "gear-polkadot-v0.9.41-canary-no-sandbox-revert-oom-changes" }
Expand Down
30 changes: 30 additions & 0 deletions core/src/code.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use alloc::{collections::BTreeSet, vec, vec::Vec};
use gear_wasm_instrument::{
parity_wasm::{
self,
builder::ModuleBuilder,
elements::{ExportEntry, GlobalEntry, GlobalType, InitExpr, Instruction, Internal, Module},
},
wasm_instrument::{
Expand Down Expand Up @@ -295,13 +296,31 @@ fn check_start_section(module: &Module) -> Result<(), CodeError> {
}
}

fn export_stack_height(module: Module) -> Module {
let globals = module
.global_section()
.expect("Global section must be create by `inject_stack_limiter` before")
.entries()
.len();
ModuleBuilder::new()
.with_module(module)
.export()
.field("__gear_stack_height")
.internal()
.global(globals as u32 - 1)
.build()
.build()
}

/// Configuration for `Code::try_new_mock_`.
/// By default all checks enabled.
pub struct TryNewCodeConfig {
/// Instrumentation version
pub version: u32,
/// Stack height limit
pub stack_height: Option<u32>,
/// Export `__gear_stack_height` global
pub export_stack_height: bool,
/// Check exports (wasm contains init or handle exports)
pub check_exports: bool,
/// Check and canonize stack end
Expand All @@ -319,6 +338,7 @@ impl Default for TryNewCodeConfig {
Self {
version: 1,
stack_height: None,
export_stack_height: false,
check_exports: true,
check_and_canonize_stack_end: true,
check_mut_global_exports: true,
Expand Down Expand Up @@ -398,10 +418,20 @@ impl Code {
}

if let Some(stack_limit) = config.stack_height {
let globals = config.export_stack_height.then(|| module.globals_space());

module = wasm_instrument::inject_stack_limiter(module, stack_limit).map_err(|err| {
log::trace!("Failed to inject stack height limits: {err}");
CodeError::StackLimitInjection
})?;

if let Some(globals_before) = globals {
// ensure stack limiter injector has created global
let globals_after = module.globals_space();
assert_eq!(globals_after, globals_before + 1);

module = export_stack_height(module);
}
}

if let Some(mut get_gas_rules) = get_gas_rules {
Expand Down
5 changes: 3 additions & 2 deletions node/cli/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,10 @@ pub fn run() -> sc_cli::Result<()> {
.execution
.get_or_insert(ExecutionStrategy::Wasm);

let is_dev = base.shared_params.dev;

// Checking if node supposed to be validator (explicitly or by shortcuts).
let is_validator = base.validator
|| base.shared_params.dev
|| base.alice
|| base.bob
|| base.charlie
Expand All @@ -169,7 +170,7 @@ pub fn run() -> sc_cli::Result<()> {
|| base.two;

// Denying ability to validate blocks with non-wasm execution.
if is_validator && *execution_strategy != ExecutionStrategy::Wasm {
if !is_dev && is_validator && *execution_strategy != ExecutionStrategy::Wasm {
return Err(
"Node can be --validator only with wasm execution strategy. To enable it run the node with `--execution wasm` or without the flag for default value."
.into(),
Expand Down
48 changes: 22 additions & 26 deletions pallets/gear/src/benchmarking/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,35 +28,25 @@ pub mod syscalls_integrity;
mod utils;

use crate::{
alloc::string::ToString,
benchmarking::{
code::{body, WasmModule},
utils as common_utils,
},
HandleKind,
};
use common::benchmarking;
use gear_core_backend::error::TrapExplanation;
use gear_wasm_instrument::parity_wasm::elements::Instruction;

pub fn check_stack_overflow<T>()
where
T: Config,
T::AccountId: Origin,
{
let instrs = vec![
Instruction::I64Const(10),
Instruction::GetGlobal(0),
Instruction::I64Add,
Instruction::SetGlobal(0),
Instruction::Call(0),
];
let instrs = vec![Instruction::Call(0)];

let module: WasmModule<T> = ModuleDefinition {
memory: Some(ImportedMemory::max::<T>()),
memory: Some(ImportedMemory::new(0)),
init_body: Some(body::from_instructions(instrs)),
stack_end: Some(0.into()),
num_globals: 1,
..Default::default()
}
.into();
Expand All @@ -70,18 +60,24 @@ where
)
.unwrap();

core_processor::process::<Ext>(&exec.block_config, exec.context, exec.random_data)
.unwrap()
.into_iter()
.find_map(|note| match note {
JournalNote::MessageDispatched { outcome, .. } => Some(outcome),
_ => None,
})
.map(|outcome| match outcome {
DispatchOutcome::InitFailure { reason, .. } => {
assert_eq!(reason, TrapExplanation::Unknown.to_string());
}
_ => panic!("Unexpected dispatch outcome: {:?}", outcome),
})
.unwrap();
let dispatch =
core_processor::process::<Ext>(&exec.block_config, exec.context, exec.random_data)
.unwrap()
.into_iter()
.find_map(|note| match note {
JournalNote::SendDispatch { dispatch, .. } => Some(dispatch),
_ => None,
})
.unwrap();

let code = dispatch
.reply_details()
.expect("reply details")
.to_reply_code();
assert_eq!(
code,
ReplyCode::Error(ErrorReplyReason::Execution(
SimpleExecutionError::UnreachableInstruction
))
);
}
17 changes: 8 additions & 9 deletions pallets/gear/src/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ pub const API_BENCHMARK_BATCH_SIZE: u32 = 80;
/// as for `API_BENCHMARK_BATCH_SIZE`.
pub const INSTR_BENCHMARK_BATCH_SIZE: u32 = 500;

// Constant for `stack_height` is calculated via `calc-stack-height` utility to be small enough
// to avoid stack overflow in wasmer and wasmi executors.
// To avoid potential stack overflow problems we have a panic in sandbox in case,
// execution is ended with stack overflow error. So, process queue execution will be
// stopped and we will be able to investigate the problem and decrease this constant if needed.
pub const STACK_HEIGHT_LIMIT: u32 = 18_369;

/// Definition of the cost schedule and other parameterization for the wasm vm.
///
/// Its [`Default`] implementation is the designated way to initialize this type. It uses
Expand Down Expand Up @@ -732,16 +739,8 @@ impl<T: Config> Default for Schedule<T> {
impl Default for Limits {
fn default() -> Self {
Self {
// Constant for `stack_height` is chosen to be small enough to avoid stack overflow in
// wasmer and wasmi executors. Currently it's just heuristic value.
// Unfortunately it's very hard to calculate this value precisely,
// because of difference of how stack height is calculated in injection and
// how wasmer and wasmi actually uses stack.
// To avoid potential stack overflow problems we have a panic in sandbox in case,
// execution is ended with stack overflow error. So, process queue execution will be
// stopped and we will be able to investigate the problem and decrease this constant if needed.
// TODO #3435. Disabled stack height is a temp solution.
stack_height: cfg!(not(feature = "fuzz")).then_some(20_000),
stack_height: cfg!(not(feature = "fuzz")).then_some(STACK_HEIGHT_LIMIT),
globals: 256,
locals: 1024,
parameters: 128,
Expand Down
4 changes: 2 additions & 2 deletions sandbox/host/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ codec = { workspace = true, features = ["std"] }
environmental = "1.1.3"
thiserror.workspace = true
log = { workspace = true, features = ["std"] }
wasmer = { version = "2.2", features = ["singlepass"] }
wasmer-types = "2.2"
sandbox-wasmer.workspace = true
sandbox-wasmer-types.workspace = true
wasmi = { git = "https://github.com/gear-tech/wasmi", branch = "v0.13.2-sign-ext", features = ["virtual_memory"] }
sp-allocator = { workspace = true, features = ["std"] }
sp-wasm-interface = { workspace = true, features = ["std"] }
Expand Down
2 changes: 1 addition & 1 deletion sandbox/host/src/sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ enum BackendInstance {
Wasmi(wasmi::ModuleRef),

/// Wasmer module instance
Wasmer(wasmer::Instance),
Wasmer(sandbox_wasmer::Instance),
}

/// Sandboxed instance of a wasm module.
Expand Down
Loading

0 comments on commit bce05ca

Please sign in to comment.