From 15dd7ba16d81e16e46885efe3ccc29010e4f7dac Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 12:47:17 -0300 Subject: [PATCH 1/9] Check index out of bounds when accessing array --- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +++-- .../array_get_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_get_known_index_out_of_bounds/Prover.toml | 0 .../array_get_known_index_out_of_bounds/src/main.nr | 4 ++++ .../array_get_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_get_unknown_index_out_of_bounds/Prover.toml | 1 + .../array_get_unknown_index_out_of_bounds/src/main.nr | 4 ++++ 7 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 468a8573307..2c049eea156 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -417,8 +417,9 @@ impl<'a> FunctionContext<'a> { Type::Slice(_) => { self.codegen_slice_access_check(index, length); } - Type::Array(..) => { - // Nothing needs to done to prepare an array access on an array + Type::Array(_, length) => { + let length = &self.builder.numeric_constant(*length, Type::field()); + self.codegen_slice_access_check(index, Some(*length)); } _ => unreachable!("must have array or slice but got {array_type}"), } diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_get_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..98b59477906 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_get_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_get_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..bdc645ec483 --- /dev/null +++ b/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 +} diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..986e9e8e2e6 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_get_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..15c2d1f1f23 --- /dev/null +++ b/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 +} From 6975d2501dff3e8eee8fbc581fa4c214146fda63 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:08:31 -0300 Subject: [PATCH 2/9] Use u32 for array index, just in case --- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 2c049eea156..99c1f6637f9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -418,7 +418,7 @@ impl<'a> FunctionContext<'a> { self.codegen_slice_access_check(index, length); } Type::Array(_, length) => { - let length = &self.builder.numeric_constant(*length, Type::field()); + let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); self.codegen_slice_access_check(index, Some(*length)); } _ => unreachable!("must have array or slice but got {array_type}"), From 05f784a73fbdc3a27f9d457d93735536b9b2dd18 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:13:51 -0300 Subject: [PATCH 3/9] Index out of bounds on array set --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 13 +++++++++++++ .../array_set_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../array_set_known_index_out_of_bounds/Prover.toml | 0 .../array_set_known_index_out_of_bounds/src/main.nr | 4 ++++ .../Nargo.toml | 7 +++++++ .../Prover.toml | 1 + .../src/main.nr | 4 ++++ 7 files changed, 36 insertions(+) create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 8e55debec1d..7c8f9376e75 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -859,6 +859,19 @@ impl<'a> FunctionContext<'a> { index: ValueId, location: Location, ) -> ValueId { + let array_type = &self.builder.type_of_value(array); + match array_type { + Type::Slice(_) => { + // TODO + // self.codegen_slice_access_check(index, length); + } + Type::Array(_, length) => { + let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); + self.codegen_slice_access_check(index, Some(*length)); + } + _ => unreachable!("must have array or slice but got {array_type}"), + } + let index = self.make_array_index(index); let element_size = self.builder.numeric_constant(self.element_size(array), Type::unsigned(SSA_WORD_SIZE)); diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_set_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..f6beafc4e90 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_set_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_set_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..9c447aee08f --- /dev/null +++ b/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 +} diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..8d3b47b2b86 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/array_set_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..dbde898f7a9 --- /dev/null +++ b/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 +} From 13badd5c19a9ecf5b5dc6091a631843bac373c4c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:37:37 -0300 Subject: [PATCH 4/9] Add tests for slice get index out of bounds --- .../slice_get_known_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_get_known_index_out_of_bounds/Prover.toml | 0 .../slice_get_known_index_out_of_bounds/src/main.nr | 4 ++++ .../slice_get_unknown_index_out_of_bounds/Nargo.toml | 7 +++++++ .../slice_get_unknown_index_out_of_bounds/Prover.toml | 1 + .../slice_get_unknown_index_out_of_bounds/src/main.nr | 4 ++++ 6 files changed, 23 insertions(+) create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..7520ed9a9c4 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_get_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..59e57664bbe --- /dev/null +++ b/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 +} diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..ddda52adeaf --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_get_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..5a62e0e9843 --- /dev/null +++ b/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 +} From 528d442f269eb64ce6e265bc38c48ba682eb9911 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:37:50 -0300 Subject: [PATCH 5/9] Make it work for slice set too --- .../src/ssa/ssa_gen/context.rs | 27 +++++++++---------- .../Nargo.toml | 7 +++++ .../Prover.toml | 0 .../src/main.nr | 4 +++ .../Nargo.toml | 7 +++++ .../Prover.toml | 1 + .../src/main.nr | 4 +++ 7 files changed, 36 insertions(+), 14 deletions(-) create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml create mode 100644 test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 7c8f9376e75..620b10c795c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -772,8 +772,20 @@ impl<'a> FunctionContext<'a> { slice_lvalue: array_lvalue, location, }; - (array_values[1], index, slice_lvalue, Some(array_values[0])) + + let length = Some(array_values[0]); + self.codegen_slice_access_check(index, length); + + (array_values[1], index, slice_lvalue, length) } else { + let array_type = &self.builder.type_of_value(array_values[0]); + let Type::Array(_, length) = array_type else { + panic!("Expected array type"); + }; + + let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); + self.codegen_slice_access_check(index, Some(*length)); + let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; (array_values[0], index, array_lvalue, None) @@ -859,19 +871,6 @@ impl<'a> FunctionContext<'a> { index: ValueId, location: Location, ) -> ValueId { - let array_type = &self.builder.type_of_value(array); - match array_type { - Type::Slice(_) => { - // TODO - // self.codegen_slice_access_check(index, length); - } - Type::Array(_, length) => { - let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, Some(*length)); - } - _ => unreachable!("must have array or slice but got {array_type}"), - } - let index = self.make_array_index(index); let element_size = self.builder.numeric_constant(self.element_size(array), Type::unsigned(SSA_WORD_SIZE)); diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..ea535921e99 --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_known_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..31d06cc200d --- /dev/null +++ b/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 +} diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Nargo.toml new file mode 100644 index 00000000000..597f7be81ba --- /dev/null +++ b/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] \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml new file mode 100644 index 00000000000..1ec81884d61 --- /dev/null +++ b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/Prover.toml @@ -0,0 +1 @@ +x = "10" \ No newline at end of file diff --git a/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr b/test_programs/execution_failure/slice_set_unknown_index_out_of_bounds/src/main.nr new file mode 100644 index 00000000000..33d30a4b91a --- /dev/null +++ b/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 +} From afab7c0f0ada4a271f156768a0f5a843da7ba085 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:39:10 -0300 Subject: [PATCH 6/9] Let `codegen_slice_access_check` take a non-Option value --- .../noirc_evaluator/src/ssa/ssa_gen/context.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 16 +++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 620b10c795c..3a34c1c11db 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -773,10 +773,10 @@ impl<'a> FunctionContext<'a> { location, }; - let length = Some(array_values[0]); + let length = array_values[0]; self.codegen_slice_access_check(index, length); - (array_values[1], index, slice_lvalue, length) + (array_values[1], index, slice_lvalue, Some(length)) } else { let array_type = &self.builder.type_of_value(array_values[0]); let Type::Array(_, length) = array_type else { @@ -784,7 +784,7 @@ impl<'a> FunctionContext<'a> { }; let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, Some(*length)); + self.codegen_slice_access_check(index, *length); let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 99c1f6637f9..279e0bf3dc2 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -415,11 +415,14 @@ impl<'a> FunctionContext<'a> { let array_type = &self.builder.type_of_value(array); match array_type { Type::Slice(_) => { - self.codegen_slice_access_check(index, length); + self.codegen_slice_access_check( + index, + length.expect("Expected a slice length"), + ); } Type::Array(_, length) => { let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, Some(*length)); + self.codegen_slice_access_check(index, *length); } _ => unreachable!("must have array or slice but got {array_type}"), } @@ -439,12 +442,11 @@ impl<'a> FunctionContext<'a> { fn codegen_slice_access_check( &mut self, index: super::ir::value::ValueId, - length: Option, + 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()); @@ -639,10 +641,10 @@ impl<'a> FunctionContext<'a> { // 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 From c9dff5d368939f956a82e5cee92be7863750b30f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:44:42 -0300 Subject: [PATCH 7/9] Always check array/slice index out of bounds --- .../src/ssa/ssa_gen/context.rs | 14 +++---- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 37 ++++++------------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 3a34c1c11db..daf107ce435 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -747,16 +747,14 @@ impl<'a> FunctionContext<'a> { } /// Compile the given `array[index]` expression as a reference. - /// This will return a triple of (array, index, lvalue_ref, Option) where the lvalue_ref records the + /// This will return a triple of (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), 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); @@ -776,19 +774,19 @@ impl<'a> FunctionContext<'a> { let length = array_values[0]; self.codegen_slice_access_check(index, length); - (array_values[1], index, slice_lvalue, Some(length)) + (array_values[1], index, slice_lvalue, length) } else { let array_type = &self.builder.type_of_value(array_values[0]); let Type::Array(_, length) = array_type else { panic!("Expected array type"); }; - let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, *length); + let length = self.builder.numeric_constant(*length, Type::unsigned(32)); + self.codegen_slice_access_check(index, length); let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue, location }; - (array_values[0], index, array_lvalue, None) + (array_values[0], index, array_lvalue, length) }) } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 279e0bf3dc2..c361cb82f8e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -370,19 +370,19 @@ impl<'a> FunctionContext<'a> { 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])) + let (array, length) = if array_or_slice.len() > 1 { + (array_or_slice[1], array_or_slice[0]) } else { - (array_or_slice[0], None) + let array_type = &self.builder.type_of_value(array_or_slice[0]); + let Type::Array(_, length) = array_type else { + panic!("Expected array type"); + }; + + let length = self.builder.numeric_constant(*length, Type::unsigned(32)); + (array_or_slice[0], length) }; - self.codegen_array_index( - array, - index_value, - &index.element_type, - index.location, - slice_length, - ) + self.codegen_array_index(array, index_value, &index.element_type, index.location, length) } /// This is broken off from codegen_index so that it can also be @@ -397,7 +397,7 @@ impl<'a> FunctionContext<'a> { index: super::ir::value::ValueId, element_type: &ast::Type, location: Location, - length: Option, + length: super::ir::value::ValueId, ) -> Result { // base_index = index * type_size let index = self.make_array_index(index); @@ -412,20 +412,7 @@ impl<'a> FunctionContext<'a> { 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.expect("Expected a slice length"), - ); - } - Type::Array(_, length) => { - let length = &self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, *length); - } - _ => 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 From 464a4e1183000adfa8f5bdf6ea19b69224ca71f1 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 13:54:37 -0300 Subject: [PATCH 8/9] Introduce `extract_indexable_type_length` --- .../src/ssa/ssa_gen/context.rs | 32 ++++----------- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 41 ++++++++++++------- 2 files changed, 35 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index daf107ce435..4ec4639295d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -747,7 +747,7 @@ impl<'a> FunctionContext<'a> { } /// Compile the given `array[index]` expression as a reference. - /// This will return a triple of (array, index, lvalue_ref, 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`. fn index_lvalue( &mut self, @@ -761,33 +761,17 @@ impl<'a> FunctionContext<'a> { 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, - }; + let (array_or_slice, length) = self.extract_indexable_type_length(&array_values); - let length = array_values[0]; - self.codegen_slice_access_check(index, length); + self.codegen_slice_access_check(index, length); - (array_values[1], index, slice_lvalue, length) + let lvalue_ref = if array_values.len() > 1 { + LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location } } else { - let array_type = &self.builder.type_of_value(array_values[0]); - let Type::Array(_, length) = array_type else { - panic!("Expected array type"); - }; - - let length = self.builder.numeric_constant(*length, Type::unsigned(32)); - self.codegen_slice_access_check(index, length); + LValue::Index { old_array: array_or_slice, index, array_lvalue, location } + }; - let array_lvalue = - LValue::Index { old_array: array_values[0], index, array_lvalue, location }; - (array_values[0], index, array_lvalue, length) - }) + Ok((array_or_slice, index, lvalue_ref, length)) } fn extract_current_value_recursive( diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c361cb82f8e..b9e0d594445 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -368,21 +368,15 @@ impl<'a> FunctionContext<'a> { fn codegen_index(&mut self, index: &ast::Index) -> Result { 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, length) = 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"); - }; - - let length = self.builder.numeric_constant(*length, Type::unsigned(32)); - (array_or_slice[0], length) - }; + let (array_or_slice, length) = self.extract_indexable_type_length(&array_or_slice); - self.codegen_array_index(array, index_value, &index.element_type, index.location, length) + self.codegen_array_index( + array_or_slice, + index_value, + &index.element_type, + index.location, + length, + ) } /// This is broken off from codegen_index so that it can also be @@ -739,4 +733,23 @@ impl<'a> FunctionContext<'a> { 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) + } + } } From 7eb665c2ae9f03cea34f960e35cea9f5ccd70022 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 14:04:22 -0300 Subject: [PATCH 9/9] Avoid issuing too many `codegen_slice_access_check` --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 4ec4639295d..95a7e694e5f 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/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)?; @@ -763,8 +768,6 @@ impl<'a> FunctionContext<'a> { let location = *location; let (array_or_slice, length) = self.extract_indexable_type_length(&array_values); - self.codegen_slice_access_check(index, length); - let lvalue_ref = if array_values.len() > 1 { LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue, location } } else {