I am trying to implement safe rust wrappers for a library that returns an opaque void * pointer, which serves as a context that is used by (almost) all functions of the library. I have looked at the suggested mechanism in the Nomicon here. However I would also like the structure to be Send and Sync (will be wrapping it inside Arc). Even though this may eventually be avoided, for now I would say this is a restriction. Also, I would like to make sure that only a single instance of a client exists at a given time. (It is possible to create/drop and re-create).
I have come with the implementation that looks like the following and this works as expected. I am looking at if there are any Safety/Soundness issues that I have overlooked?
Thanks a lot.
Abhijit
// A Wrapper over an opaque C void * pointer from FFI
// Not included the emApty field and `PhantomData` markers intentionally.
// See below
struct Client;
// Our Error type
struct ClientError
impl Client {
pub fn new() -> Result<Self, ClientError> {
client_new_internal()
}
}
impl Drop for Client {
fn drop(&mut self) {
client_drop_internal()
}
}
// Internal Unsafe functions and some global state in a separate module
static CONTEXT: *mut std::os::raw::c_void = std::ptr::null_mut();
static CLIENT_MUTEX: Mutex<()> = Mutex::new();
fn client_new_internal() -> Result <Client, Error> {
// This ensures that only one thread can succeed.
let _guard = CLIENT_MUTEX.lock().unwrap();
unsafe {
// Someone else has initialized a client already so this is not allowed now.
if !CONTEXT.is_null() {
return Err(Error)
}
CONTEXT = lib_init();
if CONTEXT.is_null() {
return Err(Error)
} else {
Ok(Client)
}
}
}
fn client_drop_internal() {
lib_close();
CONTEXT = std::ptr::null();
}
There are a lot of fixes to be made before this will even compile, so your implementation can't really look like that. It's impossible to give a soundness review without a clearer picture.
Because, as long as I can make sure only one instance of the Client exists, there is no need to protect it's access every time the CONTEXT is used.
The reason I am not using the private internal field is I would like this Client to be wrapped in an Arc and if I use private fields with PhantomData, I cannot implement Sync and Send.
The basic idea is CONTEXT is correctly initialized only once and this remains initialized as long as the Client structure is in place.
The actual code works as expected (in a few tests), so that is per se not a problem, the question is - Does this scheme have any safety/soundness issues that I have not considered.
Sorry - I should have mentioned here the code presented is just a sketch of implementation and may not compile as it is.
The idea is that - the CONTEXT in the wrapped library should be initialized only once and it remains correctly initialized as long as the Client structure is in scope. And the library can then be accessed from any thread (by using methods of the Client structure) and I don't have to do unsafeSend and Sync implementations for the client. (And if I wrap the PhatnomData as suggested in the Nomicon link above, I cannot implement Send and Sync).
The actual implementation actually works as expected (in a few quick tests), that is - only one client can be initialized at any given point of time. I am not entirely sure what is the easiest way of creating that on playground without a public crate for the wrapped FFI library. Will try to figure out a way of doing it, but from providing the sketch of the idea point of view I believe the above code should still make sense.
Can be created by anyone, so you need a private field even if it's just struct Name(()) to prevent it from being constructable.
It sounds like what you're doing is only really tangentially related to that nomicon section. Youre not trying to create a type to use as the pointer type *mut Name, you're just trying to improve the ergonomics of using the void pointer from rust. Some of the advice on that page may not really apply.
Sorry - I missed your original point! This needs to be added to the Client struct to prevent it to be initialized by anyone.
Also, yes, I am trying to improve the ergonomics (or more accurately suit my specific needs), so not all the advice from there would apply. But are there very obvious safety/soundness issues - is the question.
I think the only way for unsoundness to creep in under the scheme you've described is if the library is not actually thread safe and you allow the context pointer to be accessed on other threads.
Obviously that doesn't include whether or not all of the calls that actually use the pointer are sound though, that's a different question entirely.
Since it is guaranteed that only one instance of the client can ever be created (and client_drop_internal is called from the drop of that instance), it is guaranteed that this function cannot be called from two threads simultaneously. Am I missing something?