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

Verifying methods of <*const T> #92

Closed
wants to merge 27 commits into from

Conversation

stogaru
Copy link

@stogaru stogaru commented Sep 20, 2024

Changes made:

  • Adds the function contracts and proof for the const offset function.

Problems :

  • What should be the valid input range for the count argument as asked here? How will we know the start address and the length of the allocated object from a raw pointer?

Works toward resolving #76

By submitting this pull request, we confirm that our contribution is made under the terms of the Apache 2.0 and MIT licenses.

@stogaru stogaru requested a review from a team as a code owner September 20, 2024 23:45
@@ -388,6 +400,10 @@ impl<T: ?Sized> *const T {
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
#[requires(kani::mem::can_dereference(self))]
// TODO: Determine the valid value range for 'count' and update the precondition accordingly.
#[requires(count == 0)] // This precondition is currently a placeholder.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This precondition needs to be changed based on the valid input range agreed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate more? Is there a GitHub issue to link here? What does the documentation say? How can we "determine the valid value range for count"?

Copy link
Author

@stogaru stogaru Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem 3 in the issue here gives some details. We need to add a precondition that defines the valid input range for the count parameter. This is because the offset guarantees the safety of the resulting pointer only when the self and the result are in bounds of that allocated object.
Therefore, count should be bounded such that self.addr() +/- count is within the start and end addresses of the allocated object. However, we cannot infer the start and end addresses of the allocated object solely based on the input pointer. How can we overcome this?

@feliperodri feliperodri changed the title AWS Team 3: Verifying <*const T>::offset Verifying <*const T>::offset Sep 27, 2024
@feliperodri feliperodri changed the title Verifying <*const T>::offset Verifying <*const T>::offset Sep 27, 2024
Comment on lines 280 to 284
if self.is_null() {
None
} else {
unsafe { Some(&*self) }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the format change necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a side-effect of running cargo fmt. I'll remove it.

Comment on lines 1800 to 1811
mod verify {
use crate::kani;

#[allow(unused)]
#[kani::proof_for_contract(<*const i32>::offset)]
fn check_offset_i32() {
let mut test_val: i32 = kani::any();
let test_ptr: *const i32 = &test_val;
let offset: isize = kani::any();
unsafe { test_ptr.offset(offset) };
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can expand this to other types. Please, see my comment here for clarity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! We will implement that for other types.

Comment on lines 1796 to 1797
// This module contains all proof harnesses for function contracts.
// Each proof harness verifies the soundness of a function contract for a specific type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unnecessary, you can remove it.

@@ -388,6 +400,10 @@ impl<T: ?Sized> *const T {
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
#[requires(kani::mem::can_dereference(self))]
// TODO: Determine the valid value range for 'count' and update the precondition accordingly.
#[requires(count == 0)] // This precondition is currently a placeholder.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate more? Is there a GitHub issue to link here? What does the documentation say? How can we "determine the valid value range for count"?

@stogaru stogaru changed the title Verifying <*const T>::offset Verifying methods of <*const T> Oct 4, 2024
Comment on lines +1833 to +1933
macro_rules! generate_add_harness {
($type:ty, $proof_name:ident) => {
#[allow(unused)]
#[kani::proof_for_contract(<*const $type>::add)]
pub fn $proof_name() {
let mut test_val: $type = kani::any::<$type>();
let test_ptr: *const $type = &test_val;
let count: usize = kani::any();
unsafe {
test_ptr.add(count);
}
}
};
}

generate_add_harness!(i8, check_add_i8);
generate_add_harness!(i16, check_add_i16);
generate_add_harness!(i32, check_add_i32);
generate_add_harness!(i64, check_add_i64);
generate_add_harness!(i128, check_add_i128);
generate_add_harness!(isize, check_add_isize);
generate_add_harness!(u8, check_add_u8);
generate_add_harness!(u16, check_add_u16);
generate_add_harness!(u32, check_add_u32);
generate_add_harness!(u64, check_add_u64);
generate_add_harness!(u128, check_add_u128);
generate_add_harness!(usize, check_add_usize);
generate_add_harness!((i8, i8), check_add_tuple_1);
generate_add_harness!((f64, bool), check_add_tuple_2);
generate_add_harness!((i32, f64, bool), check_add_tuple_3);
generate_add_harness!((i8, u16, i32, u64, isize), check_add_tuple_4);
// fn <*const T>::add verification end

// fn <*const T>::sub verification begin
macro_rules! generate_sub_harness {
($type:ty, $proof_name:ident) => {
#[allow(unused)]
#[kani::proof_for_contract(<*const $type>::sub)]
pub fn $proof_name() {
let mut test_val: $type = kani::any::<$type>();
let test_ptr: *const $type = &test_val;
let count: usize = kani::any();
unsafe {
test_ptr.sub(count);
}
}
};
}

generate_sub_harness!(i8, check_sub_i8);
generate_sub_harness!(i16, check_sub_i16);
generate_sub_harness!(i32, check_sub_i32);
generate_sub_harness!(i64, check_sub_i64);
generate_sub_harness!(i128, check_sub_i128);
generate_sub_harness!(isize, check_sub_isize);
generate_sub_harness!(u8, check_sub_u8);
generate_sub_harness!(u16, check_sub_u16);
generate_sub_harness!(u32, check_sub_u32);
generate_sub_harness!(u64, check_sub_u64);
generate_sub_harness!(u128, check_sub_u128);
generate_sub_harness!(usize, check_sub_usize);
generate_sub_harness!((i8, i8), check_sub_tuple_1);
generate_sub_harness!((f64, bool), check_sub_tuple_2);
generate_sub_harness!((i32, f64, bool), check_sub_tuple_3);
generate_sub_harness!((i8, u16, i32, u64, isize), check_sub_tuple_4);
// fn <*const T>::sub verification end

// fn <*const T>::offset verification begin
macro_rules! generate_offset_harness {
($type:ty, $proof_name:ident) => {
#[allow(unused)]
#[kani::proof_for_contract(<*const $type>::offset)]
pub fn $proof_name() {
let mut test_val: $type = kani::any::<$type>();
let test_ptr: *const $type = &test_val;
let count: isize = kani::any();
unsafe {
test_ptr.offset(count);
}
}
};
}

generate_offset_harness!(i8, check_offset_i8);
generate_offset_harness!(i16, check_offset_i16);
generate_offset_harness!(i32, check_offset_i32);
generate_offset_harness!(i64, check_offset_i64);
generate_offset_harness!(i128, check_offset_i128);
generate_offset_harness!(isize, check_offset_isize);
generate_offset_harness!(u8, check_offset_u8);
generate_offset_harness!(u16, check_offset_u16);
generate_offset_harness!(u32, check_offset_u32);
generate_offset_harness!(u64, check_offset_u64);
generate_offset_harness!(u128, check_offset_u128);
generate_offset_harness!(usize, check_offset_usize);
generate_offset_harness!((i8, i8), check_offset_tuple_1);
generate_offset_harness!((f64, bool), check_offset_tuple_2);
generate_offset_harness!((i32, f64, bool), check_offset_tuple_3);
generate_offset_harness!((i8, u16, i32, u64, isize), check_offset_tuple_4);
// fn <*const T>::offset verification end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should go at the top of each code block. Also, you can consolidate all three macros into a single declarative macro using a parameter to specify the type of pointer operation (i.e., add, sub, or offset).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would update in the next PR.

mod verify {
use crate::kani;

#[allow(unused)]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[allow(unused)] was added to suppress unused code warnings during development, but it's not necessary anymore. We'll remove it to ensure we catch any unused code warnings.

Comment on lines +1801 to +1831
#[kani::proof_for_contract(<*const i32>::offset)]
fn check_offset_slice_i32(){
let mut arr: [i32; 5] = kani::any();
let test_ptr: *const i32 = arr.as_ptr();
let offset: isize = kani::any();

unsafe{
let new_ptr = test_ptr.offset(offset);
}
}

#[kani::proof_for_contract(<*const i32>::add)]
fn check_add_slice_i32() {
let mut arr: [i32; 5] = kani::any();
let test_ptr: *const i32 = arr.as_ptr();
let count: usize = kani::any();
unsafe {
let new_ptr = test_ptr.add(count);
}
}


#[kani::proof_for_contract(<*const i32>::sub)]
fn check_sub_slice_i32() {
let mut arr: [i32; 5] = kani::any();
let test_ptr: *const i32 = arr.as_ptr();
let count: usize = kani::any();
unsafe {
let new_ptr = test_ptr.sub(count);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Would this be extended to different types or only i32? We can avoid code repeition using a macro here as well.
  2. Why are we picking 5 for the length of the array? We need to add documentation for any magical numbers.
  3. Have you tried to use here kani::any_array?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes it could be extended to different type, here it is following the minimum requirement in challenge 3.
  2. Since the length of array must be defined, I was following the test 1
  3. kani::any_array() would still need to define the size of array, by referring to test2. However, I would use it in the next PR.

@stogaru stogaru closed this Oct 7, 2024
@stogaru stogaru deleted the verify/ptr_const_offset branch October 7, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants