From 276bc6ad2e903844c2914d35a3c3c037930aec3b Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Tue, 3 Jan 2023 14:04:18 -0800 Subject: [PATCH] cranelift-wasm: Better track reachability after translating loads (#5511) We can sometimes statically determine that a given load will unconditionally trap. When this happens, we emit an unconditional trap, and we need to stop adding new instructions to the block. This commit introduces a `Reachability` type that is like `Option` but specifically for communicating reachability and is marked `must_use` to receivers to handle transitions from reachable to unreachable states. Additionally, adds handling of reachable -> unreachable state transitions to some SIMD op translations that weren't checking for it. Fixes #5455 Fixes #5456 --- cranelift/wasm/src/code_translator.rs | 184 ++++++++++++------ .../wasm/src/code_translator/bounds_checks.rs | 24 +-- 2 files changed, 141 insertions(+), 67 deletions(-) diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 2250fb3cff..0d86ffa7b2 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -97,8 +97,8 @@ use std::convert::TryFrom; use std::vec::Vec; use wasmparser::{FuncValidator, MemArg, Operator, WasmModuleResources}; -/// Given an `Option`, unwrap the inner `T` or, if the option is `None`, set -/// the state to unreachable and return. +/// Given a `Reachability`, unwrap the inner `T` or, when unreachable, set +/// `state.reachable = false` and return. /// /// Used in combination with calling `prepare_addr` and `prepare_atomic_addr` /// when we can statically determine that a Wasm access will unconditionally @@ -106,8 +106,8 @@ use wasmparser::{FuncValidator, MemArg, Operator, WasmModuleResources}; macro_rules! unwrap_or_return_unreachable_state { ($state:ident, $value:expr) => { match $value { - Some(x) => x, - None => { + Reachability::Reachable(x) => x, + Reachability::Unreachable => { $state.reachable = false; return Ok(()); } @@ -670,49 +670,94 @@ pub fn translate_operator( * The memory base address is provided by the environment. ************************************************************************************/ Operator::I32Load8U { memarg } => { - translate_load(memarg, ir::Opcode::Uload8, I32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Uload8, I32, builder, state, environ)? + ); } Operator::I32Load16U { memarg } => { - translate_load(memarg, ir::Opcode::Uload16, I32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Uload16, I32, builder, state, environ)? + ); } Operator::I32Load8S { memarg } => { - translate_load(memarg, ir::Opcode::Sload8, I32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Sload8, I32, builder, state, environ)? + ); } Operator::I32Load16S { memarg } => { - translate_load(memarg, ir::Opcode::Sload16, I32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Sload16, I32, builder, state, environ)? + ); } Operator::I64Load8U { memarg } => { - translate_load(memarg, ir::Opcode::Uload8, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Uload8, I64, builder, state, environ)? + ); } Operator::I64Load16U { memarg } => { - translate_load(memarg, ir::Opcode::Uload16, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Uload16, I64, builder, state, environ)? + ); } Operator::I64Load8S { memarg } => { - translate_load(memarg, ir::Opcode::Sload8, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Sload8, I64, builder, state, environ)? + ); } Operator::I64Load16S { memarg } => { - translate_load(memarg, ir::Opcode::Sload16, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Sload16, I64, builder, state, environ)? + ); } Operator::I64Load32S { memarg } => { - translate_load(memarg, ir::Opcode::Sload32, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Sload32, I64, builder, state, environ)? + ); } Operator::I64Load32U { memarg } => { - translate_load(memarg, ir::Opcode::Uload32, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Uload32, I64, builder, state, environ)? + ); } Operator::I32Load { memarg } => { - translate_load(memarg, ir::Opcode::Load, I32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Load, I32, builder, state, environ)? + ); } Operator::F32Load { memarg } => { - translate_load(memarg, ir::Opcode::Load, F32, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Load, F32, builder, state, environ)? + ); } Operator::I64Load { memarg } => { - translate_load(memarg, ir::Opcode::Load, I64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Load, I64, builder, state, environ)? + ); } Operator::F64Load { memarg } => { - translate_load(memarg, ir::Opcode::Load, F64, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Load, F64, builder, state, environ)? + ); } Operator::V128Load { memarg } => { - translate_load(memarg, ir::Opcode::Load, I8X16, builder, state, environ)?; + unwrap_or_return_unreachable_state!( + state, + translate_load(memarg, ir::Opcode::Load, I8X16, builder, state, environ)? + ); } Operator::V128Load8x8S { memarg } => { let (flags, base) = unwrap_or_return_unreachable_state!( @@ -1502,26 +1547,32 @@ pub fn translate_operator( | Operator::V128Load16Splat { memarg } | Operator::V128Load32Splat { memarg } | Operator::V128Load64Splat { memarg } => { - translate_load( - memarg, - ir::Opcode::Load, - type_of(op).lane_type(), - builder, + unwrap_or_return_unreachable_state!( state, - environ, - )?; + translate_load( + memarg, + ir::Opcode::Load, + type_of(op).lane_type(), + builder, + state, + environ, + )? + ); let splatted = builder.ins().splat(type_of(op), state.pop1()); state.push1(splatted) } Operator::V128Load32Zero { memarg } | Operator::V128Load64Zero { memarg } => { - translate_load( - memarg, - ir::Opcode::Load, - type_of(op).lane_type(), - builder, + unwrap_or_return_unreachable_state!( state, - environ, - )?; + translate_load( + memarg, + ir::Opcode::Load, + type_of(op).lane_type(), + builder, + state, + environ, + )? + ); let as_vector = builder.ins().scalar_to_vector(type_of(op), state.pop1()); state.push1(as_vector) } @@ -1530,14 +1581,17 @@ pub fn translate_operator( | Operator::V128Load32Lane { memarg, lane } | Operator::V128Load64Lane { memarg, lane } => { let vector = pop1_with_bitcast(state, type_of(op), builder); - translate_load( - memarg, - ir::Opcode::Load, - type_of(op).lane_type(), - builder, + unwrap_or_return_unreachable_state!( state, - environ, - )?; + translate_load( + memarg, + ir::Opcode::Load, + type_of(op).lane_type(), + builder, + state, + environ, + )? + ); let replacement = state.pop1(); state.push1(builder.ins().insertlane(vector, replacement, *lane)) } @@ -2239,7 +2293,7 @@ fn prepare_addr( builder: &mut FunctionBuilder, state: &mut FuncTranslationState, environ: &mut FE, -) -> WasmResult> +) -> WasmResult> where FE: FuncEnvironment + ?Sized, { @@ -2372,8 +2426,8 @@ where } }; let addr = match addr { - None => return Ok(None), - Some(a) => a, + Reachability::Unreachable => return Ok(Reachability::Unreachable), + Reachability::Reachable(a) => a, }; // Note that we don't set `is_aligned` here, even if the load instruction's @@ -2389,7 +2443,7 @@ where // vmctx, stack) accesses. flags.set_heap(); - Ok(Some((flags, addr))) + Ok(Reachability::Reachable((flags, addr))) } fn align_atomic_addr( @@ -2436,12 +2490,30 @@ fn prepare_atomic_addr( builder: &mut FunctionBuilder, state: &mut FuncTranslationState, environ: &mut FE, -) -> WasmResult> { +) -> WasmResult> { align_atomic_addr(memarg, loaded_bytes, builder, state); prepare_addr(memarg, loaded_bytes, builder, state, environ) } +/// Like `Option` but specifically for passing information about transitions +/// from reachable to unreachable state and the like from callees to callers. +/// +/// Marked `must_use` to force callers to update +/// `FuncTranslationState::reachable` as necessary. +#[derive(PartialEq, Eq)] +#[must_use] +pub enum Reachability { + /// The Wasm execution state is reachable, here is a `T`. + Reachable(T), + /// The Wasm execution state has been determined to be statically + /// unreachable. It is the receiver of this value's responsibility to update + /// `FuncTranslationState::reachable` as necessary. + Unreachable, +} + /// Translate a load instruction. +/// +/// Returns the execution state's reachability after the load is translated. fn translate_load( memarg: &MemArg, opcode: ir::Opcode, @@ -2449,22 +2521,22 @@ fn translate_load( builder: &mut FunctionBuilder, state: &mut FuncTranslationState, environ: &mut FE, -) -> WasmResult<()> { - let (flags, base) = unwrap_or_return_unreachable_state!( +) -> WasmResult> { + let (flags, base) = match prepare_addr( + memarg, + mem_op_size(opcode, result_ty), + builder, state, - prepare_addr( - memarg, - mem_op_size(opcode, result_ty), - builder, - state, - environ, - )? - ); + environ, + )? { + Reachability::Unreachable => return Ok(Reachability::Unreachable), + Reachability::Reachable((f, b)) => (f, b), + }; let (load, dfg) = builder .ins() .Load(opcode, result_ty, flags, Offset32::new(0), base); state.push1(dfg.first_result(load)); - Ok(()) + Ok(Reachability::Reachable(())) } /// Translate a store instruction. diff --git a/cranelift/wasm/src/code_translator/bounds_checks.rs b/cranelift/wasm/src/code_translator/bounds_checks.rs index fa49b08b99..edb02b9492 100644 --- a/cranelift/wasm/src/code_translator/bounds_checks.rs +++ b/cranelift/wasm/src/code_translator/bounds_checks.rs @@ -8,6 +8,7 @@ //! //! bounds check the memory access and translate it into a native memory access. +use super::Reachability; use crate::{FuncEnvironment, HeapData, HeapStyle}; use cranelift_codegen::{ cursor::{Cursor, FuncCursor}, @@ -15,6 +16,7 @@ use cranelift_codegen::{ }; use cranelift_frontend::FunctionBuilder; use wasmtime_types::WasmResult; +use Reachability::*; /// Helper used to emit bounds checks (as necessary) and compute the native /// address of a heap access. @@ -31,7 +33,7 @@ pub fn bounds_check_and_compute_addr( offset: u32, // Static size of the heap access. access_size: u8, -) -> WasmResult> +) -> WasmResult> where Env: FuncEnvironment + ?Sized, { @@ -76,7 +78,7 @@ where // checks. HeapStyle::Dynamic { bound_gv } if offset_and_size == 1 && spectre_mitigations_enabled => { let bound = builder.ins().global_value(env.pointer_type(), bound_gv); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -96,7 +98,7 @@ where .ins() .icmp(IntCC::UnsignedGreaterThanOrEqual, index, bound); builder.ins().trapnz(oob, ir::TrapCode::HeapOutOfBounds); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -120,7 +122,7 @@ where { let bound = builder.ins().global_value(env.pointer_type(), bound_gv); let adjusted_bound = builder.ins().iadd_imm(bound, -(offset_and_size as i64)); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -142,7 +144,7 @@ where .ins() .icmp(IntCC::UnsignedGreaterThan, index, adjusted_bound); builder.ins().trapnz(oob, ir::TrapCode::HeapOutOfBounds); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -169,7 +171,7 @@ where ir::TrapCode::HeapOutOfBounds, ); let bound = builder.ins().global_value(env.pointer_type(), bound_gv); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -198,7 +200,7 @@ where .ins() .icmp(IntCC::UnsignedGreaterThan, adjusted_index, bound); builder.ins().trapnz(oob, ir::TrapCode::HeapOutOfBounds); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -219,7 +221,7 @@ where HeapStyle::Static { bound } if offset_and_size > bound.into() => { env.before_unconditionally_trapping_memory_access(builder)?; builder.ins().trap(ir::TrapCode::HeapOutOfBounds); - None + Unreachable } // 2. Second special case for when we can completely omit explicit @@ -265,7 +267,7 @@ where && u64::from(u32::MAX) <= u64::from(bound) + u64::from(heap.offset_guard_size) - offset_and_size => { - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -295,7 +297,7 @@ where let adjusted_bound = builder .ins() .iconst(env.pointer_type(), adjusted_bound as i64); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(), @@ -318,7 +320,7 @@ where .ins() .icmp_imm(IntCC::UnsignedGreaterThan, index, adjusted_bound as i64); builder.ins().trapnz(oob, ir::TrapCode::HeapOutOfBounds); - Some(compute_addr( + Reachable(compute_addr( &mut builder.cursor(), heap, env.pointer_type(),