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

CStr Safety invariant & Harnesses for from_bytes_until_nul #180

Merged
merged 12 commits into from
Nov 26, 2024

Conversation

Yenyun035
Copy link

@Yenyun035 Yenyun035 commented Nov 22, 2024

Resolves / Towards #150

Changes

  • Added a CStr Safety Invariant
  • Added a harness for from_bytes_until_nul, the harness covers:
    • The input slice contains a single null byte at the end;
    • The input slice contains no null bytes;
    • The input slice contains intermediate null bytes

Discussion

Verification Result

./scripts/run-kani.sh --kani-args --harness ffi::c_str::verify

// array size 16
Checking harness ffi::c_str::verify::check_from_bytes_until_nul...

VERIFICATION RESULT:
 ** 0 of 140 failed (5 unreachable)

VERIFICATION:- SUCCESSFUL
Verification Time: 7.3023376s

Complete - 1 successfully verified harnesses, 0 failures, 1 total.

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

@Yenyun035 Yenyun035 requested a review from a team as a code owner November 22, 2024 23:27
@Yenyun035
Copy link
Author

Yenyun035 commented Nov 23, 2024

@celinval @zhassan-aws

Questions
While we explicitly write three different checks, we're wondering if a positive harness (e.g. Check 1) and a negative harness would be sufficient. By negative, we mean that we can have a check to call from_bytes_until_nul on an array without a null byte. The difference between the two harnesses is that we need the kani::should_panic for the negative case. Here is an example of the said two harnesses:

    // Positive check
    #[kani::proof]
    #[kani::unwind(32)]
    fn check_from_bytes_until_nul() {
        const ARR_LEN: usize = 32;
        let mut string: [u8; ARR_LEN] = kani::any();
        // ensure that there is at least one null byte
        let idx: usize = kani::any_where(|x: &usize| *x >= 0 && *x < ARR_LEN);
        string[idx] = 0;

        let c_str = CStr::from_bytes_until_nul(&string).unwrap();
        assert!(c_str.is_safe());
    }

    // Negative check
    #[kani::proof]
    #[kani::unwind(5)]
    #[kani::should_panic]
    fn check_from_bytes_until_nul_panic() {
        const ARR_LEN: usize = 4;
        // let mut string: [u8; ARR_LEN] = [64, 65, 66];
        // The array does not contain any null bytes. Calling
        // from_bytes_until_nul will return an error.
        let mut string: [u8; ARR_LEN] = kani::any_where(|x: &[u8; ARR_LEN]| !x.contains(&0));

        let c_str = CStr::from_bytes_until_nul(&string).unwrap();
        assert!(c_str.is_safe());
    }

Verification result:

// panic case
SUMMARY:
 ** 1 of 134 failed (18 unreachable)
Failed Checks: This is a placeholder message; Kani doesn't support message formatted at runtime
 File: "/Users/yew005/Docs/Academic/CMU/Fall24/practicum/vrs/library/core/src/result.rs", line 1698, in result::unwrap_failed

VERIFICATION:- SUCCESSFUL (encountered one or more panics as expected)
Verification Time: 0.6366273s

// regular case
SUMMARY:
 ** 0 of 142 failed (5 unreachable)

VERIFICATION:- SUCCESSFUL
Verification Time: 5.0670085s

Complete - 2 successfully verified harnesses, 0 failures, 2 total.

@celinval
Copy link

Hi @Yenyun035, that's a great question. I think in the case of from_bytes_until_nul, you could potentially have one harness only. If you don't invoke unwrap in the result, the harness shouldn't panic, independent on whether it has a nul character or not.

Thus, it should be enough to create an arbitrary slice of [u8], and provide it to the function. You could then match on the result, and if it is Ok(cstr), you check that the cstr is safe.

@celinval celinval self-assigned this Nov 25, 2024
@Yenyun035
Copy link
Author

Yenyun035 commented Nov 25, 2024

@celinval Thank you very much for your advice! I modified the harness and the PR description accordingly, and now it should be good to go.

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.

Thanks!

library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
@Yenyun035
Copy link
Author

@celinval Thank you for your comments. I resolved the conversations. Please let me know if anything else to be modified :)

library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
@Yenyun035
Copy link
Author

@zhassan-aws Thank you for your comments. I just fixed them.

@zhassan-aws zhassan-aws enabled auto-merge (squash) November 26, 2024 00:49
@zhassan-aws zhassan-aws merged commit 82da845 into model-checking:main Nov 26, 2024
11 checks passed
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.

3 participants