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

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

Closed
mgeisler opened this issue Jun 21, 2024 · 3 comments · Fixed by #2192
Closed

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

mgeisler opened this issue Jun 21, 2024 · 3 comments · Fixed by #2192

Comments

@mgeisler
Copy link
Contributor

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

fn clone_handle(handle: Handle) -> Handle {
unsafe { Arc::increment_strong_count(handle.as_pointer::<T>()) };
handle
}
fn consume_handle(handle: Handle) -> Arc<Self> {
unsafe { Arc::from_raw(handle.as_pointer()) }
}
}

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
Copy link
Member

badboy commented Jun 21, 2024

One way this is called is through this:

Self::consume_handle(Self::clone_handle(handle))

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)

@mhammond
Copy link
Member

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

Yeah, I agree that this seems more about from_raw then the assumption a handle is valid - otherwise we'd need to treat many more things as unsafe too?

Best I can tell though, we could do all the above without many changes at all - but does that even make sense?

@mhammond
Copy link
Member

otherwise we'd need to treat many more things as unsafe too?

Not sure what I was thinking really - all use of handles is ultimately unsafe and all callers of these already are unsafe, so "all of the above" it is :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants