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

test(s2n-quic-core): add a bolero harness for the varint table #1584

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Dec 5, 2022

Description of changes:

I had a branch from ~6 months ago that included a kani proof showing that the optimized version of the varint table matches the non-optimized version that more clearly matches what the RFC describes. It also included a proof that the math operations matched the underlying u64 operations, when possible.

Testing:

$ cargo kani --tests --harness round_trip_values_test
$ cargo kani --tests --harness table_differential_test
$ cargo kani --tests --harness checked_ops_test

CI Job

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

@camshaft camshaft marked this pull request as ready for review December 5, 2022 22:26

#[test]
#[cfg_attr(miri, ignore)] // This test is too expensive for miri to complete in a reasonable amount of time
fn round_trip() {
fn round_trip_bytes_test() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Kani unable to handle this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it but it kept wanting a higher unwind value. I put kani::unwind(50) and it still complained. I also put a kani::assume(bytes.len() <= 8) and it didn't make a difference, unfortunately. I didn't want to block the rest of these proofs on it, though, but definitely worth exploring.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I'll investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems there's a loop that requires an unwinding greater than 256. Setting the unwind to 256 results in an unwinding assertion failure:

Check 941: std::array::try_collect_into_array::<std::iter::Map<&mut std::iter::Map<std::array::IntoIter<(), 256>, [closure@<[u8; 256] as kani::Arbitrary>::any::{closure#0}]>, fn(u8) -> std::ops::try_trait::NeverShortCircuit<u8> {std::ops::try_trait::NeverShortCircuit::<u8>}>, u8, std::ops::try_trait::NeverShortCircuitResidual, 256>.unwind.0
         - Status: FAILURE
         - Description: "unwinding assertion loop 0"
         - Location: ../../../../.rustup/toolchains/nightly-2022-11-20-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/array/mod.rs:884:21 in function std::array::try_collect_into_array::<std::iter::Map<&mut std::iter::Map<std::array::IntoIter<(), 256>, [closure@<[u8; 256] as kani::Arbitrary>::any::{closure#0}]>, fn(u8) -> std::ops::try_trait::NeverShortCircuit<u8> {std::ops::try_trait::NeverShortCircuit::<u8>}>, u8, std::ops::try_trait::NeverShortCircuitResidual, 256>

and with 257, it keeps unwinding till it runs out of memory :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

#[test]
#[cfg_attr(kani, kani::proof, kani::unwind(5))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is an unwind annotation necessary?

Copy link
Contributor Author

@camshaft camshaft Dec 6, 2022

Choose a reason for hiding this comment

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

I can try it without. Although, generally I've just put unwind values on every harness. We had a harness that didn't have an unwind (worked fine) and upgraded kani and then it unwound forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that completes without. but like I mentioned, I think I prefer the explicit unwind value TBH.

@camshaft camshaft merged commit a15e731 into main Dec 19, 2022
@camshaft camshaft deleted the camshaft/varint-kani branch December 19, 2022 22:29
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