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

More protocol safety improvements #478

Merged

Conversation

nicholasbishop
Copy link
Member

This is a change I've been noodling on for a bit related to #359. The basic issue is that in order to use protocols fully safely, with no chance of either the handle or protocol being modified or removed while in use, we need to ensure that protocols are opened in exclusive mode with the correct agent handle set. We've made some good steps towards that by deprecating and marking unsafe things like handle_protocol, but there are still unsafe ways to use open_protocol.

To resolve this (to the extent possible given the design of UEFI), I've added a new open_protocol_exclusive function. This function is fully safe; it ensures that the correct agent is set through the use of a new global image handle set by the #[entry] macro, and always opens the protocol in exclusive mode. This is a much nicer interface to use than open_protocol, since you only have to provide the Handle you want to open.

There are still some cases where the full power of open_protocol is needed; uefi-test-runner has some examples of this. For those cases though the caller needs to be aware that they are responsible for ensuring that the handle and protocol are valid until the protocol is no longer in use, so I've marked open_protocol as unsafe. Since most uses can be replaced with open_protocol_exclusive, this is hopefully not too much of a burden.

Having a global image handle and marking open_protocol as unsafe are fairly significant changes to the API (although notably we haven't done a release yet that includes #434, so users of open_protocol will already have to make some code adjustments when they next upgrade), but I think it will provide a nice boost in both safety and ease of use.

Copy link
Collaborator

@GabrielMajeri GabrielMajeri left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this issue. I've read through the implementation carefully and left some ideas for minor usability improvements.

@nicholasbishop nicholasbishop force-pushed the bishop-global-image-7 branch 3 times, most recently from d584e18 to b22e119 Compare August 21, 2022 17:11
Describe details of what it does and what the entry point function must
look like. Note that since `uefi-macros` does not depend on `uefi`
(except as a dev-dependency), short links to `uefi` types cannot be
used. Full links to docs.rs are used instead.
Copy link
Collaborator

@GabrielMajeri GabrielMajeri left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Went through everything again and confirmed the code is as good as it can get now :) it's ready for merging.

@nicholasbishop
Copy link
Member Author

No worries at all, thanks for reviewing :)

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.

2 participants