[patina_dxe_core] Add Missing Safety Comments #1232
Conversation
github-actions
bot
added
the
impact:non-functional
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Raymond-MS
force-pushed
the
personal/rdiaz/SafetyComments2
branch
from
9fc749a to
d6b3944
Compare
| .or(Err(EfiError::InvalidParameter))?; | ||
| let driver_binding_interface = driver_binding_interface as *mut efi::protocols::driver_binding::Protocol; | ||
| // Safety: driver_binding_interface is validated above, would have been caught with the .or and ? above with | ||
| // the loop being terminated. |
There was a problem hiding this comment.
This is again relying on get_interface_for_handle to give us a valid pointer back
There was a problem hiding this comment.
I am sorry, I am a little confused, is that not what is mentioned? driver_binding_interface is set twice. Once with the .get_interface_for_handle and then again right after as a mutable pointer. We validate the pointer with the .or, right? Is there a different way you wanted me to word this?
| for handle in driver_binding_handles { | ||
| match PROTOCOL_DB.get_interface_for_handle(handle, efi::protocols::driver_family_override::PROTOCOL_GUID) { | ||
| Ok(protocol) => { | ||
| // Safety: Pointer is validated using .expect(), will panic if .as_mut() returns a NULL pointer |
There was a problem hiding this comment.
I know we've been relatively loose with this overall, but there is more than just the pointer being null that needs to be considered. Such as:
- Aligned for the type
- Points to a properly initialized type that is not dangling
- Aliasing - Since this is mutable no other references may violate aliasing expectations to the address
- Lifetime - This reference won't outlive the region
Most of these are usually handled and accounted for. I just want to make sure they're actually being considered equally to whether the pointer is null and getting called out in comments as needed.
There was a problem hiding this comment.
I am sorry Michael, was this a general comment when investigating unsafe blocks or does this relate to this line specifically? In this situation, we don't need to validate all of those things, right?
There was a problem hiding this comment.
Just a general comment to make sure that's being taken into account for each case since a lot end up only describing the null pointer aspect.
Raymond-MS
force-pushed
the
personal/rdiaz/SafetyComments2
branch
from
d6b3944 to
8b7fd50
Compare
3 participants
Description
Added missing Safety comments to the following files in patina_dxe_core:
allocator.rs
config_tables.rs
cpu.rs
debugger_reload.rs
driver_services.rs
Relates to: #583
How This Was Tested
Built SBSA successfully in patina-dxe-core-qemu.
Integration Instructions
N/A