Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional extra-checks to Wasmi executor and make checking more consistent #1217

Merged
merged 13 commits into from
Oct 3, 2024
Merged
12 changes: 12 additions & 0 deletions crates/wasmi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ std = [
# An example of such an environment is `wasm32-unknown-unknown`.
no-hash-maps = ["wasmi_collections/no-hash-maps"]

# Enables extra checks performed during Wasmi bytecode execution.
#
# These checks are unnecessary as long as Wasmi translation works as intended.
# If Wasmi translation invariants are broken due to bugs, these checks prevent
# Wasmi execution to exhibit undefined behavior (UB) in certain cases.
#
# Expected execution overhead is upt to 20%, if enabled.
#
# - Enable if your focus is on safety.
# - Disable if your focus is on execution speed.
extra-checks = []

[[bench]]
name = "benches"
harness = false
4 changes: 3 additions & 1 deletion crates/wasmi/src/engine/code_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,9 @@ impl FuncEntity {
// Safety: we just asserted that `self` must be an uncompiled function
// since otherwise we would have returned `None` above.
// Since this is a performance critical path we need to leave out this check.
unsafe { core::hint::unreachable_unchecked() }
unsafe {
unreachable_unchecked!("expected uncompiled function but found: {self:?}")
}
}
}
}
Expand Down
20 changes: 16 additions & 4 deletions crates/wasmi/src/engine/executor/instrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
bytecode::{index, BlockFuel, Const16, Instruction, Reg},
code_map::CodeMap,
executor::stack::{CallFrame, FrameRegisters, ValueStack},
utils::unreachable_unchecked,
DedupFuncType,
EngineFunc,
},
Expand Down Expand Up @@ -1418,9 +1419,14 @@
unsafe { self.cache.$name(index) }
.unwrap_or_else(|| {
const ENTITY_NAME: &'static str = ::core::stringify!($id_ty);
::core::unreachable!(
"missing {ENTITY_NAME} at index {index:?} for the currently used instance",
)
// Safety: within the Wasmi executor it is assumed that store entity
// indices within the Wasmi bytecode are always valid for the
// store. This is an invariant of the Wasmi translation.
unsafe {
unreachable_unchecked!(
"missing {ENTITY_NAME} at index {index:?} for the currently used instance",
)
}
})
}
)*
Expand Down Expand Up @@ -1727,7 +1733,13 @@
/// This includes [`Instruction`] variants such as [`Instruction::TableIndex`]
/// that primarily carry parameters for actually executable [`Instruction`].
fn invalid_instruction_word(&mut self) -> Result<(), Error> {
self.execute_trap(TrapCode::UnreachableCodeReached)
// Safety: Wasmi translation guarantees that branches are never taken to instruction parameters directly.
unsafe {
unreachable_unchecked!(

Check warning on line 1738 in crates/wasmi/src/engine/executor/instrs.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs.rs#L1738

Added line #L1738 was not covered by tests
"expected instruction but found instruction parameter: {:?}",
*self.ip.get()

Check warning on line 1740 in crates/wasmi/src/engine/executor/instrs.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs.rs#L1740

Added line #L1740 was not covered by tests
)
}
}

