From a27bebf03115c2f8eca8a60381ec4ecbe22061fd Mon Sep 17 00:00:00 2001 From: Martin Mills Date: Wed, 31 Aug 2022 10:48:48 -0700 Subject: [PATCH 1/2] Make SdlDrop and SubsystemDrop safer to use --- src/sdl2/keyboard/mod.rs | 2 +- src/sdl2/mouse/mod.rs | 2 +- src/sdl2/sdl.rs | 243 +++++++++++++++++++++------------------ 3 files changed, 130 insertions(+), 117 deletions(-) diff --git a/src/sdl2/keyboard/mod.rs b/src/sdl2/keyboard/mod.rs index 10222222fda..5775217c3c5 100644 --- a/src/sdl2/keyboard/mod.rs +++ b/src/sdl2/keyboard/mod.rs @@ -175,7 +175,7 @@ impl crate::VideoSubsystem { /// let focused = sdl_context.keyboard().focused_window_id().is_some(); /// ``` pub struct KeyboardUtil { - _sdldrop: ::std::rc::Rc, + _sdldrop: crate::SdlDrop, } impl KeyboardUtil { diff --git a/src/sdl2/mouse/mod.rs b/src/sdl2/mouse/mod.rs index 36f363a35f0..8abd872672a 100644 --- a/src/sdl2/mouse/mod.rs +++ b/src/sdl2/mouse/mod.rs @@ -374,7 +374,7 @@ impl crate::Sdl { /// sdl_context.mouse().show_cursor(false); /// ``` pub struct MouseUtil { - _sdldrop: ::std::rc::Rc, + _sdldrop: crate::SdlDrop, } impl MouseUtil { diff --git a/src/sdl2/sdl.rs b/src/sdl2/sdl.rs index 39a881914a9..d785e8d452e 100644 --- a/src/sdl2/sdl.rs +++ b/src/sdl2/sdl.rs @@ -1,9 +1,11 @@ use libc::c_char; +use std::cell::Cell; use std::error; use std::ffi::{CStr, CString, NulError}; use std::fmt; use std::mem::transmute; -use std::rc::Rc; +use std::os::raw::c_void; +use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use crate::sys; @@ -45,10 +47,17 @@ impl error::Error for Error { } } -use std::sync::atomic::AtomicBool; -/// Only one Sdl context can be alive at a time. -/// Set to false by default (not alive). -static IS_SDL_CONTEXT_ALIVE: AtomicBool = AtomicBool::new(false); +/// True if the main thread has been declared. The main thread is declared when +/// SDL is first initialized. +static IS_MAIN_THREAD_DECLARED: AtomicBool = AtomicBool::new(false); + +/// Number of active `SdlDrop` objects keeping SDL alive. +static SDL_COUNT: AtomicU32 = AtomicU32::new(0); + +thread_local! { + /// True if the current thread is the main thread. + static IS_MAIN_THREAD: Cell = Cell::new(false); +} /// The SDL context type. Initialize with `sdl2::init()`. /// @@ -64,31 +73,42 @@ static IS_SDL_CONTEXT_ALIVE: AtomicBool = AtomicBool::new(false); /// the main thread. #[derive(Clone)] pub struct Sdl { - sdldrop: Rc, + sdldrop: SdlDrop, } impl Sdl { #[inline] #[doc(alias = "SDL_Init")] fn new() -> Result { - unsafe { - use std::sync::atomic::Ordering; - - // Atomically switch the `IS_SDL_CONTEXT_ALIVE` global to true - let was_alive = IS_SDL_CONTEXT_ALIVE.swap(true, Ordering::Relaxed); - - if was_alive { - Err("Cannot initialize `Sdl` more than once at a time.".to_owned()) - } else if sys::SDL_Init(0) == 0 { - // Initialize SDL without any explicit subsystems (flags = 0). - Ok(Sdl { - sdldrop: Rc::new(SdlDrop), - }) + // Check if we can safely initialize SDL on this thread. + let was_main_thread_declared = IS_MAIN_THREAD_DECLARED.swap(true, Ordering::SeqCst); + + IS_MAIN_THREAD.with(|is_main_thread| { + if was_main_thread_declared { + if !is_main_thread.get() { + return Err("Cannot initialize `Sdl` from more than once thread.".to_owned()); + } } else { - IS_SDL_CONTEXT_ALIVE.swap(false, Ordering::Relaxed); - Err(get_error()) + is_main_thread.set(true); + } + Ok(()) + })?; + + // Initialize SDL. + if SDL_COUNT.fetch_add(1, Ordering::Relaxed) == 0 { + let result; + + unsafe { + result = sys::SDL_Init(0); + } + + if result != 0 { + SDL_COUNT.store(0, Ordering::Relaxed); + return Err(get_error()); } } + + Ok(Sdl { sdldrop: SdlDrop { _anticonstructor: std::ptr::null_mut() }}) } /// Initializes the audio subsystem. @@ -151,27 +171,38 @@ impl Sdl { #[inline] #[doc(hidden)] - pub fn sdldrop(&self) -> Rc { + pub fn sdldrop(&self) -> SdlDrop { self.sdldrop.clone() } } -/// When SDL is no longer in use (the refcount in an `Rc` reaches 0), the library is quit. +/// When SDL is no longer in use, the library is quit. #[doc(hidden)] #[derive(Debug)] -pub struct SdlDrop; +pub struct SdlDrop { + // Make it impossible to construct `SdlDrop` without access to this member, + // and opt out of Send and Sync. + _anticonstructor: *mut c_void, +} + +impl Clone for SdlDrop { + fn clone(&self) -> SdlDrop { + let prev_count = SDL_COUNT.fetch_add(1, Ordering::Relaxed); + assert!(prev_count > 0); + SdlDrop { _anticonstructor: std::ptr::null_mut() } + } +} impl Drop for SdlDrop { #[inline] #[doc(alias = "SDL_Quit")] fn drop(&mut self) { - use std::sync::atomic::Ordering; - - let was_alive = IS_SDL_CONTEXT_ALIVE.swap(false, Ordering::Relaxed); - assert!(was_alive); - - unsafe { - sys::SDL_Quit(); + let prev_count = SDL_COUNT.fetch_sub(1, Ordering::Relaxed); + assert!(prev_count > 0); + if prev_count == 1 { + unsafe { + sys::SDL_Quit(); + } } } } @@ -182,64 +213,42 @@ impl Drop for SdlDrop { // the event queue. These subsystems implement `Sync`. macro_rules! subsystem { - ($name:ident, $flag:expr) => { - impl $name { - #[inline] - #[doc(alias = "SDL_InitSubSystem")] - fn new(sdl: &Sdl) -> Result<$name, String> { - let result = unsafe { sys::SDL_InitSubSystem($flag) }; - - if result == 0 { - Ok($name { - _subsystem_drop: Rc::new(SubsystemDrop { - _sdldrop: sdl.sdldrop.clone(), - flag: $flag, - }), - }) - } else { - Err(get_error()) - } - } - } - }; - ($name:ident, $flag:expr, nosync) => { + ($name:ident, $flag:expr, $counter:ident, nosync) => { + static $counter: AtomicU32 = AtomicU32::new(0); + #[derive(Debug, Clone)] pub struct $name { /// Subsystems cannot be moved or (usually) used on non-main threads. /// Luckily, Rc restricts use to the main thread. - _subsystem_drop: Rc, + _subsystem_drop: SubsystemDrop, } impl $name { - /// Obtain an SDL context. #[inline] - pub fn sdl(&self) -> Sdl { - Sdl { - sdldrop: self._subsystem_drop._sdldrop.clone(), - } - } - } + #[doc(alias = "SDL_InitSubSystem")] + fn new(sdl: &Sdl) -> Result<$name, String> { + if $counter.fetch_add(1, Ordering::Relaxed) == 0 { + let result; - subsystem!($name, $flag); - }; - ($name:ident, $flag:expr, sync) => { - pub struct $name { - /// Subsystems cannot be moved or (usually) used on non-main threads. - /// Luckily, Rc restricts use to the main thread. - _subsystem_drop: Rc, - } - unsafe impl Sync for $name {} + unsafe { + result = sys::SDL_InitSubSystem($flag); + } - impl std::clone::Clone for $name { - #[inline] - fn clone(&self) -> $name { - $name { - _subsystem_drop: self._subsystem_drop.clone(), + if result != 0 { + $counter.store(0, Ordering::Relaxed); + return Err(get_error()); + } } + + Ok($name { + _subsystem_drop: SubsystemDrop { + _sdldrop: sdl.sdldrop.clone(), + counter: &$counter, + flag: $flag, + }, + }) } - } - impl $name { /// Obtain an SDL context. #[inline] pub fn sdl(&self) -> Sdl { @@ -248,49 +257,69 @@ macro_rules! subsystem { } } } - - subsystem!($name, $flag); + }; + ($name:ident, $flag:expr, $counter:ident, sync) => { + subsystem!($name, $flag, $counter, nosync); + unsafe impl Sync for $name {} }; } /// When a subsystem is no longer in use (the refcount in an `Rc` reaches 0), /// the subsystem is quit. -#[derive(Debug, Clone)] +#[derive(Debug)] struct SubsystemDrop { - _sdldrop: Rc, + _sdldrop: SdlDrop, + counter: &'static AtomicU32, flag: u32, } +impl Clone for SubsystemDrop { + fn clone(&self) -> SubsystemDrop { + let prev_count = self.counter.fetch_add(1, Ordering::Relaxed); + assert!(prev_count > 0); + SubsystemDrop { + _sdldrop: self._sdldrop.clone(), + counter: self.counter, + flag: self.flag, + } + } +} + impl Drop for SubsystemDrop { #[inline] #[doc(alias = "SDL_QuitSubSystem")] fn drop(&mut self) { - unsafe { - sys::SDL_QuitSubSystem(self.flag); + let prev_count = self.counter.fetch_sub(1, Ordering::Relaxed); + assert!(prev_count > 0); + if prev_count == 1 { + unsafe { + sys::SDL_QuitSubSystem(self.flag); + } } } } -subsystem!(AudioSubsystem, sys::SDL_INIT_AUDIO, nosync); +subsystem!(AudioSubsystem, sys::SDL_INIT_AUDIO, AUDIO_COUNT, nosync); subsystem!( GameControllerSubsystem, sys::SDL_INIT_GAMECONTROLLER, + GAMECONTROLLER_COUNT, nosync ); -subsystem!(HapticSubsystem, sys::SDL_INIT_HAPTIC, nosync); -subsystem!(JoystickSubsystem, sys::SDL_INIT_JOYSTICK, nosync); -subsystem!(VideoSubsystem, sys::SDL_INIT_VIDEO, nosync); +subsystem!(HapticSubsystem, sys::SDL_INIT_HAPTIC, HAPTIC_COUNT, nosync); +subsystem!(JoystickSubsystem, sys::SDL_INIT_JOYSTICK, JOYSTICK_COUNT, nosync); +subsystem!(VideoSubsystem, sys::SDL_INIT_VIDEO, VIDEO_COUNT, nosync); // Timers can be added on other threads. -subsystem!(TimerSubsystem, sys::SDL_INIT_TIMER, sync); +subsystem!(TimerSubsystem, sys::SDL_INIT_TIMER, TIMER_COUNT, sync); // The event queue can be read from other threads. -subsystem!(EventSubsystem, sys::SDL_INIT_EVENTS, sync); -subsystem!(SensorSubsystem, sys::SDL_INIT_SENSOR, sync); +subsystem!(EventSubsystem, sys::SDL_INIT_EVENTS, EVENTS_COUNT, sync); +subsystem!(SensorSubsystem, sys::SDL_INIT_SENSOR, SENSOR_COUNT, sync); -static mut IS_EVENT_PUMP_ALIVE: bool = false; +static IS_EVENT_PUMP_ALIVE: AtomicBool = AtomicBool::new(false); /// A thread-safe type that encapsulates SDL event-pumping functions. pub struct EventPump { - _sdldrop: Rc, + _event_subsystem: EventSubsystem, } impl EventPump { @@ -299,24 +328,12 @@ impl EventPump { #[doc(alias = "SDL_InitSubSystem")] fn new(sdl: &Sdl) -> Result { // Called on the main SDL thread. - - unsafe { - if IS_EVENT_PUMP_ALIVE { - Err("an `EventPump` instance is already alive - there can only be one `EventPump` in use at a time.".to_owned()) - } else { - // Initialize the events subsystem, just in case none of the other subsystems have done it yet. - let result = sys::SDL_InitSubSystem(sys::SDL_INIT_EVENTS); - - if result == 0 { - IS_EVENT_PUMP_ALIVE = true; - - Ok(EventPump { - _sdldrop: sdl.sdldrop.clone(), - }) - } else { - Err(get_error()) - } - } + if IS_EVENT_PUMP_ALIVE.load(Ordering::Relaxed) { + Err("an `EventPump` instance is already alive - there can only be one `EventPump` in use at a time.".to_owned()) + } else { + let _event_subsystem = sdl.event()?; + IS_EVENT_PUMP_ALIVE.store(true, Ordering::Relaxed); + Ok(EventPump { _event_subsystem }) } } } @@ -326,12 +343,8 @@ impl Drop for EventPump { #[doc(alias = "SDL_QuitSubSystem")] fn drop(&mut self) { // Called on the main SDL thread. - - unsafe { - assert!(IS_EVENT_PUMP_ALIVE); - sys::SDL_QuitSubSystem(sys::SDL_INIT_EVENTS); - IS_EVENT_PUMP_ALIVE = false; - } + assert!(IS_EVENT_PUMP_ALIVE.load(Ordering::Relaxed)); + IS_EVENT_PUMP_ALIVE.store(false, Ordering::Relaxed); } } From 77e044290e13e40d13f589778af991ca6da75046 Mon Sep 17 00:00:00 2001 From: Martin Mills Date: Tue, 6 Sep 2022 22:00:38 -0700 Subject: [PATCH 2/2] Add #1254 to changelog --- changelog.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index bc4c952b235..d9a634cbc88 100644 --- a/changelog.md +++ b/changelog.md @@ -8,6 +8,8 @@ when upgrading from a version of rust-sdl2 to another. [PR #1240](https://github.com/Rust-SDL2/rust-sdl2/pull/1240) **BREAKING CHANGE** Take `PixelMasks` by refrence +[PR #1254](https://github.com/Rust-SDL2/rust-sdl2/pull/1254) **BREAKING CHANGE** Make `SdlDrop` and `SubsystemDrop` safer; forbid external code from constructing `SdlDrop` + ### v0.35.2 [PR #1173](https://github.com/Rust-SDL2/rust-sdl2/pull/1173) Fix segfault when using timer callbacks @@ -60,7 +62,7 @@ Various fixes to CI. [PR #1033](https://github.com/Rust-SDL2/rust-sdl2/pull/1033) Changed signature of TimerSubsystem::ticks to accept `&self`. [PR #1057](https://github.com/Rust-SDL2/rust-sdl2/pull/1057): fix memory safety bug in set_error - + [PR #1081](https://github.com/Rust-SDL2/rust-sdl2/pull/1081): Allow bundled build to be built in debug mode. Fixes issue when linking binary with mixed debug+release CRT dependencies. [PR #1080](https://github.com/Rust-SDL2/rust-sdl2/pull/1080): Fix line endings of patches to lf so patching of sources works on Windows.