Improve usage of unsafe blocks #18
Conversation
|
|
||
| fn try_from(tss_signature: TPMT_SIGNATURE) -> Result<Self> { | ||
| impl Signature { | ||
| pub unsafe fn try_from(tss_signature: TPMT_SIGNATURE) -> Result<Self> { |
There was a problem hiding this comment.
I think that is a very good decision to put the unsafe on the method here as we can not trust that the tss_signature argument is always well-composed. The caller of this method needs to make the decision as only him knows where the parameter comes from.
There was a problem hiding this comment.
I was thinking: "what if the method is not public?"
But even so, it would maybe be wrong to put it safe anyway.
There was a problem hiding this comment.
Maybe a comment would be nice to explain those things? Or that could be part of the documentation of the method itself.
There was a problem hiding this comment.
Yeah, I decided to just leave it to documentation, a comment isn't as visible for users
|
|
||
| // Close the context. | ||
| unsafe { tss2_esys::Esys_Finalize(&mut esys_context.into_raw() as *mut *mut ESYS_CONTEXT) }; | ||
| unsafe { tss2_esys::Esys_Finalize(&mut esys_context.into_raw()) }; |
| @@ -412,7 +412,7 @@ impl Context { | |||
|
|
|||
| if ret.is_success() { | |||
| let signature = unsafe { MBox::from_raw(signature) }; | |||
| Ok((*signature).try_into()?) | |||
| Ok(unsafe { Signature::try_from(*signature)? }) | |||
There was a problem hiding this comment.
Maybe a comment to explain why this is safe would make us better unsafe citizens 👷
There was a problem hiding this comment.
Same everywhere we use the unsafe block actually in my opinion. The same way we kind of have to justify when we panic explicitely.
This commit improves our handling of unsafe blocks. It moves the unsafe label to methods that could lead to undefined behaviour instead of hiding the lack of safety inside the method. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
Add a MAINTAINERS file
This commit improves our handling of
unsafeblocks. It moves theunsafelabel to methods that could lead to undefined behaviourinstead of hiding the lack of safety inside the method.
All remaining
unsafeblocks should be safe to a sane level - we control the inputs to the function calls within and can guarantee, based on trust (usually in the TSS stack below) that the calls are safe.Thread safety is offered by all
Contextstruct methods requiring mutable references toself.Fixes #2