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

Contracts & Harnesses for byte_add, byte_offset, and byte_offset_from #103

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

danielhumanmod
Copy link

Description:

This PR introduces function contracts and proof harness for the NonNull pointer in the Rust core library. Specifically, it verifies three new APIs—byte_offset, byte_add, and byte_offset_from with Kani. These changes enhance the functionality of pointer arithmetic and verification for NonNull pointers.

Changes Overview:

Covered APIs:

  1. NonNull::byte_add: Adds a specified byte offset to a pointer.
  2. NonNull::byte_offset: Allows calculating an offset from a pointer in bytes.
  3. NonNull::byte_offset_from: Calculates the distance between two pointers in bytes.

Proof harness:

  1. non_null_byte_add_proof
  2. non_null_byte_offset_proof
  3. non_null_byte_offset_from_proof

Resolves #53

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

@danielhumanmod danielhumanmod changed the title Contracts & Harnesses for byte_add, byte_offset, and byte_offset_from Contracts & Harnesses for byte_add, byte_offset, and byte_offset_from Oct 6, 2024
@danielhumanmod danielhumanmod marked this pull request as ready for review October 9, 2024 23:48
@danielhumanmod danielhumanmod requested a review from a team as a code owner October 9, 2024 23:48
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
@danielhumanmod
Copy link
Author

Thanks for your suggestion @zhassan-aws! I've updated the code in the latest commit based on your comments. I’d appreciate it if you could take a look when you have a chance.

library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
@danielhumanmod
Copy link
Author

Hi @zhassan-aws , thanks for the review and suggestions. I have updated the code based on the comments, fix the unnecessary assume proof harness, using addr, and add unchecked_add to avoid overflow, appreciate if any further suggestion!

Copy link

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

Nice. Thanks!

library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
Copy link

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Please replace kani::requires and kani::ensures by safety::requires and safety::ensures. Thanks!

library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
library/core/src/ptr/non_null.rs Outdated Show resolved Hide resolved
Copy link

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Looks good. Can you please add harnesses for the dangling pointers cases?

@@ -529,6 +529,11 @@ impl<T: ?Sized> NonNull<T> {
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
#[stable(feature = "non_null_convenience", since = "1.80.0")]
#[rustc_const_stable(feature = "non_null_convenience", since = "1.80.0")]
#[requires(

Choose a reason for hiding this comment

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

I believe it's safe to call this function with count == 0 even if self.pointer is dangling.

Copy link
Author

@danielhumanmod danielhumanmod Nov 20, 2024

Choose a reason for hiding this comment

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

I believe it's safe to call this function with count == 0 even if self.pointer is dangling.

Appreciate the suggestion @celinval! But there is some failure with the my dangling proof:

Proof harness:

#[kani::proof_for_contract(NonNull::byte_add)]
    pub fn non_null_byte_add_dangling_proof() {
        let ptr = NonNull::<i32>::dangling();
        let count: usize = 0;
        unsafe {
            let _ = ptr.byte_add(count);
        }
    }

Error:

VERIFICATION RESULT:
 ** 1 of 178 failed
Failed Checks: Kani does not support reasoning about pointer to unallocated memory
 File: "/Users/danieltu/Developer/verify-rust-std/library/core/src/lib.rs", line 423, in kani::mem::cbmc::same_allocation::<i32>

VERIFICATION:- FAILED
Verification Time: 0.26384938s

I am not sure whether it is because I didn't create a proper dangling scenario, could you provide some suggestion on this? Thanks!

Choose a reason for hiding this comment

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

Did you extend the requires clause?

Copy link
Author

Choose a reason for hiding this comment

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

Did you extend the requires clause?

Hi @celinval, do you mind being more specific about it? Based on my understanding, we need to add a proof harness where ptr is dangling and count = 0. Any additional contract need to be added into the API?

library/core/src/ptr/non_null.rs Show resolved Hide resolved
@@ -812,6 +821,11 @@ impl<T: ?Sized> NonNull<T> {
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
#[stable(feature = "non_null_convenience", since = "1.80.0")]
#[rustc_const_stable(feature = "non_null_convenience", since = "1.80.0")]
#[requires(kani::mem::same_allocation(self.as_ptr() as *const(), origin.as_ptr() as *const()))]

Choose a reason for hiding this comment

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

This should also be safe if the pointers hold the same address, even if they are dangling

Copy link
Author

Choose a reason for hiding this comment

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

This should also be safe if the pointers hold the same address, even if they are dangling

Same as above, there is a failure:

Proof harness

#[kani::proof_for_contract(NonNull::byte_offset_from)]
    pub fn non_null_byte_offset_from_dangling_proof() {
        let origin = NonNull::<i32>::dangling();
        let ptr = origin;
        unsafe {
            let _ = ptr.byte_offset_from(origin);
        }
    }

Error:

VERIFICATION RESULT:
 ** 1 of 184 failed (1 unreachable)
Failed Checks: Kani does not support reasoning about pointer to unallocated memory
 File: "/Users/danieltu/Developer/verify-rust-std/library/core/src/lib.rs", line 423, in kani::mem::cbmc::same_allocation::<()>

VERIFICATION:- FAILED
Verification Time: 0.24433088s

Choose a reason for hiding this comment

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

Please see my question above.

@feliperodri
Copy link

@danielhumanmod could you resolve the conflicts?

@danielhumanmod
Copy link
Author

@danielhumanmod could you resolve the conflicts?

Hi @feliperodri , I have solved the conflicts in latest commit, thanks for reminding that

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.

Challenge 6: Safety of NonNull
5 participants