Fix an accidental regression from bytecodealliance#697

This commit fixes a regression introduced in bytecodealliance#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

@@ -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);

crates/wasmparser/src/validator/operators.rs

@@ -69,6 +69,10 @@ pub(crate) struct OperatorValidator {
     control: Vec<Frame>,
     /// The `operands` is the current type stack.
     operands: Vec<Option<ValType>>,
+
+    /// Offset of the `end` instruction which emptied the `control` stack, which
+    /// must be the end of the function.
+    end_which_emptied_control: Option<usize>,
 }
 
 // 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<R> 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<ValType>) -> Result<Option<ValType>> {
-        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<Frame> {
         // 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 {

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));
 });

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")