Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Only decrement the counter of an array if its address has not changed #7297

Merged
merged 21 commits into from
Feb 13, 2025
Merged
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add regression test that shows there is a bug
aakoshh committed Feb 7, 2025

Verified

This commit was signed with the committer’s verified signature.
aakoshh Akosh Farkash
commit 548987ed08f6eed16e9e772d91d548c0a62592b3
Original file line number Diff line number Diff line change
@@ -873,7 +873,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
ctx.deallocate_register(rc_register);
};

if array_register == orig_array_register {
if true || array_register == orig_array_register {
codegen_dec_rc(self.brillig_context);
} else {
// Check if the current and original register point at the same memory.
30 changes: 29 additions & 1 deletion test_programs/execution_success/reference_counts/src/main.nr
Original file line number Diff line number Diff line change
@@ -14,6 +14,8 @@ fn main() {
let rc1 = array_refcount(array);
let rc2 = array_refcount(u32_array);
borrow_mut_two_separate(&mut array, &mut u32_array, rc1, rc2);

regression_7297();
}

fn borrow(array: [Field; 3], rc_before_call: u32) {
@@ -29,10 +31,12 @@ fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) {
println(array[0]);
}

fn copy_mut(mut array: [Field; 3], rc_before_call: u32) {
// Returning a copy of the array, otherwise the SSA optimizes it away and just prints 4 verbatim.
fn copy_mut(mut array: [Field; 3], rc_before_call: u32) -> [Field; 3] {
assert_refcount(array, rc_before_call + 1);
array[0] = 4;
println(array[0]);
array
}

/// Borrow the same array mutably through both parameters, inc_rc is necessary here, although
@@ -76,3 +80,27 @@ fn assert_refcount<T>(array: [T; 3], expected: u32) {
assert_eq(count, 0);
}
}


fn regression_7297() {
println("regression_7297");
let mut array = [0, 1, 2];

let refcount_0 = array_refcount(array);
borrow_mut_two(&mut array, &mut array, refcount_0);
let refcount_1 = array_refcount(array);
let array_2 = copy_mut(array, refcount_1);
let refcount_2 = array_refcount(array);

println([refcount_0, refcount_1, refcount_2]);

// Mutation of the original could occur if we double decremented the RC and then went back to 1 by accident.
// For this to come out we have to run the test with `--inliner-aggressiveness -9223372036854775808`
assert_eq(array[0], 6, "the original should not be mutated by copy_mut, only borrow_mut_two");
// Double decrementing the RC could occur if we don't realize that array mutation made a copy,
// which decreases the RC of the original and sets the new one to 1.
assert(refcount_1 != 0, "borrow_mut_two should create a fresh array and not decrease its RC");
assert_eq(refcount_1, 1, "borrow_mut_two should create a fresh array with an RC of 1");
// XXX: This assertion is here to demonstrate that this is happening, not because it must be like this
assert_eq(refcount_2, refcount_1 + 1, "not ideal, but original RC permanently increased by copy_mut");
}