Respond to review

Author: keithw

.github/workflows/main.yml

@@ -154,8 +154,8 @@ jobs:
     - run: cargo test --locked -p wasm-encoder --all-features
     - run: cargo test -p wasm-smith --features wasmparser
     - run: cargo test -p wasm-smith --features component-model
-    - run: cargo test --locked -p wasmparser --features atomic
-    - run: cargo test --locked -F wasmparser/atomic
+    - run: cargo test --locked -p wasmparser --features try-op
+    - run: cargo test --locked -F wasmparser/try-op
 
   test_capi:
     name: Test the C API
@@ -278,7 +278,7 @@ jobs:
       - run: cargo check --no-default-features -p wasmparser --target x86_64-unknown-none --features validate,serde,prefer-btree-collections
       - run: cargo check --no-default-features -p wasmparser --features std
       - run: cargo check --no-default-features -p wasmparser --features validate
-      - run: cargo check --no-default-features -p wasmparser --features validate,atomic
+      - run: cargo check --no-default-features -p wasmparser --features validate,try-op
       - run: cargo check --no-default-features -p wasmparser --features features
       - run: cargo check --no-default-features -p wasmparser --features features,validate
       - run: cargo check --no-default-features -p wasmparser --features prefer-btree-collections

crates/wasmparser/Cargo.toml

@@ -90,4 +90,4 @@ simd = []
 
 # A feature that allows validating an operator atomically, leaving the validator
 # unchanged if the operator is invalid.
-atomic = []
+try-op = []

crates/wasmparser/src/validator/func.rs

