(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666

jasonish · 2020-03-11T20:46:32Z

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):

PR jasonish-pcap: https://buildbot.openinfosecfoundation.org/builders/jasonish-pcap/builds/448
PR jasonish: https://buildbot.openinfosecfoundation.org/builders/jasonish/builds/805

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.

jasonish · 2020-03-11T20:48:33Z

CC: @chifflier @dbcfd

victorjulien · 2020-03-12T07:28:19Z

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?

victorjulien · 2020-03-12T07:29:42Z

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 unsafe blocks.

chifflier · 2020-03-12T09:16:47Z

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?

Yes, that's the usual recommendation for FFI code: write unsafe wrappers (mostly to wrap pointers and cast types), and call safe (classic) functions as soon as possible.

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?

There are usually two kinds ofunsafe:

code that must access objects outside Rust's memory checks: syscalls, calling C, wrapping arguments
voodoo code that casts objects or performs lifetime changes etc.

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 unsafe (if they dereference pointers in particular), and must perform as little actions as possible (immediately call a Rust function, and only handle wrapping to/from C).

So, my suggestion would be to indeed mark unsafe these functions, but in the same time to ensure they perform only the unsafe part, and call safe code as soon as possible. This may require splitting functions, but should not be bad since Rust tends to try inlining functions as much as possible.

dbcfd · 2020-03-12T15:00:03Z

I agree with Pierre's recommendation and approach.

jasonish · 2020-03-12T15:02:32Z

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.

victorjulien · 2020-03-12T15:04:28Z

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.

jasonish · 2020-03-12T15:11:08Z

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"
function unsafe, as the caller (our C) needs to ensure its passing valid
data as well.

dbcfd · 2020-03-12T15:11:11Z

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.

Oh, I see now more what you're saying. extern "C" functions called from C code should not be marked unsafe, as they have to be marked in this fashion for interop. We should not be calling extern "C" functions from Rust code that have been defined in Rust code. We should be exposing a normal rust interface that is invoked, and the extern "C" version should only be invoked by C code.

chifflier · 2020-03-12T15:11:47Z

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.

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.

dbcfd · 2020-03-12T15:12:55Z

rust/src/parser.rs

-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);


I think the linter is specifically complaining about these extern "C" functions.

There is no complaint here, these types just had to be updated after the offending functions were updated.

dbcfd · 2020-03-12T15:13:18Z

rust/src/rdp/log.rs

