diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index 13a2e023d1c..640055a2abf 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -69,7 +69,7 @@ pub enum InterpreterError { #[error("Reached the unreachable")] ReachedTheUnreachable, #[error("Array index {index} is out of bounds for array of length {length}")] - IndexOutOfBounds { index: u32, length: u32 }, + IndexOutOfBounds { index: FieldElement, length: u32 }, } /// These errors can only result from interpreting malformed SSA diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index f8577b3dd89..62679937e6b 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -384,6 +384,27 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { self.lookup_helper(value_id, instruction, "array or slice", Value::as_array_or_slice) } + /// Look up an array index. + /// + /// If the value exists but it's `Unfit`, returns `IndexOutOfBounds`. + fn lookup_array_index( + &self, + value_id: ValueId, + instruction: &'static str, + length: u32, + ) -> IResult { + self.lookup_helper(value_id, instruction, "u32", Value::as_u32).map_err(|e| { + if matches!(e, InterpreterError::Internal(InternalError::TypeError { .. })) { + if let Ok(Value::Numeric(NumericValue::U32(Fitted::Unfit(index)))) = + self.lookup(value_id) + { + return InterpreterError::IndexOutOfBounds { index, length }; + } + } + e + }) + } + fn lookup_bytes(&self, value_id: ValueId, instruction: &'static str) -> IResult> { let array = self.lookup_array_or_slice(value_id, instruction)?; let array = array.elements.borrow(); @@ -945,16 +966,17 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { ) -> IResult<()> { let offset = self.dfg().array_offset(array, index); let array = self.lookup_array_or_slice(array, "array get")?; - let index = self.lookup_u32(index, "array get index")?; + let length = array.elements.borrow().len() as u32; + let index = self.lookup_array_index(index, "array get index", length)?; let mut index = index - offset.to_u32(); - let element = if array.elements.borrow().is_empty() { + let element = if length == 0 { // Accessing an array of 0-len is replaced by asserting // the branch is not-taken during acir-gen and // a zeroed type is used in case of array get // So we can simply replace it with uninitialized value if side_effects_enabled { - return Err(InterpreterError::IndexOutOfBounds { index, length: 0 }); + return Err(InterpreterError::IndexOutOfBounds { index: index.into(), length }); } else { let typ = self.dfg().type_of_value(result); Value::uninitialized(&typ, result) @@ -973,9 +995,9 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { } } let elements = array.elements.borrow(); - let element = elements.get(index as usize).ok_or_else(|| { - InterpreterError::IndexOutOfBounds { index, length: elements.len() as u32 } - })?; + let element = elements + .get(index as usize) + .ok_or(InterpreterError::IndexOutOfBounds { index: index.into(), length })?; // Either return a fresh nested array (in constrained context) or just clone the element. if !self.in_unconstrained_context() { @@ -1014,16 +1036,16 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { let array = self.lookup_array_or_slice(array, "array set")?; let result_array = if side_effects_enabled { - let index = self.lookup_u32(index, "array set index")?; + let length = array.elements.borrow().len() as u32; + let index = self.lookup_array_index(index, "array set index", length)?; let index = index - offset.to_u32(); let value = self.lookup(value)?; let should_mutate = if self.in_unconstrained_context() { *array.rc.borrow() == 1 } else { mutable }; - let len = array.elements.borrow().len(); - if index as usize >= len { - return Err(InterpreterError::IndexOutOfBounds { index, length: len as u32 }); + if index >= length { + return Err(InterpreterError::IndexOutOfBounds { index: index.into(), length }); } if should_mutate { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 006de36d3a0..a315faf8825 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -495,7 +495,10 @@ impl FunctionContext<'_> { // and we skipped inserting constraints. To stay on the conservative side, start with // a checked operation, and simplify to unchecked if it can be evaluated. // In Brillig we will be protected from overflows by constraints, so we can go unchecked. - let unchecked = runtime.is_brillig(); + // In ACIR the values are represented as Fields, so they don't wrap around, but ACIR has built-in OOB checks, + // so it's okay to use unchecked operations. The SSA interpreter has been updated to have similar semantics. + let unchecked = true; + let base_index = self.builder.set_location(location).insert_binary( index, BinaryOp::Mul { unchecked }, diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap index a2926e6fa0d..9959207eb63 100644 --- a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap +++ b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_9888/execute__tests__stderr.snap @@ -51,7 +51,16 @@ warning: Unsafe block must have a safety comment above it │ ------ The comment must start with the "Safety: " word │ -bug: Assertion is always false: attempt to multiply with overflow +warning: Return variable contains a constant value + ┌─ src/main.nr:54:5 + │ +54 │ G_B.4 + │ ----- This variable contains a value which is constrained to be a constant. Consider removing this value as additional return values increase proving/verification time + │ + = Call stack: + 1. src/main.nr:54:5 + +bug: Assertion is always false: Index out of bounds ┌─ src/main.nr:22:60 │ 22 │ if (item_c.2 > unsafe { func_4([]) }[1532016929_u32].3) {