From 56664abe6f6e2240dede8fb74a633b1b58e391bb Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 8 Oct 2024 14:56:02 +0200 Subject: [PATCH 1/3] create the initial structure and errors for translation invariant checking --- .../wasmi/src/engine/translator/conditions.rs | 317 ++++++++++++++++++ .../src/engine/translator/instr_encoder.rs | 4 +- crates/wasmi/src/engine/translator/mod.rs | 7 + crates/wasmi/src/module/mod.rs | 23 ++ 4 files changed, 349 insertions(+), 2 deletions(-) create mode 100644 crates/wasmi/src/engine/translator/conditions.rs diff --git a/crates/wasmi/src/engine/translator/conditions.rs b/crates/wasmi/src/engine/translator/conditions.rs new file mode 100644 index 0000000000..9e9f2c07e8 --- /dev/null +++ b/crates/wasmi/src/engine/translator/conditions.rs @@ -0,0 +1,317 @@ +//! Wasmi translation post-conditions. +//! +//! These are run if `cfg(debug-assertions)` are enabled and +//! provide another layer of protection for invalid Wasmi translations. +//! +//! They are mostly intended for debugging purposes or for fuzzing Wasmi +//! as they would add too much overhead to conventional Wasmi translation. +//! +//! The set of post-conditions is run right after Wasmi translation has +//! finished compiling a Wasm function and just before the compiled function +//! is stored into Wasmi's `CodeMap`. +//! +//! The checks generally check invariants and guarantees given by the Wasmi +//! translation and therefore also act as "documentation" about some of them. + +use super::{FuncTranslator, Instr}; +use crate::ir::{self, BranchOffset, Instruction, Reg}; +use core::{ + cmp, + fmt::{self, Display}, +}; + +/// An error describing a broken Wasmi translation invariant. +#[derive(Debug)] +pub struct Error { + /// The erraneous `Instruction` index. + instr: Instr, + /// The reason for the error. + reason: Reason, +} + +/// The reason behind a broken Wasmi translation invariant. +#[derive(Debug)] +pub enum Reason { + InvalidRegister { + /// The invalid `Reg`. + reg: Reg, + }, + InvalidGlobal { + /// The invalid `Global` index. + invalid_global: ir::index::Global, + }, + InvalidFunc { + /// The invalid `Global` index. + invalid_func: ir::index::Func, + }, + InvalidTable { + /// The invalid `Table` index. + invalid_table: ir::index::Table, + }, + InvalidMemory { + /// The invalid `Memory` index. + invalid_memory: ir::index::Memory, + }, + InvalidBranchOffset { + /// The invalid `BranchOffset`. + invalid_offset: BranchOffset, + }, + InvalidBranchTarget { + /// The invalid target of the branch `Instruction`. + invalid_target: Instr, + }, + InvalidConsumeFuel { + /// The invalid fuel consumption amount. (usually 0) + invalid_fuel: u64, + }, + InvalidInstructionParameter { + /// The invalid `Instruction` parameter index. + invalid_param: Instr, + }, + InvalidReturnValues { + /// The invalid number of returned values. + invalid_results: u32, + }, +} + +impl Display for ErrorWithContext<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let pos = self.pos(); + let instr = self.instr(); + match self.error.reason { + Reason::InvalidRegister { reg } => { + writeln!( + f, + "unexpected invalid register for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- register: {reg:?}\ + " + ) + } + Reason::InvalidGlobal { invalid_global } => { + let len_globals = self.ctx.len_globals(); + write!( + f, + "unexpected invalid global for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid global: {invalid_global:?}\n\ + \t- number of globals in Wasm module: {len_globals}\ + " + ) + } + Reason::InvalidFunc { invalid_func } => { + let len_funcs = self.ctx.len_funcs(); + write!( + f, + "unexpected invalid function for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid function: {invalid_func:?}\n\ + \t- number of functions in Wasm module: {len_funcs}\ + " + ) + } + Reason::InvalidTable { invalid_table } => { + let len_tables = self.ctx.len_tables(); + write!( + f, + "unexpected invalid table for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid table: {invalid_table:?}\n\ + \t- number of tables in Wasm module: {len_tables}\ + " + ) + } + Reason::InvalidMemory { invalid_memory } => { + let len_memories = self.ctx.len_memories(); + write!( + f, + "unexpected invalid linear memory for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid linear memory: {invalid_memory:?}\n\ + \t- number of memories in Wasm module: {len_memories}\ + " + ) + } + Reason::InvalidBranchOffset { invalid_offset } => { + let fuel_metering_status = match self.ctx.is_fuel_metering_enabled() { + true => "enabled", + false => "disabled", + }; + write!( + f, + "unexpected invalid branching offset for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid branch offset: {invalid_offset:?}\n\ + \t- fuel metering: {fuel_metering_status}\ + " + ) + } + Reason::InvalidBranchTarget { invalid_target } => { + let invalid_target_pos = Self::instr_pos(invalid_target); + let invalid_target = self.ctx.resolve_instr(invalid_target); + let fuel_metering_status = match self.ctx.is_fuel_metering_enabled() { + true => "enabled", + false => "disabled", + }; + let branch_kind = match invalid_target_pos.cmp(&pos) { + cmp::Ordering::Less => "backward", + cmp::Ordering::Equal => "self-loop", + cmp::Ordering::Greater => "forward", + }; + write!( + f, + "unexpected invalid branching offset for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid target at: {invalid_target_pos:?}\n\ + \t- invalid branch target: {invalid_target:?}\n\ + \t- branch kind: {branch_kind}\n\ + \t- fuel metering: {fuel_metering_status}\ + " + ) + } + Reason::InvalidConsumeFuel { invalid_fuel } => { + let fuel_metering_status = match self.ctx.is_fuel_metering_enabled() { + true => "enabled", + false => "disabled", + }; + write!( + f, + "unexpected invalid fuel consumption for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid fuel consumption: {invalid_fuel:?}\n\ + \t- fuel metering: {fuel_metering_status}\ + " + ) + } + Reason::InvalidInstructionParameter { invalid_param } => { + let invalid_param_pos = Self::instr_pos(invalid_param); + write!( + f, + "unexpected invalid instruction parameter for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid instruction parameter at: {invalid_param_pos}\ + \t- invalid instruction parameter: {invalid_param:?}\ + " + ) + } + Reason::InvalidReturnValues { invalid_results } => { + write!( + f, + "unexpected invalid instruction parameter for instruction at {pos}:\n\ + \t- instruction: {instr:?}\n\ + \t- invalid number of results: {invalid_results}\ + " + ) + } + } + } +} + +/// An error combined with contextual information to improve its [`Display`] implementation. +pub struct ErrorWithContext<'ctx> { + /// The contextual information for the error. + ctx: ErrorContext<'ctx>, + /// The underlying error. + error: Error, +} + +/// A context to populate an [`Error`] with more information for its [`Display`] implementation. +pub struct ErrorContext<'ctx> { + /// The underlying Wasmi function translator that has already finished its job. + translator: &'ctx FuncTranslator, +} + +impl ErrorContext<'_> { + /// Returns `true` if fuel metering is enabled for the translation. + fn is_fuel_metering_enabled(&self) -> bool { + self.translator.fuel_costs.is_some() + } + + /// Resolves the [`Instruction`] at `instr`. + /// + /// # Panics + /// + /// If `instr` is invalid for the underlying Wasmi translator. + fn resolve_instr(&self, instr: Instr) -> Instruction { + *self.translator.alloc.instr_encoder.instrs.get(instr) + } + + /// Returns the number of global variables for the Wasm module of the compiled function. + fn len_globals(&self) -> usize { + self.translator.module.len_globals() + } + + /// Returns the number of functions for the Wasm module of the compiled function. + fn len_funcs(&self) -> usize { + self.translator.module.len_funcs() + } + + /// Returns the number of tables for the Wasm module of the compiled function. + fn len_tables(&self) -> usize { + self.translator.module.len_tables() + } + + /// Returns the number of linear memories for the Wasm module of the compiled function. + fn len_memories(&self) -> usize { + self.translator.module.len_memories() + } +} + +impl ErrorWithContext<'_> { + /// Returns the `u32` position of `instr` within the instruction sequence. + fn instr_pos(instr: Instr) -> u32 { + u32::from(ir::index::Instr::from(instr)) + } + + /// Returns the `u32` position of the error's `instr` within the instruction sequence. + fn pos(&self) -> u32 { + Self::instr_pos(self.error.instr) + } + + /// Resolves the error's `instr` to [`Instruction`]. + fn instr(&self) -> Instruction { + self.ctx.resolve_instr(self.error.instr) + } +} + +/// Checks if the invariants of Wasmi function translation are uphold. +/// +/// This function is called after Wasmi function translation has finished +/// and before the Wasmi engine's `CodeMap` is populated with the compiled +/// function. +/// +/// The checking is designed to only be performed for builds with +/// `debug-assertions` enabled due to massive translation overhead. +/// +/// # Errors +/// +/// If a Wasmi translation invariant is broken. +pub fn verify_translation_invariants(translator: &FuncTranslator) -> Result<(), ErrorWithContext> { + let checker = TranslationInvariantsChecker { translator }; + match checker.verify_translation_invariants() { + Ok(_) => Ok(()), + Err(error) => { + let ctx = ErrorContext { translator }; + Err(ErrorWithContext { ctx, error }) + } + } +} + +/// Encapsulates state required for translation invariants checking. +struct TranslationInvariantsChecker<'translator> { + /// The underlying Wasmi function translator which has already finished its job. + translator: &'translator FuncTranslator, +} + +impl TranslationInvariantsChecker<'_> { + /// Checks if the invariants of Wasmi function translation are uphold. + /// + /// Read more here: [`verify_translation_invariants`] + /// + /// # Errors + /// + /// If a Wasmi translation invariant is broken. + fn verify_translation_invariants(&self) -> Result<(), Error> { + todo!() + } +} diff --git a/crates/wasmi/src/engine/translator/instr_encoder.rs b/crates/wasmi/src/engine/translator/instr_encoder.rs index 5ed65e62f2..39de04aa07 100644 --- a/crates/wasmi/src/engine/translator/instr_encoder.rs +++ b/crates/wasmi/src/engine/translator/instr_encoder.rs @@ -99,7 +99,7 @@ impl Instr { #[derive(Debug, Default)] pub struct InstrEncoder { /// Already encoded [`Instruction`] words. - instrs: InstrSequence, + pub(super) instrs: InstrSequence, /// Unresolved and unpinned labels created during function translation. labels: LabelRegistry, /// The last [`Instruction`] created via [`InstrEncoder::push_instr`]. @@ -173,7 +173,7 @@ impl InstrSequence { /// # Panics /// /// If no [`Instruction`] is associated to the [`Instr`] for this [`InstrSequence`]. - fn get(&self, instr: Instr) -> &Instruction { + pub(super) fn get(&self, instr: Instr) -> &Instruction { &self.instrs[instr.into_usize()] } diff --git a/crates/wasmi/src/engine/translator/mod.rs b/crates/wasmi/src/engine/translator/mod.rs index d046c9f620..747d4725e9 100644 --- a/crates/wasmi/src/engine/translator/mod.rs +++ b/crates/wasmi/src/engine/translator/mod.rs @@ -17,6 +17,9 @@ mod visit_register; #[cfg(test)] mod tests; +#[cfg(debug_assertions)] +mod conditions; + use self::{ comparator::{ComparatorExt, ComparatorExtImm}, control_frame::{ @@ -577,6 +580,10 @@ impl WasmTranslator<'_> for FuncTranslator { let func_consts = self.alloc.stack.func_local_consts(); let instrs = self.alloc.instr_encoder.drain_instrs(); finalize(CompiledFuncEntity::new(len_registers, instrs, func_consts)); + #[cfg(debug_assertions)] + if let Err(err) = conditions::verify_translation_invariants(&self) { + panic!("{err}") + } Ok(self.into_allocations()) } } diff --git a/crates/wasmi/src/module/mod.rs b/crates/wasmi/src/module/mod.rs index 192c157a35..ec86312654 100644 --- a/crates/wasmi/src/module/mod.rs +++ b/crates/wasmi/src/module/mod.rs @@ -85,6 +85,29 @@ struct ModuleHeaderInner { element_segments: Box<[ElementSegment]>, } +#[cfg(debug_assertions)] +impl ModuleHeader { + /// Returns the number of global variables in `self`. + pub fn len_globals(&self) -> usize { + self.inner.globals.len() + } + + /// Returns the number of functions in `self`. + pub fn len_funcs(&self) -> usize { + self.inner.funcs.len() + } + + /// Returns the number of tables in `self`. + pub fn len_tables(&self) -> usize { + self.inner.tables.len() + } + + /// Returns the number of linear memories in `self`. + pub fn len_memories(&self) -> usize { + self.inner.memories.len() + } +} + impl ModuleHeader { /// Returns the [`Engine`] of the [`ModuleHeader`]. pub fn engine(&self) -> &EngineWeak { From cd8352f2aae535fab5908f37e86fba2249a98207 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 8 Oct 2024 16:51:31 +0200 Subject: [PATCH 2/3] make invariant checker return potentially multiple errors --- .../wasmi/src/engine/translator/conditions.rs | 81 +++++++++++++++---- 1 file changed, 67 insertions(+), 14 deletions(-) diff --git a/crates/wasmi/src/engine/translator/conditions.rs b/crates/wasmi/src/engine/translator/conditions.rs index 9e9f2c07e8..9ed3c47f1b 100644 --- a/crates/wasmi/src/engine/translator/conditions.rs +++ b/crates/wasmi/src/engine/translator/conditions.rs @@ -19,9 +19,58 @@ use core::{ cmp, fmt::{self, Display}, }; +use std::vec::Vec; + +/// A non-empty list of [`Error`]s. +pub struct ErrorList<'ctx> { + ctx: ErrorContext<'ctx>, + errors: Vec, +} + +impl<'ctx> ErrorList<'ctx> { + /// Creates a new [`ErrorList`]. + /// + /// # Panics + /// + /// If `errors` is empty. + fn new(ctx: ErrorContext<'ctx>, errors: Vec) -> Self { + assert!(!errors.is_empty()); + Self { ctx, errors } + } +} + +impl ErrorList<'_> { + /// Returns `true` if `self` contains at least one [`Error`]. + fn has_errors(&self) -> bool { + !self.errors.is_empty() + } + + /// Returns the number of [`Error`]s in `self`. + fn len_errors(&self) -> usize { + self.errors.len() + } +} + +impl Display for ErrorList<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if !self.has_errors() { + return writeln!(f, "all checked Wasmi translation invariants are uphold"); + } + writeln!( + f, + "encountered {} broken invariants for Wasmi translation:", + self.len_errors() + )?; + let ctx = self.ctx; + for (n, error) in self.errors.iter().cloned().enumerate() { + write!(f, "error({n}): {}", ErrorWithContext { ctx, error })?; + } + Ok(()) + } +} /// An error describing a broken Wasmi translation invariant. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct Error { /// The erraneous `Instruction` index. instr: Instr, @@ -30,7 +79,7 @@ pub struct Error { } /// The reason behind a broken Wasmi translation invariant. -#[derive(Debug)] +#[derive(Debug, Clone)] pub enum Reason { InvalidRegister { /// The invalid `Reg`. @@ -216,6 +265,7 @@ pub struct ErrorWithContext<'ctx> { } /// A context to populate an [`Error`] with more information for its [`Display`] implementation. +#[derive(Copy, Clone)] pub struct ErrorContext<'ctx> { /// The underlying Wasmi function translator that has already finished its job. translator: &'ctx FuncTranslator, @@ -286,15 +336,8 @@ impl ErrorWithContext<'_> { /// # Errors /// /// If a Wasmi translation invariant is broken. -pub fn verify_translation_invariants(translator: &FuncTranslator) -> Result<(), ErrorWithContext> { - let checker = TranslationInvariantsChecker { translator }; - match checker.verify_translation_invariants() { - Ok(_) => Ok(()), - Err(error) => { - let ctx = ErrorContext { translator }; - Err(ErrorWithContext { ctx, error }) - } - } +pub fn verify_translation_invariants(translator: &FuncTranslator) -> Result<(), ErrorList> { + TranslationInvariantsChecker { translator }.verify_translation_invariants() } /// Encapsulates state required for translation invariants checking. @@ -303,7 +346,7 @@ struct TranslationInvariantsChecker<'translator> { translator: &'translator FuncTranslator, } -impl TranslationInvariantsChecker<'_> { +impl<'translator> TranslationInvariantsChecker<'translator> { /// Checks if the invariants of Wasmi function translation are uphold. /// /// Read more here: [`verify_translation_invariants`] @@ -311,7 +354,17 @@ impl TranslationInvariantsChecker<'_> { /// # Errors /// /// If a Wasmi translation invariant is broken. - fn verify_translation_invariants(&self) -> Result<(), Error> { - todo!() + fn verify_translation_invariants(&self) -> Result<(), ErrorList<'translator>> { + let errors = Vec::new(); + // TODO: perform invariant checking + if errors.is_empty() { + return Ok(()); + } + Err(ErrorList::new( + ErrorContext { + translator: self.translator, + }, + errors, + )) } } From 880f917b7166f8ec36c67174e3b2f0233f01fde1 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 8 Oct 2024 16:53:57 +0200 Subject: [PATCH 3/3] add clarifying comment --- crates/wasmi/src/engine/translator/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/wasmi/src/engine/translator/mod.rs b/crates/wasmi/src/engine/translator/mod.rs index 747d4725e9..6e444fd176 100644 --- a/crates/wasmi/src/engine/translator/mod.rs +++ b/crates/wasmi/src/engine/translator/mod.rs @@ -582,6 +582,9 @@ impl WasmTranslator<'_> for FuncTranslator { finalize(CompiledFuncEntity::new(len_registers, instrs, func_consts)); #[cfg(debug_assertions)] if let Err(err) = conditions::verify_translation_invariants(&self) { + // Note: we do not propagate these errors to the caller as usual since + // breaking Wasmi translation invariants is considered a bug in Wasmi itself + // that should never occur if Wasmi translation works as intended. panic!("{err}") } Ok(self.into_allocations())