diff --git a/crates/wasm-smith/src/core/code_builder.rs b/crates/wasm-smith/src/core/code_builder.rs index 1bc09008ad..6897c8b636 100644 --- a/crates/wasm-smith/src/core/code_builder.rs +++ b/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(()) } diff --git a/crates/wasmparser/src/validator/operators.rs b/crates/wasmparser/src/validator/operators.rs index 654492d2a5..ae034d19c8 100644 --- a/crates/wasmparser/src/validator/operators.rs +++ b/crates/wasmparser/src/validator/operators.rs @@ -187,13 +187,6 @@ impl MaybeType { Self::Bot | Self::HeapBot => None, } } - - fn or(self, alternative: impl Into) -> MaybeType { - match self { - Self::Type(_) => self, - Self::Bot | Self::HeapBot => alternative.into(), - } - } } impl OperatorValidator { @@ -409,11 +402,6 @@ impl 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, - 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(()) } diff --git a/tests/local/gc/invalid.wast b/tests/local/gc/invalid.wast new file mode 100644 index 0000000000..3f3342b5aa --- /dev/null +++ b/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" + ) diff --git a/tests/local/unreachable-valid.wast b/tests/local/unreachable-valid.wast new file mode 100644 index 0000000000..4ab4d8b7c5 --- /dev/null +++ b/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") diff --git a/tests/snapshots/local/unreachable-valid.wast/0.print b/tests/snapshots/local/unreachable-valid.wast/0.print new file mode 100644 index 0000000000..4014aafa03 --- /dev/null +++ b/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)) +) \ No newline at end of file