-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
PIDRUsage and PIDFDInfo should be marked unsafe #146
Comments
I'm out on PTO until July 22nd, then I can look at this.
…On Mon, Jul 15, 2024, 5:32 PM Ethan Pailes ***@***.***> wrote:
First of all, thanks for the crate! It's really nice to have these sorts
of foundational cross platform crates do the portability for you.
I beleive the two public traits PIDRUsage and PIDFDInfo allow for
potential unsoundness if a user implements them incorrectly.
https://doc.rust-lang.org/reference/unsafe-keyword.html#unsafe-traits-unsafe-trait
says that traits with this property ought to be marked as unsafe so that
users know there is something special they need to think about when
implementing them. Unfortunately because of the public nature of these
traits it will require a breaking version bump to fix the issue.
I went ahead and wrote #145
<#145> since the
fix seems pretty simple.
This came up as part of Google's unsafe review process for importing new
crates, and I'll need a new version to pick up the fix.
—
Reply to this email directly, view it on GitHub
<#146>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKF4LCLI2OLA5MFFK4O47LZMPTR3AVCNFSM6AAAAABK4ZIBDKVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQYDSMBTGY2TINA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Thanks! |
Back and looking at this now. Thanks for the issue. I didn't even know I'm not an expert in these topics, so bear with me... :-) TBH, I'm not sure I get it... Virtually all of the methods of this FFI crate call down into mac's libproc library, using a binding to the C lib, within an unsafe block. So, all the implementations are necessarily unsafe. I have reduced the scope of the unsafe blocks to the minimum possible, and not made all the functions unsafe, causing unsafe to "bubble up" all through the application. I am not sure what you see different about those two particular traits mentioned - or just the fact that those are traits as opposed to functions? There is also In the case of this lib, I think libproc will be the only implementation of the trait, and so marking it as unsafe is of limited value? Can you elaborate more on:
I am initially against a compatibility breaking change, with no change in functionality, that would cause rework among all the dependants unless I understand it better and see the benefits. |
With the caviat that I'm also not an I don't think there is any potential for actual crashes in the code as it exists today, this is more of a formal soundness issue where the contracts of the Rust language are being violated. One of the things that Rust promises you is that if you implement an ordinary trait (that is a non-unsafe trait), and you don't write any
I hadn't looked at all the other traits in this crate in detail, these were just the ones that got flagged in review, but now that you mention it they do seem like they might have the same issue. Sorry about that. I've updated the PR to reflect that.
My understanding is that we only need to mark these traits
In practice, when the program is running, I suspect none. It seems highly unlikely to me that someone would make their own implementation of these traits since you've already made traits for all the relevant structures. For me personally, it will mean that I'm allowed to use your very useful crate (thanks again!) as a google engineer, which I am pretty excited to be able to do, since we have fairly strict internal policy about unsoundness. I guess I can also gesture vaugely at benifits to the Rust community as a whole for maintaining quality standards and whatnot, but that's a little far removed from direct users. To be honest, for most users who don't care about unsoundness quite as much as me, this probably doesn't matter all that much, but I also doubt it will really break anyone (and if it does it means they were implementing these traits, so it is probably a good thing that they get broken as information about the constraints they need to know about). If I were in your shoes I would probably defer the change until the next planned breaking release (which fortunately looks like is coming up given #141). Also, I think that idea for sealing the traits is pretty clever and seems like a good idea even if it doesn't help with meeting the formal definition of soundness. |
For you and Google's purposes, how would you look at using this
https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
I *think* this would not cause a backwards compatibility break...
?
…On Mon, Jul 29, 2024, 3:10 PM Ethan Pailes ***@***.***> wrote:
With the caviat that I'm also not an unsafe expert and this issue was
originally caught by some real unsafe experts at google, I'll do my best to
explain my understanding of it.
I don't think there is any potential for actual crashes in the code as it
exists today, this is more of a formal soundness issue where the contracts
of the Rust language are being violated. One of the things that Rust
promises you is that if you implement an ordinary trait (that is a
non-unsafe trait), and you don't write any unsafe code yourself in that
trait implementation, there is no possibility of unsoundness. The unsafe
marker on traits, unlike unsafe blocks or functions, doesn't give you any
special powers, but just signals to users of the trait that this promise
that Rust normally makes is no longer guarenteed to be true. This property
is most importaint for code outside of your crate of course, but in terms
of formally making sure your code is sound you are supposed to mark these
kinds of traits as unsafe even within a crate (so it can make sense to have
a non-pub unsafe trait).
why those two traits and not more should be marked unsafe
I hadn't looked at all the other traits in this crate in detail, these
were just the ones that got flagged in review, but now that you mention it
they do seem like they might have the same issue. Sorry about that. I've
updated the PR to reflect that.
why not also all the functions that have unsafe in the implementations
My understanding is that we only need to mark these traits unsafe because
without it, we open up the possibility of someone writing a patch for the
crate or a client of the crate that doesn't touch any unsafe blocks yet
introduces new unsound behavior. I'm honestly a bit confused by this
though, because soundness is normally meant to be determined on the
compilation unit as a whole, so it seems like it is possible to do this
sort of introducing unsoundness without touching unsafe thing by changing
some data flow that leads to a normal unsafe block. It might just be that
for traits in particular, Rust makes the guarentee and the compiler is
in-principle allowed to rely on that guarentee for optimization which could
open up real world problems.
what you think the benefit for users of the lib would be
In practice, when the program is running, I suspect none. It seems highly
unlikely to me that someone would make their own implementation of these
traits since you've already made traits for all the relevant structures.
For me personally, it will mean that I'm allowed to use your very useful
crate (thanks again!) as a google engineer, which I am pretty excited to be
able to do, since we have fairly strict internal policy about unsoundness.
I guess I can also gesture vaugely at benifits to the Rust community as a
whole for maintaining quality standards and whatnot, but that's a little
far removed from direct users.
To be honest, for most users who don't care about unsoundness quite as
much as me, this probably doesn't matter all that much, but I also doubt it
will really break anyone (and if it does it means they were implementing
these traits, so it is probably a good thing that they get broken as
information about the constraints they need to know about). If I were in
your shoes I would probably defer the change until the next planned
breaking release (which fortunately looks like is coming up given #141
<#141>).
Also, I think that idea for sealing the traits is pretty clever and seems
like a good idea even if it doesn't help with meeting the formal definition
of soundness.
—
Reply to this email directly, view it on GitHub
<#146 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKF4LEZQU2PN26J2JDGGHDZOY5M7AVCNFSM6AAAAABK4ZIBDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJVHA4TSNBUGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I beleive that would still be rejected by the unsafe review since you can still introduce unsoundness by implementing these traits within the libproc crate. I'm also not really sure why that is preferable because sealing the traits is a breaking change as well, so it would require a semver bump either way. Really, I think the best thing would be to do both since sealing the traits imposes additional constraints on users of the crate beyond just marking them unsafe. Basically, I think sealing the traits adds better in-practice guard rails, but marking the traits unsafe is required to meet the formalism of the rust type system. As I mentioned before, I'm not an unsafe expert, but Google's unsafe experts that I've interacted with through our review process are really good and can probably explain this better. I'll see if I can get one to chime in on this thread since they may be able to be more precise than I can. |
Oh, I realize I said this would be a compatibilty break without really explaing why I think so. The reason is that right now users can theoretically have code that implements these traits and sealing the traits would break that code. In fact I think that all the same users who would get broken by marking the traits unsafe would get broken by sealing them and vice versa. The migration path would be much easier for unsafe (since users would just need to add a keyword to their impl instead of getting rid of it entirely), but all the same users would be broken. |
First of all, thanks for the crate! It's really nice to have these sorts of foundational cross platform crates do the portability for you.
I beleive the two public traits PIDRUsage and PIDFDInfo allow for potential unsoundness if a user implements them incorrectly. https://doc.rust-lang.org/reference/unsafe-keyword.html#unsafe-traits-unsafe-trait says that traits with this property ought to be marked as unsafe so that users know there is something special they need to think about when implementing them. Unfortunately because of the public nature of these traits it will require a breaking version bump to fix the issue.
I went ahead and wrote #145 since the fix seems pretty simple.
This came up as part of Google's unsafe review process for importing new crates, and I'll need a new version to pick up the fix.
The text was updated successfully, but these errors were encountered: