mozilla / uniffi-rs

a multi-language bindings generator for rust
https://mozilla.github.io/uniffi-rs/
Mozilla Public License 2.0
2.48k stars 211 forks source link

`HandleAlloc::clone_handle` and `consume_handle` should be marked unsafe #2164

Open mgeisler opened 1 week ago

mgeisler commented 1 week ago

The clone_handle and consume_handle methods in the HandleAlloc trait should be unsafe:

https://github.com/mozilla/uniffi-rs/blob/cd38ccea8236df7d93aff336c325a3a8e524af5d/uniffi_core/src/ffi_converter_traits.rs#L629-L637

The problem is that you can create a Handle with any u64 value you want in safe Rust:

let h = Handle::from_raw(42);
h.clone_handle(); // calls the unsafe Arc::increment_strong_count on 42

I discussed this with @badboy at RustFest and there is a chance that none of the generated bindings code ever calls it like that. If so, then it should be possible to add unsafe to the trait methods and propagate this upwards.

badboy commented 1 week ago

One way this is called is through this:

https://github.com/mozilla/uniffi-rs/blob/cd38ccea8236df7d93aff336c325a3a8e524af5d/uniffi_core/src/ffi_converter_traits.rs#L459

which gets called from e.g. this: https://github.com/mozilla/uniffi-rs/blob/main/uniffi_core/src/ffi/rustfuture/mod.rs#L75 which ultimately ends up being a FFI function. So the handle should indeed only be coming from generated code, so if the other uses are correct, the Handle received in clone_handle should be correct. Maybe we should still have that safety documentation somewhere.

Given we make assumptions about a Handle maybe the from_raw should already be unsafe and have additional documentation.

Again because this should not be called outside of generated code I still think the current code is sound, though would benefit from making those assumptions and guarantees more clear (through docs, unsafe and/or assertions)