@@ -124,11 +124,11 @@ impl<T: WasmModuleResources> FuncValidator<T> {
         while !reader.eof() {
             // In a debug build, verify that `rollback` successfully returns the
             // validator to its previous state after each (valid or invalid) operator.
-            #[cfg(all(debug_assertions, feature = "atomic"))]
+            #[cfg(all(debug_assertions, feature = "try-op"))]
             {
                 let snapshot = self.validator.clone();
                 let op = reader.peek_operator(&self.visitor(reader.original_position()))?;
-                self.validator.begin_atomic_op();
+                self.validator.begin_try_op();
                 let _ = self.op(reader.original_position(), &op);
                 self.validator.rollback();
                 self.validator.pop_push_log.clear();
@@ -208,19 +208,21 @@ arity mismatch in validation
 
     /// Validates the next operator in a function.
     ///
-    /// This functions is expected to be called once-per-operator in a
+    /// This function is expected to be called once-per-operator in a
     /// WebAssembly function. Each operator's offset in the original binary and
     /// the operator itself are passed to this function to provide more useful
-    /// error messages.
+    /// error messages. On error, the validator may be left in an undefined
+    /// state and should not be reused.
     pub fn op(&mut self, offset: usize, operator: &Operator<'_>) -> Result<()> {
         self.visitor(offset).visit_operator(operator)
     }
 
     /// Validates the next operator in a function, rolling back the validator
-    /// to its previous state if this is unsuccesful.
-    #[cfg(feature = "atomic")]
-    pub fn atomic_op(&mut self, offset: usize, operator: &Operator<'_>) -> Result<()> {
-        self.validator.begin_atomic_op();
+    /// to its previous state if this is unsuccesful. The validator may be reused
+    /// even after an error.
+    #[cfg(feature = "try-op")]
+    pub fn try_op(&mut self, offset: usize, operator: &Operator<'_>) -> Result<()> {
+        self.validator.begin_try_op();
         let res = self.op(offset, operator);
         if res.is_ok() {
             self.validator.commit();

crates/wasmparser/src/validator/operators.rs

@@ -38,6 +38,15 @@ use core::{cmp, iter, mem};
 #[cfg(feature = "simd")]
 mod simd;
 
+#[cfg(feature = "try-op")]
+mod transaction;
+#[cfg(not(feature = "try-op"))]
+mod transaction_disabled;
+#[cfg(not(feature = "try-op"))]
+use transaction_disabled as transaction;
+
+use transaction::{RollbackLogAllocations, Transaction};
+
 #[derive(Clone, PartialEq)]
 pub(crate) struct OperatorValidator {
     pub(super) locals: Locals,
@@ -65,128 +74,12 @@ pub(crate) struct OperatorValidator {
     #[cfg(debug_assertions)]
     pub(crate) pop_push_log: Vec<bool>,
 
-    /// When "atomic" validation of an operator is pending, this is a trace
+    /// When "try-op" validation of an operator is pending, this is a trace
     /// of discarded info that can restore the OperatorValidator to its
     /// pre-operator state if necessary.
-    #[cfg(feature = "atomic")]
     transaction: Transaction,
 }
 
-#[cfg(feature = "atomic")]
-#[derive(Clone, PartialEq)]
-enum Transaction {
-    Active(RollbackLog),
-    Inactive(RollbackLogAllocations),
-}
-
-#[cfg(feature = "atomic")]
-#[derive(Clone, Default, PartialEq)]
-struct RollbackLog {
-    /// A trace of operands popped. Pushes are recorded as `None`.
-    operands: Vec<Option<MaybeType>>,
-
-    /// A trace of frames popped. Pushes are recorded as `None`.
-    frames: Vec<Option<Frame>>,
-
-    /// A trace of local init indices reset at the end of a frame.
-    inits: Vec<u32>,
-
-    /// The local init height when transaction was begun.
-    init_height: usize,
-
-    /// Whether the current frame was made unreachable.
-    unreachable: bool,
-}
-
-#[cfg(feature = "atomic")]
-#[derive(Clone, Default, PartialEq)]
-struct RollbackLogAllocations {
-    operands: Vec<Option<MaybeType>>,
-    frames: Vec<Option<Frame>>,
-    inits: Vec<u32>,
-}
-
-#[cfg(feature = "atomic")]
-impl Transaction {
-    fn begin(&mut self, init_height: usize) {
-        match self {
-            Transaction::Active(_) => panic!("transaction already in progress"),
-            Transaction::Inactive(allocs) => {
-                *self = Transaction::Active(RollbackLog::new(init_height, mem::take(allocs)))
-            }
-        }
-    }
-
-    fn end(&mut self) {
-        if let Transaction::Active(log) = self {
-            *self = Transaction::Inactive(mem::take(log).into_allocations())
-        }
-    }
-
-    fn into_allocations(self) -> RollbackLogAllocations {
-        match self {
-            Transaction::Active(log) => log.into_allocations(),
-            Transaction::Inactive(allocs) => allocs,
-        }
-    }
-
-    fn map(&mut self, f: impl FnOnce(&mut RollbackLog)) {
-        if let Transaction::Active(log) = self {
-            f(log);
-        }
-    }
-}
-
-#[cfg(feature = "atomic")]
-impl RollbackLog {
-    fn new(init_height: usize, allocs: RollbackLogAllocations) -> Self {
-        let RollbackLogAllocations {
-            operands,
-            frames,
-            inits,
-        } = allocs;
-        debug_assert!(operands.is_empty());
-        debug_assert!(frames.is_empty());
-        debug_assert!(inits.is_empty());
-        Self {
-            operands,
-            frames,
-            inits,
-            init_height,
-            unreachable: false,
-        }
-    }
-
-    fn into_allocations(self) -> RollbackLogAllocations {
-        fn clear<T>(mut tmp: Vec<T>) -> Vec<T> {
-            tmp.clear();
-            tmp
-        }
-        RollbackLogAllocations {
-            operands: clear(self.operands),
-            frames: clear(self.frames),
-            inits: clear(self.inits),
-        }
-    }
-
-    fn record_push(&mut self) {
-        self.operands.push(None);
-    }
-
-    fn record_pop(&mut self, ty: MaybeType) {
-        self.operands.push(Some(ty));
-    }
-
-    fn push_ctrl(&mut self) {
-        self.frames.push(None);
-    }
-
-    fn pop_ctrl(&mut self, frame: Frame, inits: Vec<u32>) {
-        self.frames.push(Some(frame));
-        self.inits.extend(inits);
-    }
-}
-
 /// Captures the initialization of non-defaultable locals.
 #[derive(Clone, PartialEq)]
 struct LocalInits {
@@ -353,7 +246,6 @@ pub struct OperatorValidatorAllocations {
     local_inits: LocalInits,
     locals_first: Vec<ValType>,
     locals_uncached: Vec<(u32, ValType)>,
-    #[cfg(feature = "atomic")]
     rollback_log: RollbackLogAllocations,
 }
 
@@ -460,7 +352,6 @@ impl OperatorValidator {
             local_inits,
             locals_first,
             locals_uncached,
-            #[cfg(feature = "atomic")]
             rollback_log,
         } = allocs;
         debug_assert!(popped_types_tmp.is_empty());
@@ -483,8 +374,7 @@ impl OperatorValidator {
             shared: false,
             #[cfg(debug_assertions)]
             pop_push_log: vec![],
-            #[cfg(feature = "atomic")]
-            transaction: Transaction::Inactive(rollback_log),
+            transaction: Transaction::new(rollback_log),
         }
     }
 
@@ -672,39 +562,46 @@ impl OperatorValidator {
             },
             locals_first: clear(self.locals.first),
             locals_uncached: clear(self.locals.uncached),
-            #[cfg(feature = "atomic")]
             rollback_log: self.transaction.into_allocations(),
         }
     }
 
-    fn record_pop(&mut self) {
+    // records a pop that mutated the operand stack
+    fn record_pop(&mut self, ty: MaybeType) {
+        self.transaction.map(|log| log.record_pop(ty));
+        self.record_any_pop();
+    }
+
+    // records any pop, including a Bottom synthesized from an empty polymorphic operand stack
+    fn record_any_pop(&mut self) {
         #[cfg(debug_assertions)]
         {
             self.pop_push_log.push(false);
         }
     }
 
     fn record_push(&mut self) {
+        self.transaction.map(|log| log.record_push());
         #[cfg(debug_assertions)]
         {
             self.pop_push_log.push(true);
         }
     }
 
-    #[cfg(feature = "atomic")]
-    pub(super) fn begin_atomic_op(&mut self) {
+    #[allow(dead_code)]
+    pub(super) fn begin_try_op(&mut self) {
         self.transaction.begin(self.local_inits.height());
     }
 
-    #[cfg(feature = "atomic")]
+    #[allow(dead_code)]
     pub(super) fn commit(&mut self) {
         self.transaction.end();
     }
 
-    /// Reverse the actions in the rollback log. This is used by `FuncValidator::atomic_op()`
+    /// Reverse the actions in the rollback log. This is used by `FuncValidator::try_op()`
     /// if validating the operator fails. The rollback log is sufficient to handle
     /// the mutations of any individual operator (but not necessarily multiple operators).
-    #[cfg(feature = "atomic")]
+    #[cfg(feature = "try-op")]
     pub(super) fn rollback(&mut self) {
         let Transaction::Active(rollback_log) = &self.transaction else {
             panic!("no transaction pending");
@@ -793,8 +690,6 @@ where
 
         self.operands.push(maybe_ty);
         self.record_push();
-        #[cfg(feature = "atomic")]
-        self.transaction.map(|log| log.record_push());
         Ok(())
     }
 
@@ -919,10 +814,7 @@ where
                 if Some(actual_ty) == expected {
                     if let Some(control) = self.control.last() {
                         if self.operands.len() >= control.height {
-                            #[cfg(feature = "atomic")]
-                            self.transaction
-                                .map(|log| log.record_pop(MaybeType::Known(actual_ty)));
-                            self.record_pop();
+                            self.record_pop(MaybeType::Known(actual_ty));
                             return Ok(MaybeType::Known(actual_ty));
                         }
                     }
@@ -947,6 +839,7 @@ where
         self.operands.extend(popped);
         let control = self.control.last().unwrap();
         let actual = if self.operands.len() == control.height && control.unreachable {
+            self.record_any_pop();
             MaybeType::Bottom
         } else {
             if self.operands.len() == control.height {
@@ -960,8 +853,7 @@ where
                 )
             } else {
                 let ty = self.operands.pop().unwrap();
-                #[cfg(feature = "atomic")]
-                self.transaction.map(|log| log.record_pop(ty));
+                self.record_pop(ty);
                 ty
             }
         };
@@ -1022,7 +914,6 @@ where
                 }
             }
         }
-        self.record_pop();
         Ok(actual)
     }
 
@@ -1135,24 +1026,20 @@ where
     /// Flags the current control frame as unreachable, additionally truncating
     /// the currently active operand stack.
     fn unreachable(&mut self) -> Result<()> {
-        #[cfg(feature = "atomic")]
         if !self.control.last().unwrap().unreachable {
-            self.transaction.map(|log| log.unreachable = true);
+            self.transaction.map(|log| log.set_unreachable());
         }
 
         let control = self.control.last_mut().unwrap();
         control.unreachable = true;
         let new_height = control.height;
 
-        #[cfg(feature = "atomic")]
-        {
-            let ops = self.operands.split_off(new_height);
-            self.transaction.map(|log| {
-                for op in ops.iter().rev() {
-                    log.record_pop(*op);
-                }
-            });
-        }
+        let operands = self.operands.split_off(new_height);
+        self.transaction.map(|log| {
+            for op in operands.iter().rev() {
+                log.record_pop(*op);
+            }
+        });
 
         self.operands.truncate(new_height);
         Ok(())
@@ -1188,7 +1075,6 @@ where
             unreachable: false,
             init_height,
         });
-        #[cfg(feature = "atomic")]
         self.transaction.map(|log| log.push_ctrl());
     }
 
@@ -1221,7 +1107,6 @@ where
         // And then we can remove it and reset_locals.
         let frame = self.control.pop().unwrap();
         let _inits = self.local_inits.pop_ctrl(frame.init_height);
-        #[cfg(feature = "atomic")]
         self.transaction.map(|log| log.pop_ctrl(frame, _inits));
 
         Ok(frame)