fix: always check index out of bounds for arrays and slices

compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs

@@ -715,7 +715,12 @@ impl<'a> FunctionContext<'a> {
                 }
             }
             ast::LValue::Index { array, index, location, .. } => {
-                self.index_lvalue(array, index, location)?.2
+                let (_array_or_slice, index, lvalue_ref, length) =
+                    self.index_lvalue(array, index, location)?;
+
+                self.codegen_slice_access_check(index, length);
+
+                lvalue_ref
             }
             ast::LValue::MemberAccess { object, field_index } => {
                 let (old_object, object_lvalue) = self.extract_current_value_recursive(object)?;
@@ -747,37 +752,29 @@ impl<'a> FunctionContext<'a> {
     }
 
     /// Compile the given `array[index]` expression as a reference.
-    /// This will return a triple of (array, index, lvalue_ref, Option<length>) where the lvalue_ref records the
+    /// This will return a tuple of four elements, (array, index, lvalue_ref, length), where the lvalue_ref records the
     /// structure of the lvalue expression for use by `assign_new_value`.
-    /// The optional length is for indexing slices rather than arrays since slices
-    /// are represented as a tuple in the form: (length, slice contents).
     fn index_lvalue(
         &mut self,
         array: &ast::LValue,
         index: &ast::Expression,
         location: &Location,
-    ) -> Result<(ValueId, ValueId, LValue, Option<ValueId>), RuntimeError> {
+    ) -> Result<(ValueId, ValueId, LValue, ValueId), RuntimeError> {
         let (old_array, array_lvalue) = self.extract_current_value_recursive(array)?;
         let index = self.codegen_non_tuple_expression(index)?;
         let array_lvalue = Box::new(array_lvalue);
         let array_values = old_array.clone().into_value_list(self);
 
         let location = *location;
-        // A slice is represented as a tuple (length, slice contents).
-        // We need to fetch the second value.
-        Ok(if array_values.len() > 1 {
-            let slice_lvalue = LValue::SliceIndex {
-                old_slice: old_array,
-                index,
-                slice_lvalue: array_lvalue,
-                location,
-            };
-            (array_values[1], index, slice_lvalue, Some(array_values[0]))
+        let (array_or_slice, length) = self.extract_indexable_type_length(&array_values);
+
+        let lvalue_ref = if array_values.len() > 1 {
+            LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location }
         } else {
-            let array_lvalue =
-                LValue::Index { old_array: array_values[0], index, array_lvalue, location };
-            (array_values[0], index, array_lvalue, None)
-        })
+            LValue::Index { old_array: array_or_slice, index, array_lvalue, location }
+        };
+
+        Ok((array_or_slice, index, lvalue_ref, length))
     }
 
     fn extract_current_value_recursive(

compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -368,20 +368,14 @@
     fn codegen_index(&mut self, index: &ast::Index) -> Result<Values, RuntimeError> {
         let array_or_slice = self.codegen_expression(&index.collection)?.into_value_list(self);
         let index_value = self.codegen_non_tuple_expression(&index.index)?;
-        // Slices are represented as a tuple in the form: (length, slice contents).
-        // Thus, slices require two value ids for their representation.
-        let (array, slice_length) = if array_or_slice.len() > 1 {
-            (array_or_slice[1], Some(array_or_slice[0]))
-        } else {
-            (array_or_slice[0], None)
-        };
+        let (array_or_slice, length) = self.extract_indexable_type_length(&array_or_slice);
 
         self.codegen_array_index(
-            array,
+            array_or_slice,
             index_value,
             &index.element_type,
             index.location,
-            slice_length,
+            length,
         )
     }
 
@@ -397,7 +391,7 @@
         index: super::ir::value::ValueId,
         element_type: &ast::Type,
         location: Location,
-        length: Option<super::ir::value::ValueId>,
+        length: super::ir::value::ValueId,
     ) -> Result<Values, RuntimeError> {
         // base_index = index * type_size
         let index = self.make_array_index(index);
@@ -412,16 +406,7 @@
             let offset = self.make_offset(base_index, field_index);
             field_index += 1;
 
-            let array_type = &self.builder.type_of_value(array);
-            match array_type {
-                Type::Slice(_) => {
-                    self.codegen_slice_access_check(index, length);
-                }
-                Type::Array(..) => {
-                    // Nothing needs to done to prepare an array access on an array
-                }
-                _ => unreachable!("must have array or slice but got {array_type}"),
-            }
+            self.codegen_slice_access_check(index, length);
 
             // Reference counting in brillig relies on us incrementing reference
             // counts when nested arrays/slices are constructed or indexed. This
@@ -438,12 +423,11 @@
     fn codegen_slice_access_check(
         &mut self,
         index: super::ir::value::ValueId,
-        length: Option<super::ir::value::ValueId>,
+        length: super::ir::value::ValueId,
     ) {
         let index = self.make_array_index(index);
         // We convert the length as an array index type for comparison
-        let array_len = self
-            .make_array_index(length.expect("ICE: a length must be supplied for indexing slices"));
+        let array_len = self.make_array_index(length);
 
         let is_offset_out_of_bounds = self.builder.insert_binary(index, BinaryOp::Lt, array_len);
         let true_const = self.builder.numeric_constant(true, Type::bool());
@@ -472,7 +456,7 @@
     ///   br loop_entry(v0)
     /// loop_entry(i: Field):
     ///   v2 = lt i v1
     ///   brif v2, then: loop_body, else: loop_end
     /// loop_body():
     ///   v3 = ... codegen body ...
     ///   v4 = add 1, i
@@ -531,7 +515,7 @@
     /// For example, the expression `if cond { a } else { b }` is codegen'd as:
     ///
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: else_block
     /// then_block():
     ///   v1 = ... codegen a ...
     ///   br end_if(v1)
@@ -544,7 +528,7 @@
     /// As another example, the expression `if cond { a }` is codegen'd as:
     ///
     ///   v0 = ... codegen cond ...
     ///   brif v0, then: then_block, else: end_block
     /// then_block:
     ///   v1 = ... codegen a ...
     ///   br end_if()
@@ -638,10 +622,10 @@
                     // can be converted to a slice push back
                     let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one);
 
-                    self.codegen_slice_access_check(arguments[2], Some(len_plus_one));
+                    self.codegen_slice_access_check(arguments[2], len_plus_one);
                 }
                 Intrinsic::SliceRemove => {
-                    self.codegen_slice_access_check(arguments[2], Some(arguments[0]));
+                    self.codegen_slice_access_check(arguments[2], arguments[0]);
                 }
                 _ => {
                     // Do nothing as the other intrinsics do not require checks
@@ -749,4 +733,23 @@
         self.builder.terminate_with_jmp(loop_.loop_entry, vec![new_loop_index]);
         Self::unit_value()
     }
