From 2af0eaff2d701f39181eb77475520e1679e1fe55 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 11 Aug 2022 10:15:58 -0700 Subject: [PATCH] Fix an accidental regression from #697 This commit fixes a regression introduced in #697 which could cause a panic when validating an invalid wasm module. The issue introduced was that a [check that the control stack is non-empty][check] was lost in the refactoring of the operator validator. This check ran for every single operator and verified that there was a frame on the control stack that the operator could be attached to, otherwise it means instructions were present after the end of the function. The current design of `VisitOperator` doesn't have an easy place to slot this in so I decided to fix this via a different route than was implemented before. Anything which operates on the control stack now checks to see if it's empty instead of asserting it's non-empty. Operators which don't touch the control stack are then checked by ensuring that the `end` opcode which emptied the control stack was the last operator processed in the function. This exposed a minor issue where when validating const expressions the offset that was passed in as the final offset of the expression was actually the first offset of the expression. Additionally this adds some tests to exercise this corner case (unsure why the spec test suite doesn't have them already!) [check]: https://github.com/bytecodealliance/wasm-tools/blob/8732e0bc8a579cd9f15d9134af997c5d3d95af5d/crates/wasmparser/src/validator/operators.rs#L581-L583 --- crates/wasmparser/src/validator/core.rs | 17 ++---- crates/wasmparser/src/validator/operators.rs | 27 ++++++++- fuzz/fuzz_targets/validate.rs | 8 ++- tests/local/invalid/func.wast | 62 ++++++++++++++++++++ 4 files changed, 99 insertions(+), 15 deletions(-) create mode 100644 tests/local/invalid/func.wast diff --git a/crates/wasmparser/src/validator/core.rs b/crates/wasmparser/src/validator/core.rs index b5f65dbc2b..d48f56987a 100644 --- a/crates/wasmparser/src/validator/core.rs +++ b/crates/wasmparser/src/validator/core.rs @@ -136,13 +136,7 @@ impl ModuleState { ) -> Result<()> { self.module .check_global_type(&global.ty, features, offset)?; - self.check_const_expr( - &global.init_expr, - global.ty.content_type, - features, - types, - offset, - )?; + self.check_const_expr(&global.init_expr, global.ty.content_type, features, types)?; self.module.assert_mut().globals.push(global.ty); Ok(()) } @@ -161,7 +155,7 @@ impl ModuleState { offset_expr, } => { let ty = self.module.memory_at(memory_index, offset)?.index_type(); - self.check_const_expr(&offset_expr, ty, features, types, offset) + self.check_const_expr(&offset_expr, ty, features, types) } } } @@ -197,7 +191,7 @@ impl ModuleState { )); } - self.check_const_expr(&offset_expr, ValType::I32, features, types, offset)?; + self.check_const_expr(&offset_expr, ValType::I32, features, types)?; } ElementKind::Passive | ElementKind::Declared => { if !features.bulk_memory { @@ -219,7 +213,7 @@ impl ModuleState { let offset = items.original_position(); match items.read()? { ElementItem::Expr(expr) => { - self.check_const_expr(&expr, e.ty, features, types, offset)?; + self.check_const_expr(&expr, e.ty, features, types)?; } ElementItem::Func(f) => { if e.ty != ValType::FuncRef { @@ -244,7 +238,6 @@ impl ModuleState { expected_ty: ValType, features: &WasmFeatures, types: &TypeList, - offset: usize, ) -> Result<()> { let mut ops = expr.get_operators_reader(); let mut validator = OperatorValidator::new_const_expr(features, expected_ty); @@ -333,7 +326,7 @@ impl ModuleState { .visit_operator(offset, &op)?; } - validator.finish(offset)?; + validator.finish(ops.original_position())?; // See comment in `RefFunc` above for why this is an assert. assert!(!uninserted_funcref); diff --git a/crates/wasmparser/src/validator/operators.rs b/crates/wasmparser/src/validator/operators.rs index 96662f1211..6c94ace964 100644 --- a/crates/wasmparser/src/validator/operators.rs +++ b/crates/wasmparser/src/validator/operators.rs @@ -69,6 +69,10 @@ pub(crate) struct OperatorValidator { control: Vec, /// The `operands` is the current type stack. operands: Vec>, + + /// Offset of the `end` instruction which emptied the `control` stack, which + /// must be the end of the function. + end_which_emptied_control: Option, } // This structure corresponds to `ctrl_frame` as specified at in the validation @@ -128,6 +132,7 @@ impl OperatorValidator { height: 0, unreachable: false, }], + end_which_emptied_control: None, }; let locals = OperatorValidatorTemp { inner: &mut ret, @@ -159,6 +164,7 @@ impl OperatorValidator { height: 0, unreachable: false, }], + end_which_emptied_control: None, } } @@ -209,8 +215,15 @@ impl OperatorValidator { "control frames remain at end of function: END opcode expected" ); } + if offset != self.end_which_emptied_control.unwrap() + 1 { + return Err(self.err_beyond_end(offset)); + } Ok(()) } + + fn err_beyond_end(&self, offset: usize) -> BinaryReaderError { + format_op_err!(offset, "operators remaining after end of function") + } } impl Deref for OperatorValidatorTemp<'_, '_, R> { @@ -265,7 +278,10 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R /// expected and a type was successfully popped, but its exact type is /// indeterminate because the current block is unreachable. fn pop_operand(&mut self, offset: usize, expected: Option) -> Result> { - let control = self.control.last().unwrap(); + let control = match self.control.last() { + Some(c) => c, + None => return Err(self.err_beyond_end(offset)), + }; let actual = if self.operands.len() == control.height { if control.unreachable { None @@ -358,7 +374,10 @@ impl<'resources, R: WasmModuleResources> OperatorValidatorTemp<'_, 'resources, R fn pop_ctrl(&mut self, offset: usize) -> Result { // Read the expected type and expected height of the operand stack the // end of the frame. - let frame = self.control.last().unwrap(); + let frame = match self.control.last() { + Some(f) => f, + None => return Err(self.err_beyond_end(offset)), + }; let ty = frame.block_type; let height = frame.height; @@ -997,6 +1016,10 @@ where for ty in self.results(offset, frame.block_type)? { self.push_operand(offset, ty)?; } + + if self.control.is_empty() && self.end_which_emptied_control.is_none() { + self.end_which_emptied_control = Some(offset); + } Ok(()) } fn visit_br(&mut self, offset: usize, relative_depth: u32) -> Self::Output { diff --git a/fuzz/fuzz_targets/validate.rs b/fuzz/fuzz_targets/validate.rs index 7d2e5b538a..229d324bc6 100644 --- a/fuzz/fuzz_targets/validate.rs +++ b/fuzz/fuzz_targets/validate.rs @@ -4,6 +4,7 @@ use libfuzzer_sys::*; use wasmparser::{Validator, WasmFeatures}; fuzz_target!(|data: &[u8]| { + drop(env_logger::try_init()); let byte1 = match data.get(0) { Some(byte) => byte, None => return, @@ -31,5 +32,10 @@ fuzz_target!(|data: &[u8]| { sign_extension: (byte2 & 0b1000_0000) != 0, }); - drop(validator.validate_all(&data[2..])); + let wasm = &data[2..]; + if log::log_enabled!(log::Level::Debug) { + log::debug!("writing input to `test.wasm`"); + std::fs::write("test.wasm", wasm).unwrap(); + } + drop(validator.validate_all(wasm)); }); diff --git a/tests/local/invalid/func.wast b/tests/local/invalid/func.wast new file mode 100644 index 0000000000..71c01c5943 --- /dev/null +++ b/tests/local/invalid/func.wast @@ -0,0 +1,62 @@ +(assert_invalid + (module binary + "\00asm" "\01\00\00\00" ;; magic header + + "\01\04" ;; type section + "\01" ;; 1 count + "\60\00\00" ;; no params or results + + "\03\02" ;; func section + "\01" ;; 1 count + "\00" ;; type 0 + + "\0a\05" ;; code section + "\01" ;; 1 count + "\03" ;; size of function + "\00" ;; no locals + "\0b" ;; end + "\01" ;; nop + ) + "operators remaining after end of function") + +(assert_invalid + (module binary + "\00asm" "\01\00\00\00" ;; magic header + + "\01\04" ;; type section + "\01" ;; 1 count + "\60\00\00" ;; no params or results + + "\03\02" ;; func section + "\01" ;; 1 count + "\00" ;; type 0 + + "\0a\05" ;; code section + "\01" ;; 1 count + "\03" ;; size of function + "\00" ;; no locals + "\0b" ;; end + "\9d" ;; f64.trunc + ) + "operators remaining after end of function") + +(assert_invalid + (module binary + "\00asm" "\01\00\00\00" ;; magic header + + "\01\04" ;; type section + "\01" ;; 1 count + "\60\00\00" ;; no params or results + + "\03\02" ;; func section + "\01" ;; 1 count + "\00" ;; type 0 + + "\0a\05" ;; code section + "\01" ;; 1 count + "\03" ;; size of function + "\00" ;; no locals + "\0b" ;; end + "\0b" ;; end + ) + "operators remaining after end of function")