-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666
Conversation
Currently the macros build_slice and cast_pointer wrap their code in unsafe not requiring the containing function to be unsafe, but clippy recommends that any public function working with raw pointers be marked unsafe. To comply with this idiom, remove unsafe from the macros, requiring any function that uses these macros to be unsafe itself. Reference: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsaf
Now that many of the ffi functions are marked unsafe, remove unnecessary unsafe blocks within these functions.
Suppresses some clippy lints that have more to do with style than anything else, to reduce the amount of noise in the clippy output.
CC: @chifflier @dbcfd |
This seems a bit counter intuitive to me. Should it lead to the functions just be as minimal as possible to pass the objects created from the raw pointers into 'safe' functions? Ultimately we pass around data based on unsafe input, so it seems a bit strange to classify more code as unsafe while fundamentally nothing changes. All rust needs to know that we vouch for the validity of the raw pointers, right? |
Btw I don't object to refactoring to make code checkers happy, but I would like to see if we can avoid having more of the logic inside |
Yes, that's the usual recommendation for FFI code: write
There are usually two kinds of
The former is quite expected at FFI borders, the latter should be avoided unless really justified. The idiomatic Rust way is what you and @ish described (and I think in fact everyone agrees): FFI functions are usually marked So, my suggestion would be to indeed mark |
I agree with Pierre's recommendation and approach. |
So I think its a bit more than making code checkers safe, but keeping up with generally accepted practices, and this is one lint I'd like to get rid of. I've also felt that when seeing these unsafe blocks, you really have to consider the code after them as well as being unsafe. So I think moving to a practice where the ffi functions are unsafe, but do as little as possible makes sense for us. |
But isn't the consequence of this that we should consider all our Rust code unsafe? It's all about dealing with data from unsafe origin. |
Thats somewhat true, and is why you should consider any extern "C" |
Oh, I see now more what you're saying. |
I would more explain that as "if the input arguments of the Rust code are trusted/verified (resp. untrusted), then the Rust code is safe (resp. unsafe)". One cannot expect miracles from Rust: for ex, if you pass a buffer and a wrong (too short) length as arguments, then the built slice will trigger something wrong inside Rust safe code. |
pub type StateTxFreeFn = extern "C" fn (*mut c_void, u64); | ||
pub type StateGetTxFn = extern "C" fn (*mut c_void, u64) -> *mut c_void; | ||
pub type StateGetTxCntFn = extern "C" fn (*mut c_void) -> u64; | ||
pub type StateTxFreeFn = unsafe extern "C" fn (*mut c_void, u64); |
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.
I think the linter is specifically complaining about these extern "C"
functions.
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.
There is no complaint here, these types just had to be updated after the offending functions were updated.
@@ -25,7 +25,7 @@ use std; | |||
use x509_parser::parse_x509_der; | |||
|
|||
#[no_mangle] | |||
pub extern "C" fn rs_rdp_to_json(tx: *mut std::os::raw::c_void) -> *mut JsonT { | |||
pub unsafe extern "C" fn rs_rdp_to_json(tx: *mut std::os::raw::c_void) -> *mut JsonT { |
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.
The linter is probably not complaining about these, since they're exposing an interface for C to call.
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.
The linter was complaining about this function as it dereferences a raw pointer, and suggest the function should be unsafe, not just block.
This is indeed not required, but the good practice (enforced by clippy) is to add an
I agree with that |
Can we enforce this somehow? I agree with it as well. While I do not see a way to enforce it, with a naming convention we could easily spot it.. For example, if all FFI functions are prefixed with "rs_", or follow our C style of SomeLongFunctionName, it would be easy to spot during review. |
I think I wasn't clear at some point: The Marking a block or a function as |
The keyword is definitely nice for identifying code which has to deal with raw input, but I don't have strong opinions about whether the keyword is applied at the function level, or at the code level, since it is more of an indication to the rust developer. Both methods still require examining the code to make sure it is handling the raw input appropriately. Definitely would like to see less logic in functions dealing with raw input, and more in a safe function that is called from the |
Ah. In that case, all of our rust functions exposed to C would likely be unsafe. Which is fine, especially if it forces us to split out safe code from unsafe code, so the exposed interfaces only deal with the untrusted input, and the remainder of the logic is in safe functions called from the exposed interface. |
This comment has been minimized.
This comment has been minimized.
Continued at #6280 which also removes transmute. |
Currently the macros build_slice and cast_pointer wrap their code in unsafe not
requiring the containing function to be unsafe, but clippy recommends that any
public function working with raw pointers be marked unsafe. To comply with this
idiom, remove unsafe from the macros, requiring any function that uses these
macros to be unsafe itself.
Reference:
https://rust-lang.github.io/rust-clippy/master/index.html#not_unsaf
The next step would be to find the ffi funcations that use an unsafe block
and mark them unsafe, removing the unsafe blocks.
PRScript output (if applicable):