@@ -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 {


The linter is probably not complaining about these, since they're exposing an interface for C to call.

The linter was complaining about this function as it dereferences a raw pointer, and suggest the function should be unsafe, not just block.

chifflier · 2020-03-12T15:15:13Z

extern "C" functions called from C code should not be marked unsafe, as they have to be marked in this fashion for interop.

This is indeed not required, but the good practice (enforced by clippy) is to add an unsafe keyword if it dereferences an input pointer (because it cannot guarantee safety).

We should not be calling extern "C" functions from Rust code that have been defined in Rust code. We should be exposing a normal rust interface that is invoked, and the extern "C" version should only be invoked by C code.

I agree with that

victorjulien · 2020-03-12T15:20:26Z

We should not be calling extern "C" functions from Rust code that have been defined in Rust code. We should be exposing a normal rust interface that is invoked, and the extern "C" version should only be invoked by C code.

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.

chifflier · 2020-03-12T15:22:15Z

I think I wasn't clear at some point: extern "C" functions are not required to be unsafe. Imagine a function adding two integers, it would be extern "C" but not unsafe.

The unsafe keyword has the usual meaning: this function is doing something that could be dangerous. The only difference is that, while the usual approach is to have an unsafe block, there is an exception for the specific case when you dereference a pointer. In that case, clippy suggest to mark the whole function as unsafe (not the block), so it appears in the documentation.

Marking a block or a function as unsafe is always possible, it's just the usual rules on when to choose which approach to use.

dbcfd · 2020-03-12T15:25:52Z

extern "C" functions called from C code should not be marked unsafe, as they have to be marked in this fashion for interop.

This is indeed not required, but the good practice (enforced by clippy) is to add an unsafe keyword if it dereferences an input pointer (because it cannot guarantee safety).

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 extern "C" function.

dbcfd · 2020-03-12T15:36:59Z

In that case, clippy suggest to mark the whole function as unsafe (not the block), so it appears in the documentation.

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.

jasonish · 2021-07-27T14:07:30Z

Continued at #6280 which also removes transmute.

jasonish added 3 commits March 11, 2020 11:23

rust: remove unnecessary unsafe

00b7175

Now that many of the ffi functions are marked unsafe, remove unnecessary unsafe blocks within these functions.

rust: allow some clippy lints without warning

45e9867

Suppresses some clippy lints that have more to do with style than anything else, to reduce the amount of noise in the clippy output.

jasonish requested a review from victorjulien as a code owner March 11, 2020 20:46

dbcfd reviewed Mar 12, 2020

View reviewed changes

jasonish mentioned this pull request Jun 23, 2020

Http2 v10 #4985

Closed

jasonish added the preview label Jul 13, 2020

catenacyber marked this pull request as draft July 23, 2020 14:53

This comment has been minimized.

Sign in to view

This was referenced Jul 23, 2021

(RFC) rust: mark extern "C" ffi functions as unsafe - v4 #6276

Closed

(RFC) rust: mark extern fn's unsafe; remove tranmute - v7 #6280

Closed

jasonish closed this Jul 27, 2021

jasonish mentioned this pull request Aug 18, 2021

rust: remove transmute; make fn's unsafe - v8 #6299

Closed

jasonish deleted the rust/unsafe-ffi/v1 branch August 23, 2021 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666

(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666

jasonish commented Mar 11, 2020

jasonish commented Mar 11, 2020

victorjulien commented Mar 12, 2020

victorjulien commented Mar 12, 2020

chifflier commented Mar 12, 2020

dbcfd commented Mar 12, 2020

jasonish commented Mar 12, 2020

victorjulien commented Mar 12, 2020

jasonish commented Mar 12, 2020 •

edited

Loading

dbcfd commented Mar 12, 2020

chifflier commented Mar 12, 2020

dbcfd Mar 12, 2020

jasonish Mar 12, 2020

dbcfd Mar 12, 2020

jasonish Mar 12, 2020

chifflier commented Mar 12, 2020

victorjulien commented Mar 12, 2020 •

edited by jasonish

Loading

chifflier commented Mar 12, 2020

dbcfd commented Mar 12, 2020

dbcfd commented Mar 12, 2020

This comment has been minimized.

jasonish commented Jul 27, 2021

(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666

(RFC) rust: mark extern "C" (ffi) functions as unsafe #4666

Conversation

jasonish commented Mar 11, 2020

jasonish commented Mar 11, 2020

victorjulien commented Mar 12, 2020

victorjulien commented Mar 12, 2020

chifflier commented Mar 12, 2020

dbcfd commented Mar 12, 2020

jasonish commented Mar 12, 2020

victorjulien commented Mar 12, 2020

jasonish commented Mar 12, 2020 • edited Loading

dbcfd commented Mar 12, 2020

chifflier commented Mar 12, 2020

dbcfd Mar 12, 2020

Choose a reason for hiding this comment

jasonish Mar 12, 2020

Choose a reason for hiding this comment

dbcfd Mar 12, 2020

Choose a reason for hiding this comment

jasonish Mar 12, 2020

Choose a reason for hiding this comment

chifflier commented Mar 12, 2020

victorjulien commented Mar 12, 2020 • edited by jasonish Loading

chifflier commented Mar 12, 2020

dbcfd commented Mar 12, 2020

dbcfd commented Mar 12, 2020

This comment has been minimized.

jasonish commented Jul 27, 2021

jasonish commented Mar 12, 2020 •

edited

Loading

victorjulien commented Mar 12, 2020 •

edited by jasonish

Loading