+
+    // Given an indexable type (array or slice) that can either be a single value (an array) or two values (a slice),
+    // returns two values where the first one is the array or the slice, and the second one is the
+    // length.
+    fn extract_indexable_type_length(&mut self, array_or_slice: &[ValueId]) -> (ValueId, ValueId) {
+        // Slices are represented as a tuple in the form: (length, slice contents).
+        // Thus, slices require two value ids for their representation.
+        if array_or_slice.len() > 1 {
+            (array_or_slice[1], array_or_slice[0])
+        } else {
+            let array_type = &self.builder.type_of_value(array_or_slice[0]);
+            let Type::Array(_, length) = array_type else {
+                panic!("Expected array type when array is just a single value");
+            };
+
+            let length = self.builder.numeric_constant(*length, Type::unsigned(32));
+            (array_or_slice[0], length)
+        }
+    }
 }

test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "array_get_known_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main() {
+    let array = [1, 2, 3];
+    let _ = array[10]; // Index out of bounds
+}

test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "array_get_unknown_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml

@@ -0,0 +1 @@
+x = "10"

test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main(x: Field) {
+    let array = [1, 2, 3];
+    let _ = array[x]; // Index out of bounds
+}

test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "array_set_known_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main() {
+    let mut array = [1, 2, 3];
+    array[10] = 1; // Index out of bounds
+}

test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "array_set_unknown_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml

@@ -0,0 +1 @@
+x = "10"

test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main(x: Field) {
+    let mut array = [1, 2, 3];
+    array[x] = 1; // Index out of bounds
+}

test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "slice_get_known_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main() {
+    let slice = &[1, 2, 3];
+    let _ = slice[10]; // Index out of bounds
+}

test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "slice_get_unknown_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml

@@ -0,0 +1 @@
+x = "10"

test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main(x: Field) {
+    let slice = &[1, 2, 3];
+    let _ = slice[x]; // Index out of bounds
+}

test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "slice_set_known_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main() {
+    let mut slice = &[1, 2, 3];
+    slice[10] = 1; // Index out of bounds
+}

test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "slice_set_unknown_index_out_of_bounds"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.31.0"
+
+[dependencies]

test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml

@@ -0,0 +1 @@
+x = "10"

test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr

@@ -0,0 +1,4 @@
+fn main(x: Field) {
+    let mut slice = &[1, 2, 3];
+    slice[x] = 1; // Index out of bounds
+}