Thread Safety: EventSubsystem and TimerSubsystem violate Sync, allows using an Sdl context from multiple threads.

[LunarLambda]

According to sdl.rs, the event and timer subsystems can be safely accessed from multiple threads. For this reason, they unsafe impl Sync.

The Rust standard library defines Sync as such:

a type T is Sync if and only if &T is Send. In other words, if there is no possibility of undefined behavior (including data races) when passing &T references between threads.

This is an important invariant that must be upheld for unsafe impl Sync to be sound. However, subsystems contain an Rc<SubsystemDrop>. Rust makes it very clear that Rc is not Send:

An example of a non-Send type is the reference-counting pointer rc::Rc. If two threads attempt to clone Rcs that point to the same reference-counted value, they might try to update the reference count at the same time, which is undefined behavior because Rc doesn't use atomic operations.

This allows us to pass a &TimerSubsystem between threads. From there, we could clone it, and cause a race on the contained Rcs changing their reference count, or obtain a handle to the main Sdl context, and access other, not thread-safe subsystems.

Here is an example demonstrating the latter:

use std::thread;

fn main() {
    let sdl2 = sdl2::init().expect("failed to initialize SDL2");
    let timer_system = sdl2.timer().expect("failed to initialize timer subsystem");

    // Contrived, but definitely safe.
    // Another way to get a 'static subsystem reference is through the use of a global,
    // i.e. via `lazy_static`. This is also the *only* way to actually use a subsystem on another thread.
    let static_timer: &'static _ = Box::leak(Box::new(timer_system));

    let _a = thread::spawn(move || {
        // Now that we have sent our TimerSubsystem to another thread...
        let timer = static_timer;

        // ...we now have an Sdl context on two threads.
        let _smuggled_sdl = timer.sdl();
    });
}

One fix for this would be to simply make all subsystems nosync, as the current structure does not allow any way for this to be sound. Additionally, without Send, we have to have a &'static Subsystem to even use these subsystems from another thread, which means proper deinitialization via SDL_QuitSubsystem or SDL_Quit cannot occur.

Alternatively, although I have not tested this yet, using Arc for Sdl, opting out of Send and Sync, and also using Arc for EventSubsystem and ThreadSubsystem may be enough to fix things.

[Lokathor]

Upgrading from Rc to Arc is better, but as far as I understand SDL2 the events system in particular is not send and in general SDL2 is absolutely not thread safe.

The only part of the API that's documented as thread safe is Audio.

Video/Events is absolutely not thread safe.

• On Mac, accessing video from a non-main thread is UB
• On Windows accessing video/events from a different thread than initialized that system is UB.

[LunarLambda]

some event and timer function SDL2 says are explicitly thread safe. The issue is the thread-unsafe refcounting (upgrade to Arc), and (supposedly) thread-safe subsystems exposing the definitely not thread-safe Sdl context.

[LunarLambda]

for testing I wrote a quick gist that roughly mocks the sdl2-rs API, but thread-safely.

[LunarLambda]

following some further discussion with @Lokathor I think I'd rather be in favour of the nosync approach. More of a breaking change, but the thread-safety of the event and timer functions is already questionable even without this particular issue.

[andrewhickman]

Its possible to remove the Box::leak call by using scoped threads (e.g. https://docs.rs/crossbeam/0.8.0/crossbeam/fn.scope.html), which allow passing non-static data to other threads

[Lokathor]

1. you can pass a Box to another thread without leaking it.
2. this is Rc, not Box.
3. Many these functions aren't thread safe on the C side, even if if Arc is used on the Rust side.

[LunarLambda]

Its possible to remove the Box::leak call by using scoped threads (e.g. https://docs.rs/crossbeam/0.8.0/crossbeam/fn.scope.html), which allow passing non-static data to other threads

Thanks for pointing this out. I had heard about scoped threads while preparing this issue but presumed them to be a nightly feature rather than from a external crate. Either way I hope my initial example shows the bug definitely lies with rust-sdl2, and not any other dependencies of mine

1. you can pass a `Box` to another thread without leaking it.

2. this is `Rc`, not `Box`.

3. Many these functions aren't thread safe on the C side, even if if `Arc` is used on the Rust side.

I believe I tried 1. but Box is only Send or Sync if its contents are, so you specifically need to pass a shared reference to the subsystem. I may be misunderstanding though, if you can provide a better example that would be appreciated.

[Lokathor]

Ah, right. You are correct. Boxes are only Send when the content is. Sorry, I misunderstood where the box was being talked about.

Honestly I'm not the most familiar with sdl2 the rust crate, I just know a fair amount about SDL2 the C library. So offhand I don't remember the internal details of the timer subsystem as given in the Rust crate. However, it shouldn't be Send, I can tell ya that.