-
Notifications
You must be signed in to change notification settings - Fork 61
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
Use rust's own bool type in abstraction crate #61
Conversation
97182bd
to
b6c62ee
Compare
Ref for the data layout, which is also present in a comment in the commit: https://doc.rust-lang.org/reference/type-layout.html#primitive-data-layout |
b6c62ee
to
d2a795a
Compare
The Bbool type was translating between the FFI's CK_BBOOL and rust's own bool, by providing a backing intermediate type. But the storage for a bool is guaranteed to be large enough for the job, and the point of conversion from byte back to bool is already structured to check for validity. Signed-off-by: Keith Koskie <[email protected]>
d2a795a
to
1b34cc0
Compare
@@ -617,7 +617,7 @@ impl Attribute { | |||
| Attribute::Verify(_) | |||
| Attribute::VerifyRecover(_) | |||
| Attribute::Wrap(_) | |||
| Attribute::WrapWithTrusted(_) => 1, | |||
| Attribute::WrapWithTrusted(_) => std::mem::size_of::<bool>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extreme corner case -> CK_BBOOL
is defined as an unsigned char
. What if the architecture defines a char
to not be 8-bits (e.g. not POSIX compliant).
Googling this question turns up two TI DSPs that if someone implemented a cryptoki token on I'd question their sanity.
I think this is fine, but just wanted to pose the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It may be possible at an architectural level, but the standard itself excludes that possibility by explicitly saying "unsigned 8-bit value".
At this particular location, the type being sized is exactly the type that the raw pointer points to. So if, say, CK_BBOOL somehow gets bigger because of a weird backing c_uchar, the ptr and len calls still match each other. And the provider should return CKR_BUFFER_TOO_SMALL
or some other error code.
This looks fine by me. And since Rust's |
Funny enough, there is some ambiguity here. CK_TRUE is defined to 1, but the range of valid values is much more lax:
Should the range error condition on the u8->bool be removed? |
That's why I love this spec 🙄
Yeah probably to be as spec-compliant as possible (which I have droned on about in the past). |
Signed-off-by: Keith Koskie <[email protected]>
Head branch was pushed to by a user without write access
If it's any consolation, I don't like it either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks! 👍🏻
The Bbool type was translating between the FFI's CK_BBOOL and rust's own bool, by providing a backing intermediate type. But the storage for a bool is guaranteed to be large enough for the job, and the point of conversion from byte back to bool is already structured to check for validity.