TheButlah / ovr_overlay

Rust OpenVR bindings using an up-to-date OpenVR and autocxx
https://docs.rs/ovr_overlay
Other
7 stars 8 forks source link

Soundness bug: we are aliasing in `&mut *Manager` #16

Open TheButlah opened 1 year ago

TheButlah commented 1 year ago

I just found this out from the cnlohr discord that the systems returned to us from openvr actually all point to the same object (same pointer address). So our current approach of passing to the user a new mutable ref each time we call the functions is super unsound, as its violating rust's aliasing XOR mutability principle.

We will need to rethink how to structure the code to make it possible to treat most functions as not thread safe by default, but allow some functions to be called concurrently.

kitlith commented 1 year ago

We could fix the aliasing by saying that we call methods on a shared reference, and it uses internal mutability. It doesn't fix the potential thread safety issues, but having read that conversation, it sounds like the most we'll probably see is a little raciness, and openvr isn't built to be perfectly thread safe. so we can provide this as a best effort wrapper, and slap a warning on it that it is questionably thread-safe, but has been left that way for convenience and not having the energy to dig into the internals enough to figure out exactly what is thread safe and what isn't.

TheButlah commented 1 year ago

Whatever we do, I think we should be maximally pessimistic on the guarantees that openvr gives us and at all costs keep the code sound.

I have 0 interest in debugging openvr internal data races. We should make it !Sync if we need to. More likely though, we should just be doing mutability differently.