/// Executes a Wasm `unreachable` instruction.
Expand Down
71 changes: 55 additions & 16 deletions crates/wasmi/src/engine/executor/instrs/branch.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use super::Executor;
use crate::{
core::UntypedVal,
engine::bytecode::{
BranchOffset,
BranchOffset16,
Comparator,
ComparatorAndOffset,
Const16,
Instruction,
Reg,
engine::{
bytecode::{
BranchOffset,
BranchOffset16,
Comparator,
ComparatorAndOffset,
Const16,
Instruction,
Reg,
},
utils::unreachable_unchecked,
},
};
use core::cmp;
Expand Down Expand Up @@ -58,7 +61,14 @@
Instruction::Const32 { value } => UntypedVal::from(u32::from(value)),
Instruction::I64Const32 { value } => UntypedVal::from(i64::from(value)),
Instruction::F64Const32 { value } => UntypedVal::from(f64::from(value)),
_ => unreachable!(),
unexpected => {

Check warning on line 64 in crates/wasmi/src/engine/executor/instrs/branch.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/branch.rs#L64

Added line #L64 was not covered by tests
// Safety: one of the above instruction parameters is guaranteed to exist by the Wasmi translation.
unsafe {
unreachable_unchecked!(
"expected instruction parameter for `Instruction::BranchTable1` but found: {unexpected:?}"
)
}
}
};
self.ip.add(offset);
if let Instruction::BranchTableTarget { results, offset } = *self.ip.get() {
Expand All @@ -72,8 +82,16 @@
pub fn execute_branch_table_2(&mut self, index: Reg, len_targets: u32) {
let offset = self.fetch_branch_table_offset(index, len_targets);
self.ip.add(1);
let Instruction::Register2 { regs } = *self.ip.get() else {
unreachable!()
let regs = match *self.ip.get() {
Instruction::Register2 { regs } => regs,
unexpected => {

Check warning on line 87 in crates/wasmi/src/engine/executor/instrs/branch.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/branch.rs#L87

Added line #L87 was not covered by tests
// Safety: Wasmi translation guarantees that `Instruction::Register2` follows.
unsafe {
unreachable_unchecked!(
"expected `Instruction::Register2` but found: {unexpected:?}"
)
}
}
};
self.ip.add(offset);
if let Instruction::BranchTableTarget { results, offset } = *self.ip.get() {
Expand All @@ -91,8 +109,16 @@
pub fn execute_branch_table_3(&mut self, index: Reg, len_targets: u32) {
let offset = self.fetch_branch_table_offset(index, len_targets);
self.ip.add(1);
let Instruction::Register3 { regs } = *self.ip.get() else {
unreachable!()
let regs = match *self.ip.get() {
Instruction::Register3 { regs } => regs,
unexpected => {

Check warning on line 114 in crates/wasmi/src/engine/executor/instrs/branch.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/branch.rs#L112-L114

Added lines #L112 - L114 were not covered by tests
// Safety: Wasmi translation guarantees that `Instruction::Register3` follows.
unsafe {
unreachable_unchecked!(
"expected `Instruction::Register3` but found: {unexpected:?}"
)
}
}
};
self.ip.add(offset);
if let Instruction::BranchTableTarget { results, offset } = *self.ip.get() {
Expand All @@ -110,8 +136,16 @@
pub fn execute_branch_table_span(&mut self, index: Reg, len_targets: u32) {
let offset = self.fetch_branch_table_offset(index, len_targets);
self.ip.add(1);
let Instruction::RegisterSpan { span: values } = *self.ip.get() else {
unreachable!()
let values = match *self.ip.get() {
Instruction::RegisterSpan { span } => span,
unexpected => {

Check warning on line 141 in crates/wasmi/src/engine/executor/instrs/branch.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/branch.rs#L139-L141

Added lines #L139 - L141 were not covered by tests
// Safety: Wasmi translation guarantees that `Instruction::RegisterSpan` follows.
unsafe {
unreachable_unchecked!(
"expected `Instruction::RegisterSpan` but found: {unexpected:?}"
)
}
}
};
let len = values.len();
let values = values.span();
Expand Down Expand Up @@ -154,7 +188,12 @@
// will point to `Instruction::Return` which does the job for us.
// This has some technical advantages for us.
}
_ => unreachable!(),
unexpected => {

Check warning on line 191 in crates/wasmi/src/engine/executor/instrs/branch.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/branch.rs#L191

Added line #L191 was not covered by tests
// Safety: Wasmi translator guarantees that one of the above `Instruction` variants exists.
unsafe {
unreachable_unchecked!("expected target for `Instruction::BranchTableMany` but found: {unexpected:?}")
}
}
}
}

Expand Down
32 changes: 23 additions & 9 deletions crates/wasmi/src/engine/executor/instrs/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
bytecode::{index, Instruction, Reg, RegSpan},
code_map::CompiledFuncRef,
executor::stack::{CallFrame, FrameParams, ValueStack},
utils::unreachable_unchecked,
EngineFunc,
FuncParams,
},
Expand Down Expand Up @@ -184,9 +185,14 @@
let index = u32::from(self.get_register(index));
(index, table)
}
unexpected => unreachable!(
"expected `Instruction::CallIndirectParams[Imm16]` but found {unexpected:?}"
),
unexpected => {

Check warning on line 188 in crates/wasmi/src/engine/executor/instrs/call.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/call.rs#L188

Added line #L188 was not covered by tests
// Safety: Wasmi translation guarantees that correct instruction parameter follows.
unsafe {
unreachable_unchecked!(

Check warning on line 191 in crates/wasmi/src/engine/executor/instrs/call.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/call.rs#L191

Added line #L191 was not covered by tests
"expected `Instruction::CallIndirectParams` but found {unexpected:?}"
)
}
}
}
}

Expand All @@ -208,9 +214,14 @@
let index = u32::from(index);
(index, table)
}
unexpected => unreachable!(
"expected `Instruction::CallIndirectParams[Imm16]` but found {unexpected:?}"
),
unexpected => {

Check warning on line 217 in crates/wasmi/src/engine/executor/instrs/call.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/call.rs#L217

Added line #L217 was not covered by tests
// Safety: Wasmi translation guarantees that correct instruction parameter follows.
unsafe {
unreachable_unchecked!(

Check warning on line 220 in crates/wasmi/src/engine/executor/instrs/call.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/call.rs#L220

Added line #L220 was not covered by tests
"expected `Instruction::CallIndirectParamsImm16` but found {unexpected:?}"
)
}
}
}
}

