Fix bugs involving the combination of branching and subtyping (#1403)

* wasmparser: Fix behavior of branching to labels and subtyping

Addresses issues discussed in WebAssembly/gc#516

* wasm-smith: Properly erase subtype information after conditional branches

When, for example, `[a i32]` is on the stack and we have a `br_if @label` where
`@label`'s type is `[b]` and `a <: b`, then whole `br_if` is typed `[b i32] ->
[b]`. Because `a <: b` the `br_if` is valid. However, `a <: b` does *not* mean
that the instruction results in `[a]`. It "erases" the `a` and pushes a `b`.

This addresses WebAssembly/gc#516 in `wasm-smith`.

Author: fitzgen

crates/wasm-smith/src/core/code_builder.rs

@@ -922,6 +922,33 @@ impl CodeBuilder<'_> {
         self.allocs.operands.push(ty);
     }
 
+    fn pop_label_types(&mut self, module: &Module, target: u32) {
+        let target = usize::try_from(target).unwrap();
+        let control = &self.allocs.controls[self.allocs.controls.len() - 1 - target];
+        debug_assert!(self.label_types_on_stack(module, control));
+        self.allocs
+            .operands
+            .truncate(self.allocs.operands.len() - control.label_types().len());
+    }
+
+    fn push_label_types(&mut self, target: u32) {
+        let target = usize::try_from(target).unwrap();
+        let control = &self.allocs.controls[self.allocs.controls.len() - 1 - target];
+        self.allocs
+            .operands
+            .extend(control.label_types().iter().copied().map(Some));
+    }
+
+    /// Pop the target label types, and then push them again.
+    ///
+    /// This is not a no-op due to subtyping: if we have a `T <: U` on the
+    /// stack, and the target label's type is `[U]`, then this will erase the
+    /// information about `T` and subsequent operations may only operate on `U`.
+    fn pop_push_label_types(&mut self, module: &Module, target: u32) {
+        self.pop_label_types(module, target);
+        self.push_label_types(target)
+    }
+
     fn label_types_on_stack(&self, module: &Module, to_check: &Control) -> bool {
         self.types_on_stack(module, to_check.label_types())
     }
@@ -1710,10 +1737,9 @@ fn br(
         .filter(|(_, l)| builder.label_types_on_stack(module, l))
         .nth(i)
         .unwrap();
-    let control = &builder.allocs.controls[builder.allocs.controls.len() - 1 - target];
-    let tys = control.label_types().to_vec();
-    builder.pop_operands(module, &tys);
-    instructions.push(Instruction::Br(target as u32));
+    let target = u32::try_from(target).unwrap();
+    builder.pop_label_types(module, target);
+    instructions.push(Instruction::Br(target));
     Ok(())
 }
 
@@ -1757,7 +1783,9 @@ fn br_if(
         .filter(|(_, l)| builder.label_types_on_stack(module, l))
         .nth(i)
         .unwrap();
-    instructions.push(Instruction::BrIf(target as u32));
+    let target = u32::try_from(target).unwrap();
+    builder.pop_push_label_types(module, target);
+    instructions.push(Instruction::BrIf(target));
     Ok(())
 }
 
@@ -2203,13 +2231,15 @@ fn br_on_null(
         .filter(|(_, l)| builder.label_types_on_stack(module, l))
         .nth(i)
         .unwrap();
+    let target = u32::try_from(target).unwrap();
 
+    builder.pop_push_label_types(module, target);
     builder.push_operands(&[ValType::Ref(RefType {
         nullable: false,
         heap_type,
     })]);
 
-    instructions.push(Instruction::BrOnNull(u32::try_from(target).unwrap()));
+    instructions.push(Instruction::BrOnNull(target));
     Ok(())
 }
 
@@ -2270,9 +2300,11 @@ fn br_on_non_null(
         .filter(|(_, l)| is_valid_br_on_non_null_control(module, l, builder))
         .nth(i)
         .unwrap();
+    let target = u32::try_from(target).unwrap();
 
+    builder.pop_push_label_types(module, target);
     builder.pop_ref_type();
-    instructions.push(Instruction::BrOnNonNull(u32::try_from(target).unwrap()));
+    instructions.push(Instruction::BrOnNonNull(target));
     Ok(())
 }
 
@@ -2354,6 +2386,7 @@ fn br_on_cast(
         .unwrap();
     let relative_depth = u32::try_from(relative_depth).unwrap();
 
+    let num_label_types = control.label_types().len();
     let to_ref_type = match control.label_types().last() {
         Some(ValType::Ref(r)) => *r,
         _ => unreachable!(),
@@ -2363,6 +2396,15 @@ fn br_on_cast(
     let from_ref_type = from_ref_type.unwrap_or(to_ref_type);
     let from_ref_type = module.arbitrary_super_type_of_ref_type(u, from_ref_type)?;
 
+    // Do `pop_push_label_types` but without its debug assert that the types are
+    // on the stack, since we know that we have a `from_ref_type` but the label
+    // requires a `to_ref_type`.
+    for _ in 0..num_label_types {
+        builder.pop_operand();
+    }
+    builder.push_label_types(relative_depth);
+
+    // Replace the label's `to_ref_type` with the type difference.
     builder.pop_operand();
     builder.push_operands(&[ValType::Ref(ref_type_difference(
         from_ref_type,
@@ -2433,7 +2475,7 @@ fn br_on_cast_fail(
     debug_assert!(n > 0);
 
     let i = u.int_in_range(0..=n - 1)?;
-    let (target, control) = builder
+    let (relative_depth, control) = builder
         .allocs
         .controls
         .iter()
@@ -2442,6 +2484,7 @@ fn br_on_cast_fail(
         .filter(|(_, l)| is_valid_br_on_cast_fail_control(module, builder, l, from_ref_type))
         .nth(i)
         .unwrap();
+    let relative_depth = u32::try_from(relative_depth).unwrap();
 
     let from_ref_type =
         from_ref_type.unwrap_or_else(|| match control.label_types().last().unwrap() {
@@ -2450,13 +2493,16 @@ fn br_on_cast_fail(
         });
     let to_ref_type = module.arbitrary_matching_ref_type(u, from_ref_type)?;
 
+    // Pop-push the label types and then replace its last reference type with
+    // our `to_ref_type`.
+    builder.pop_push_label_types(module, relative_depth);
     builder.pop_operand();
     builder.push_operand(Some(ValType::Ref(to_ref_type)));
 
     instructions.push(Instruction::BrOnCastFail {
         from_ref_type,
         to_ref_type,
-        relative_depth: u32::try_from(target).unwrap(),
+        relative_depth,
     });
     Ok(())
 }

crates/wasmparser/src/validator/operators.rs

@@ -187,13 +187,6 @@ impl MaybeType {
             Self::Bot | Self::HeapBot => None,
         }
     }
-
-    fn or(self, alternative: impl Into<MaybeType>) -> MaybeType {
-        match self {
-            Self::Type(_) => self,
-            Self::Bot | Self::HeapBot => alternative.into(),
-        }
-    }
 }
 
 impl OperatorValidator {
@@ -409,11 +402,6 @@ impl<R> DerefMut for OperatorValidatorTemp<'_, '_, R> {
     }
 }
 
-enum PopPushBottomBehavior {
-    PushBottom,
-    OrExpected,
-}
-
 impl<'resources, R> OperatorValidatorTemp<'_, 'resources, R>
 where
     R: WasmModuleResources,
@@ -482,41 +470,12 @@ where
     fn pop_push_label_types(
         &mut self,
         label_types: impl PreciseIterator<Item = ValType>,
-        bottom_behavior: PopPushBottomBehavior,
     ) -> Result<()> {
-        // When popping off the expected types from the stack for a branch
-        // label, the actual types might be subtypes of those expected types,
-        // and we don't want to accidentally erase that type information by
-        // pushing the label's expected types, so we have to save the actual
-        // popped types.
-
-        debug_assert!(self.popped_types_tmp.is_empty());
-        self.popped_types_tmp.reserve(label_types.len());
-        for expected_ty in label_types.rev() {
-            let actual_ty = self.pop_operand(Some(expected_ty))?;
-
-            let actual_ty = match bottom_behavior {
-                // However, in addition, if we pop bottom, then we generally
-                // don't want to push bottom again! That would allow for the
-                // "same" stack slot to be used as an `i32` in one unreachable
-                // instruction and as an `i64` in a different unreachable
-                // instruction, which leads to invalid test cases to be treated
-                // as valid.
-                PopPushBottomBehavior::OrExpected => actual_ty.or(expected_ty),
-                // The only exception to the above is when we are handling
-                // `br_table` and we aren't logically pushing the *same* value
-                // back to the stack again, but instead resetting the stack to
-                // its previous state, as if we didn't branch to the previous
-                // target. In this case, we can interpret bottom as `i32` for
-                // one branch target, and as `i64` for another branch target, so
-                // we really do want to re-push bottom again.
-                PopPushBottomBehavior::PushBottom => actual_ty,
-            };
-
-            self.popped_types_tmp.push(actual_ty);
+        for ty in label_types.clone().rev() {
+            self.pop_operand(Some(ty))?;
         }
-        for ty in self.inner.popped_types_tmp.drain(..).rev() {
-            self.inner.operands.push(ty.into());
+        for ty in label_types {
+            self.push_operand(ty)?;
         }
         Ok(())
     }
@@ -1473,7 +1432,7 @@ where
         self.pop_operand(Some(ValType::I32))?;
         let (ty, kind) = self.jump(relative_depth)?;
         let label_types = self.label_types(ty, kind)?;
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         Ok(())
     }
     fn visit_br_table(&mut self, table: BrTable) -> Self::Output {
@@ -1490,7 +1449,16 @@ where
                     "type mismatch: br_table target labels have different number of types"
                 );
             }
-            self.pop_push_label_types(label_tys, PopPushBottomBehavior::PushBottom)?;
+
+            debug_assert!(self.popped_types_tmp.is_empty());
+            self.popped_types_tmp.reserve(label_tys.len());
+            for expected_ty in label_tys.rev() {
+                let actual_ty = self.pop_operand(Some(expected_ty))?;
+                self.popped_types_tmp.push(actual_ty);
+            }
+            for ty in self.inner.popped_types_tmp.drain(..).rev() {
+                self.inner.operands.push(ty.into());
+            }
         }
         for ty in default_types.rev() {
             self.pop_operand(Some(ty))?;
@@ -2471,7 +2439,7 @@ where
         };
         let (ft, kind) = self.jump(relative_depth)?;
         let label_types = self.label_types(ft, kind)?;
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         self.push_operand(ref_ty)?;
         Ok(())
     }
@@ -2506,7 +2474,7 @@ where
             ),
         }
 
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         Ok(())
     }
     fn visit_ref_is_null(&mut self) -> Self::Output {
@@ -3940,7 +3908,7 @@ where
             ),
         };
 
-        self.pop_push_label_types(label_types, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_types)?;
         let diff_ty = RefType::difference(from_ref_type, to_ref_type);
         self.push_operand(diff_ty)?;
         Ok(())
@@ -3984,7 +3952,7 @@ where
             ),
         }
 
-        self.pop_push_label_types(label_tys, PopPushBottomBehavior::OrExpected)?;
+        self.pop_push_label_types(label_tys)?;
         self.push_operand(to_ref_type)?;
         Ok(())
     }

tests/local/gc/invalid.wast

@@ -0,0 +1,31 @@
+;; https://github.com/WebAssembly/gc/issues/516
+
+(assert_invalid
+  (module
+    (func (param anyref) (result anyref)
+      ref.null struct
+      local.get 0
+      ;; The label pushes an anyref, even though we really have a structref.
+      br_on_null 0
+      drop
+      br_on_cast_fail 0  structref structref
+    )
+  )
+  "type mismatch: expected structref, found anyref"
+)
+
+(assert_invalid
+  (module
+    (func (param anyref) (result anyref)
+      ref.null struct
+      ;; Adding an `unreachable` shouldn't change the fact that the `br_on_null`
+      ;; pushes an `anyref` and the `br_on_cast_fail` expects a `structref`.
+      unreachable
+      local.get 0
+      br_on_null 0
+      drop
+      br_on_cast_fail 0  structref structref
+    )
+  )
+ "type mismatch: expected structref, found anyref"
+ )

tests/local/unreachable-valid.wast

@@ -0,0 +1,11 @@
+(module
+  (func (export "f")
+    f32.const 1.0
+    f32.const 2.0
+    br 0
+    i32.add
+    drop
+  )
+)
+
+(assert_trap (invoke "f") "unreachable")

tests/snapshots/local/unreachable-valid.wast/0.print

@@ -0,0 +1,11 @@
+(module
+  (type (;0;) (func))
+  (func (;0;) (type 0)
+    f32.const 0x1p+0 (;=1;)
+    f32.const 0x1p+1 (;=2;)
+    br 0 (;@0;)
+    i32.add
+    drop
+  )
+  (export "f" (func 0))
+)