diff --git a/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs b/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs index c15ee1b5b2c..cb644e547cf 100644 --- a/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs +++ b/compiler/noirc_evaluator/src/ssa/validation/dynamic_array_indices.rs @@ -1,7 +1,7 @@ use crate::{ errors::{RtResult, RuntimeError}, ssa::{ - ir::{function::Function, instruction::Instruction}, + ir::{dfg::DataFlowGraph, function::Function, instruction::Instruction, value::ValueId}, ssa_gen::Ssa, }, }; @@ -37,7 +37,7 @@ impl Function { let array_type = self.dfg.type_of_value(*array); let contains_reference = array_type.contains_reference(); - if contains_reference && self.dfg.get_numeric_constant(*index).is_none() { + if contains_reference && !is_non_dynamic(&self.dfg, *index) { let call_stack = self.dfg.get_instruction_call_stack(*instruction); return Err(RuntimeError::DynamicIndexingWithReference { call_stack }); } @@ -50,6 +50,16 @@ impl Function { } } +/// Check if an value is a numeric constant, or a result of an instruction that only uses numeric constant inputs. +fn is_non_dynamic(dfg: &DataFlowGraph, value: ValueId) -> bool { + // We could check if a non-constant-numeric value is a result of for example a binary an instruction that only + // takes numeric constant input. However, if we have such a value, it might be a result of an overflowing + // index expression that we could not simplify at runtime, which means most likely mem2reg could not eliminate + // the reference allocation either, so even if we classified such indexes as non-dynamic, since they only use + // known constants, we would just get another obscure error down the line with a less obvious call stack. + dfg.get_numeric_constant(value).is_some() +} + #[cfg(test)] mod tests { use crate::{errors::RuntimeError, ssa::ssa_gen::Ssa}; @@ -104,4 +114,27 @@ mod tests { let result = ssa.verify_no_dynamic_indices_to_references(); assert!(result.is_ok()); } + + #[test] + fn error_on_index_overflow() { + // https://github.com/noir-lang/noir/issues/9853 + // fn main() -> pub bool { + // let mut e: [(&mut Field, bool); 1] = [((&mut -1), false)]; + // e[2147483648].1 + // } + let src = r#" + acir(inline) predicate_pure fn main f0 { + b0(): + v0 = allocate -> &mut Field + v2 = make_array [v0, u1 0] : [(&mut Field, u1); 1] + v5 = unchecked_mul u32 2147483648, u32 2 + v7 = unchecked_add v5, u32 1 + v8 = array_get v2, index v7 -> u1 + return v8 + }"#; + + let ssa = Ssa::from_str(src).unwrap(); + let result = ssa.verify_no_dynamic_indices_to_references(); + assert!(matches!(result, Err(RuntimeError::DynamicIndexingWithReference { .. }))); + } } diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 7b6015878aa..f4b647331ed 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -910,24 +910,6 @@ impl<'a> FunctionContext<'a> { let Type::Slice(item_type) = src_type else { unreachable!("only expected to be called with Slice"); }; - // The rules around dynamic indexing is the same as for arrays. - let (mut idx_expr, idx_dyn) = if max_depth == 0 || bool::arbitrary(u)? { - // Avoid any stack overflow where we look for an index in the slice itself. - (self.gen_literal(u, &types::U32)?, false) - } else { - let no_dynamic = - self.in_no_dynamic || !self.unconstrained() && types::contains_reference(item_type); - let was_in_no_dynamic = std::mem::replace(&mut self.in_no_dynamic, no_dynamic); - - // Choose a random index. - let (idx_expr, idx_dyn) = - self.gen_expr(u, &types::U32, max_depth.saturating_sub(1), Flags::NESTED)?; - - assert!(!(no_dynamic && idx_dyn), "non-dynamic index expected"); - - self.in_no_dynamic = was_in_no_dynamic; - (idx_expr, idx_dyn) - }; // Unless the slice is coming from an identifier or literal, we should create a let binding for it // to avoid doubling up any side effects, or double using local variables when it was created. @@ -951,10 +933,29 @@ impl<'a> FunctionContext<'a> { // Get the runtime length. let len_expr = self.call_array_len(Expression::Ident(ident_1), src_type.clone()); - // Take the modulo, but leave a small chance for index OOB. - if self.avoid_index_out_of_bounds(u)? { - idx_expr = expr::modulo(idx_expr, len_expr); - } + // The rules around dynamic indexing is the same as for arrays. + let (idx_expr, idx_dyn) = if max_depth == 0 || bool::arbitrary(u)? { + // Avoid any stack overflow where we look for an index in the slice itself. + (self.gen_literal(u, &types::U32)?, false) + } else { + let no_dynamic = + self.in_no_dynamic || !self.unconstrained() && types::contains_reference(item_type); + let was_in_no_dynamic = std::mem::replace(&mut self.in_no_dynamic, no_dynamic); + + // Choose a random index. + let (mut idx_expr, idx_dyn) = + self.gen_expr(u, &types::U32, max_depth.saturating_sub(1), Flags::NESTED)?; + + assert!(!(no_dynamic && idx_dyn), "non-dynamic index expected"); + + // Take the modulo, but leave a small chance for index OOB. + if self.avoid_index_out_of_bounds(u)? { + idx_expr = expr::modulo(idx_expr, len_expr); + } + + self.in_no_dynamic = was_in_no_dynamic; + (idx_expr, idx_dyn) + }; // Access the item by index let item_expr = access_item(self, ident_2, idx_expr); @@ -2291,8 +2292,12 @@ impl<'a> FunctionContext<'a> { /// on a specific array or slice access operation. /// /// If [Config::avoid_index_out_of_bounds] is turned on, then this is always `true`. + /// + /// It also returns `true` when `in_no_dynamic` mode is on, because an overflowing + /// index might not be simplified out of the SSA in ACIR, and end up being considered + /// a dynamic index, and leave reference allocations until ACIR gen, where they fail. fn avoid_index_out_of_bounds(&self, u: &mut Unstructured) -> arbitrary::Result { - if self.config().avoid_index_out_of_bounds { + if self.config().avoid_index_out_of_bounds || self.in_no_dynamic { return Ok(true); } // Avoid OOB with 90% chance.