Panic freedom #3
Conversation
There was a problem hiding this comment.
Nice!
I think there are two different categories in here on overflow/underflow issues. This is definitely the typical class of issues in code like this. And I guess we didn't always review well.
- When the function is returning an error anyway, these should really be caught and errors returned.
- For non-failing functions it's a little tricky. Because we don't always want a
Result. At the same time would it be nice to do better here. One easy option would be_checkedvariants that return aResult. Another option would be returning a tuple with aboolon whether the result is correct or not. Then the caller can decide what to do with it. This would be breaking though.
In some cases overflows may also not be possible and we may want to treat code differently, depending on whether that can happen or not.
tls_codec/src/tls_vec.rs
Outdated
| /// The serialized len | ||
| #[inline(always)] | ||
| fn tls_serialized_byte_length(&$self) -> usize { | ||
| hax_lib::fstar!("admit ()"); // overflow |
There was a problem hiding this comment.
Yeah these kind of functions are difficult. We really don't want to return a Result but it's of course true that this may overflow.
In most cases this can't happen. But on a TlsVecU32 (or the slice version) on a 32 bit system this may indeed happen.
We could add a variant of the function with a _checked postfix and return a Result there.
| // large and write it out. | ||
| let (tls_serialized_len, byte_length) = $self.get_content_lengths()?; | ||
|
|
||
| hax_lib::fstar!("admit ()"); // Need to prove unwrap for usize to U24 |
There was a problem hiding this comment.
Maybe U24 should be implemented separately here to allow more checks?
tls_codec/src/tls_vec.rs
Outdated
|
|
||
| #[inline] | ||
| fn index(&self, i: usize) -> &T { | ||
| hax_lib::fstar!("admit ()"); // index precondition |
There was a problem hiding this comment.
Similar to the length. Maybe we should have a checked version here.
There was a problem hiding this comment.
Here it is really the caller's responsibility to provide a valid index so it makes sense to have a precondition. I switched to this in the latest version.
tls_codec/src/lib.rs
Outdated
| fn from(value: U24) -> usize { | ||
| const LEN: usize = core::mem::size_of::<usize>(); | ||
| let mut usize_bytes = [0u8; LEN]; | ||
| hax_lib::assume!(LEN == 8); // https://github.com/cryspen/hax/issues/1702 |
There was a problem hiding this comment.
This may also be 4 on 32 bit systems. Maybe we should just assume >= 4? We don't want to support smaller than 32 bit systems here.
There was a problem hiding this comment.
Yes, I wanted to do LEN == 4 || LEN == 8. Unfortunately this doesn't work because of a hax bug (the issue that is linked in the comment).
Thanks for the feedback. I'll have a look at the suggestions and push a new version with some more disruptive changes to fix some over/underflows. |
I just pushed a new version where additions and subtractions are replaced by |
|
@franziskuskiefer I made a try for having a checked version for |
2 participants
This is a first step for proving panic freedom. Extraction using hax and F* type checking works, but some parts are admitted (with comments). The most common reasons for admits are:
usizetoU24is unsafe (should work because it is used only on valid values but that makes it hard to prove)debug_asserts are not proven for nowsize_ofis inlined by hax in types but not values hax#1702eqtypehax#1700&mutparameters hax#1584core::cmp::PartialEq::eqby F* equality which is sometimes not available hax#1704The current state works with hax branch
tls-codec-panic-freedom