Expand Down Expand Up @@ -260,9 +271,12 @@
self.copy_regs(uninit_params, regs);
}
unexpected => {
unreachable!(
"unexpected Instruction found while copying call parameters: {unexpected:?}"
)
// Safety: Wasmi translation guarantees that register list finalizer exists.
unsafe {
unreachable_unchecked!(

Check warning on line 276 in crates/wasmi/src/engine/executor/instrs/call.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/call.rs#L276

Added line #L276 was not covered by tests
"expected register-list finalizer but found: {unexpected:?}"
)
}
}
}
}
Expand Down
25 changes: 20 additions & 5 deletions crates/wasmi/src/engine/executor/instrs/copy.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use super::{Executor, InstructionPtr};
use crate::{
core::UntypedVal,
engine::bytecode::{AnyConst32, Const32, FixedRegSpan, Instruction, Reg, RegSpan},
engine::{
bytecode::{AnyConst32, Const32, FixedRegSpan, Instruction, Reg, RegSpan},
utils::unreachable_unchecked,
},
};
use core::slice;
use smallvec::SmallVec;
Expand Down Expand Up @@ -132,9 +135,14 @@
Instruction::Register { reg } => slice::from_ref(reg),
Instruction::Register2 { regs } => regs,
Instruction::Register3 { regs } => regs,
unexpected => unreachable!(
"unexpected Instruction found while copying many values: {unexpected:?}"
),
unexpected => {

Check warning on line 138 in crates/wasmi/src/engine/executor/instrs/copy.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/copy.rs#L138

Added line #L138 was not covered by tests
// Safety: Wasmi translator guarantees that register-list finalizer exists.
unsafe {
unreachable_unchecked!(

Check warning on line 141 in crates/wasmi/src/engine/executor/instrs/copy.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/copy.rs#L141

Added line #L141 was not covered by tests
"expected register-list finalizer but found: {unexpected:?}"
)
}
}
};
tmp.extend(values.iter().map(|value| self.get_register(*value)));
for (result, value) in results.iter_sized(tmp.len()).zip(tmp) {
Expand Down Expand Up @@ -175,7 +183,14 @@
Instruction::Register { reg } => slice::from_ref(reg),
Instruction::Register2 { regs } => regs,
Instruction::Register3 { regs } => regs,
unexpected => unreachable!("unexpected Instruction found while copying many non-overlapping values: {unexpected:?}"),
unexpected => {

Check warning on line 186 in crates/wasmi/src/engine/executor/instrs/copy.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/copy.rs#L186

Added line #L186 was not covered by tests
// Safety: Wasmi translator guarantees that register-list finalizer exists.
unsafe {
unreachable_unchecked!(

Check warning on line 189 in crates/wasmi/src/engine/executor/instrs/copy.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/copy.rs#L189

Added line #L189 was not covered by tests
"expected register-list finalizer but found: {unexpected:?}"
)
}
}
};
copy_values(values);
ip
Expand Down
8 changes: 7 additions & 1 deletion crates/wasmi/src/engine/executor/instrs/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
engine::{
bytecode::{Const16, Reg},
executor::instr_ptr::InstructionPtr,
utils::unreachable_unchecked,
},
ir::{index::Memory, Instruction},
store::StoreInner,
Expand All @@ -22,7 +23,12 @@
match *addr.get() {
Instruction::RegisterAndImm32 { reg, imm } => (reg, u32::from(imm)),
instr => {
unreachable!("expected an `Instruction::RegisterAndImm32` but found: {instr:?}")
// Safety: Wasmi translation guarantees that `Instruction::RegisterAndImm32` exists.
unsafe {
unreachable_unchecked!(

Check warning on line 28 in crates/wasmi/src/engine/executor/instrs/load.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/load.rs#L28

Added line #L28 was not covered by tests
"expected an `Instruction::RegisterAndImm32` but found: {instr:?}"
)
}
}
}
}
Expand Down
19 changes: 16 additions & 3 deletions crates/wasmi/src/engine/executor/instrs/memory.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use super::{Executor, InstructionPtr};
use crate::{
core::TrapCode,
engine::bytecode::{index::Data, Const16, Instruction, Reg},
engine::{
bytecode::{index::Data, Const16, Instruction, Reg},
utils::unreachable_unchecked,
},
error::EntityGrowError,
ir::index::Memory,
store::{ResourceLimiterRef, StoreInner},
Expand All @@ -17,7 +20,12 @@
match *addr.get() {
Instruction::MemoryIndex { index } => index,
unexpected => {
unreachable!("expected `Instruction::MemoryIndex` but found: {unexpected:?}")
// Safety: Wasmi translation guarantees that [`Instruction::MemoryIndex`] exists.
unsafe {
unreachable_unchecked!(

Check warning on line 25 in crates/wasmi/src/engine/executor/instrs/memory.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/memory.rs#L25

Added line #L25 was not covered by tests
"expected `Instruction::MemoryIndex` but found: {unexpected:?}"
)
}
}
}
}
Expand All @@ -29,7 +37,12 @@
match *addr.get() {
Instruction::DataIndex { index } => index,
unexpected => {
unreachable!("expected `Instruction::DataIndex` but found: {unexpected:?}")
// Safety: Wasmi translation guarantees that [`Instruction::DataIndex`] exists.
unsafe {
unreachable_unchecked!(

Check warning on line 42 in crates/wasmi/src/engine/executor/instrs/memory.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/memory.rs#L42

Added line #L42 was not covered by tests
"expected `Instruction::DataIndex` but found: {unexpected:?}"
)
}
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion crates/wasmi/src/engine/executor/instrs/return_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
engine::{
bytecode::{AnyConst32, BoundedRegSpan, Const32, Instruction, Reg, RegSpan},
executor::stack::FrameRegisters,
utils::unreachable_unchecked,
},
store::StoreInner,
};
Expand Down Expand Up @@ -236,7 +237,14 @@
Instruction::Register { reg } => slice::from_ref(reg),
Instruction::Register2 { regs } => regs,
Instruction::Register3 { regs } => regs,
unexpected => unreachable!("unexpected `Instruction` found while executing `Instruction::ReturnMany`: {unexpected:?}"),
unexpected => {

Check warning on line 240 in crates/wasmi/src/engine/executor/instrs/return_.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/return_.rs#L240

Added line #L240 was not covered by tests
// Safety: Wasmi translation guarantees that a register-list finalizer exists.
unsafe {
unreachable_unchecked!(

Check warning on line 243 in crates/wasmi/src/engine/executor/instrs/return_.rs

View check run for this annotation

Codecov / codecov/patch

crates/wasmi/src/engine/executor/instrs/return_.rs#L243

Added line #L243 was not covered by tests
"unexpected register-list finalizer but found: {unexpected:?}"
)
}
}
};
copy_results(values);
}
Expand Down
Loading
Loading