From 3894dbfbebeabd3ba0dbcc2a47e2120b43fe7706 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 22:56:06 +0200 Subject: [PATCH 1/7] fix transmuting to invalid value --- src/sdl2/audio.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 6cda64a530..748d70c030 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -337,10 +337,15 @@ impl TryFrom for AudioStatus { use self::AudioStatus::*; use crate::sys::SDL_AudioStatus::*; - Ok(match unsafe { mem::transmute(n) } { - SDL_AUDIO_STOPPED => Stopped, - SDL_AUDIO_PLAYING => Playing, - SDL_AUDIO_PAUSED => Paused, + const STOPPED: u32 = SDL_AUDIO_STOPPED as u32; + const PLAYING: u32 = SDL_AUDIO_PLAYING as u32; + const PAUSED: u32 = SDL_AUDIO_PAUSED as u32; + + Ok(match n { + STOPPED => Stopped, + PLAYING => Playing, + PAUSED => Paused, + _ => return Err(()), }) } } From 595f8a83868a63305d4db66d9e30a366a12e8157 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 22:56:56 +0200 Subject: [PATCH 2/7] fix public access to raw pointer --- src/sdl2/mixer/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sdl2/mixer/mod.rs b/src/sdl2/mixer/mod.rs index 8f87ad0426..761906a4c4 100644 --- a/src/sdl2/mixer/mod.rs +++ b/src/sdl2/mixer/mod.rs @@ -212,8 +212,8 @@ pub fn get_chunk_decoder(index: i32) -> String { /// The internal format for an audio chunk. #[derive(PartialEq)] pub struct Chunk { - pub raw: *mut mixer::Mix_Chunk, - pub owned: bool, + raw: *mut mixer::Mix_Chunk, + owned: bool, } impl Drop for Chunk { From df062023c7b3888503a5c0339634f8a3b1ac1fc6 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 22:57:13 +0200 Subject: [PATCH 3/7] add `repr(transparent)` to safely pointer-cast between `FRect` and `sys::SDL_FRect`, and `FPoint` and`sys::SDL_FPoint`. --- src/sdl2/rect.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sdl2/rect.rs b/src/sdl2/rect.rs index 6f1c0817aa..79f8957ed0 100644 --- a/src/sdl2/rect.rs +++ b/src/sdl2/rect.rs @@ -984,6 +984,9 @@ impl std::iter::Sum for Point { /// recommended to use `Option`, with `None` representing an empty /// rectangle (see, for example, the output of the /// [`intersection`](#method.intersection) method). +// Uses repr(transparent) to allow pointer casting between FRect and SDL_FRect (see +// `FRect::raw_slice`) +#[repr(transparent)] #[derive(Clone, Copy)] pub struct FRect { raw: sys::SDL_FRect, @@ -1600,6 +1603,9 @@ impl BitOr for FRect { } /// Immutable point type with float precision, consisting of x and y. +// Uses repr(transparent) to allow pointer casting between FPoint and SDL_FPoint (see +// `FPoint::raw_slice`) +#[repr(transparent)] #[derive(Copy, Clone)] pub struct FPoint { raw: sys::SDL_FPoint, From f642dd28fd5a794114e97774fc64c67489415fe4 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 22:58:57 +0200 Subject: [PATCH 4/7] add null pointer check before calling `CStr::from_ptr` also shrink unsafe blocks --- src/sdl2/filesystem.rs | 48 ++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/sdl2/filesystem.rs b/src/sdl2/filesystem.rs index 27d46c5898..5eac03dd3d 100644 --- a/src/sdl2/filesystem.rs +++ b/src/sdl2/filesystem.rs @@ -1,5 +1,4 @@ use crate::get_error; -use libc::c_char; use libc::c_void; use std::error; use std::ffi::{CStr, CString, NulError}; @@ -9,17 +8,15 @@ use crate::sys; #[doc(alias = "SDL_GetBasePath")] pub fn base_path() -> Result { - let result = unsafe { + unsafe { let buf = sys::SDL_GetBasePath(); - let s = CStr::from_ptr(buf as *const _).to_str().unwrap().to_owned(); - sys::SDL_free(buf as *mut c_void); - s - }; - - if result.is_empty() { - Err(get_error()) - } else { - Ok(result) + if buf.is_null() { + Err(get_error()) + } else { + let s = CStr::from_ptr(buf).to_str().unwrap().to_owned(); + sys::SDL_free(buf as *mut c_void); + Ok(s) + } } } @@ -58,23 +55,18 @@ impl error::Error for PrefPathError { #[doc(alias = "SDL_GetPrefPath")] pub fn pref_path(org_name: &str, app_name: &str) -> Result { use self::PrefPathError::*; - let result = unsafe { - let org = match CString::new(org_name) { - Ok(s) => s, - Err(err) => return Err(InvalidOrganizationName(err)), - }; - let app = match CString::new(app_name) { - Ok(s) => s, - Err(err) => return Err(InvalidApplicationName(err)), - }; - let buf = - sys::SDL_GetPrefPath(org.as_ptr() as *const c_char, app.as_ptr() as *const c_char); - CStr::from_ptr(buf as *const _).to_str().unwrap().to_owned() - }; - if result.is_empty() { - Err(SdlError(get_error())) - } else { - Ok(result) + let org = CString::new(org_name).map_err(InvalidOrganizationName)?; + let app = CString::new(app_name).map_err(InvalidApplicationName)?; + + unsafe { + let buf = sys::SDL_GetPrefPath(org.as_ptr(), app.as_ptr()); + if buf.is_null() { + Err(SdlError(get_error())) + } else { + let ret = CStr::from_ptr(buf).to_str().unwrap().to_owned(); + sys::SDL_free(buf as *mut c_void); + Ok(ret) + } } } From 103d5bdecbef9da97b96a2acaa255fe6f9f653c6 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 23:00:15 +0200 Subject: [PATCH 5/7] return pointer with mutable provenance from `FRect::raw_mut` before this commit writing to the pointer returned by `FRect::raw_mut` was undefined behavior. --- src/sdl2/rect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/rect.rs b/src/sdl2/rect.rs index 79f8957ed0..4e0e4c0929 100644 --- a/src/sdl2/rect.rs +++ b/src/sdl2/rect.rs @@ -1360,7 +1360,7 @@ impl FRect { } pub fn raw_mut(&mut self) -> *mut sys::SDL_FRect { - self.raw() as *mut _ + &mut self.raw } #[doc(alias = "SDL_FRect")] From 10b888edb26b412d50fa0e29fbad864f4ca4202a Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 18 Sep 2024 22:45:09 +0200 Subject: [PATCH 6/7] fix use after free and unwind through FFI boundary in timer.rs --- src/sdl2/timer.rs | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/sdl2/timer.rs b/src/sdl2/timer.rs index 2281dba6e0..f911d9d6ce 100644 --- a/src/sdl2/timer.rs +++ b/src/sdl2/timer.rs @@ -1,7 +1,8 @@ use crate::sys; use libc::c_void; use std::marker::PhantomData; -use std::mem; +use std::panic::catch_unwind; +use std::process::abort; use crate::TimerSubsystem; @@ -14,16 +15,16 @@ impl TimerSubsystem { /// * or when the callback returns a non-positive continuation interval /// /// The callback is run in a thread that is created and managed internally - /// by SDL2 from C. The callback *must* not panic! + /// by SDL2 from C. If the callback panics, the process will be [aborted][abort]. #[must_use = "if unused the Timer will be dropped immediately"] #[doc(alias = "SDL_AddTimer")] - pub fn add_timer<'b, 'c>(&'b self, delay: u32, callback: TimerCallback<'c>) -> Timer<'b, 'c> { + pub fn add_timer(&self, delay: u32, callback: TimerCallback) -> Timer<'_> { unsafe { - let callback = Box::new(callback); + let mut callback = Box::new(callback); let timer_id = sys::SDL_AddTimer( delay, Some(c_timer_callback), - mem::transmute_copy(&callback), + &mut *callback as *mut TimerCallback as *mut c_void, ); Timer { @@ -90,23 +91,23 @@ impl TimerSubsystem { } } -pub type TimerCallback<'a> = Box u32 + 'a + Send>; +pub type TimerCallback = Box u32 + 'static + Send>; -pub struct Timer<'b, 'a> { - callback: Option>>, +pub struct Timer<'a> { + callback: Option>, raw: sys::SDL_TimerID, - _marker: PhantomData<&'b ()>, + _marker: PhantomData<&'a ()>, } -impl<'b, 'a> Timer<'b, 'a> { +impl<'a> Timer<'a> { /// Returns the closure as a trait-object and cancels the timer /// by consuming it... - pub fn into_inner(mut self) -> TimerCallback<'a> { + pub fn into_inner(mut self) -> TimerCallback { *self.callback.take().unwrap() } } -impl<'b, 'a> Drop for Timer<'b, 'a> { +impl<'a> Drop for Timer<'a> { #[inline] #[doc(alias = "SDL_RemoveTimer")] fn drop(&mut self) { @@ -117,16 +118,14 @@ impl<'b, 'a> Drop for Timer<'b, 'a> { } } -extern "C" fn c_timer_callback(_interval: u32, param: *mut c_void) -> u32 { - // FIXME: This is UB if the callback panics! (But will realistically - // crash on stack underflow.) - // - // I tried using `std::panic::catch_unwind()` here and it compiled but - // would not catch. Maybe wait for `c_unwind` to stabilize? Then the behavior - // will automatically abort the process when panicking over an `extern "C"` - // function. - let f = param as *mut TimerCallback<'_>; - unsafe { (*f)() } +unsafe extern "C" fn c_timer_callback(_interval: u32, param: *mut c_void) -> u32 { + match catch_unwind(|| { + let f = param.cast::(); + unsafe { (*f)() } + }) { + Ok(ret) => ret, + Err(_) => abort(), + } } #[cfg(not(target_os = "macos"))] @@ -151,7 +150,7 @@ mod test { let _timer = timer_subsystem.add_timer( 20, - Box::new(|| { + Box::new(move || { // increment up to 10 times (0 -> 9) // tick again in 100ms after each increment // @@ -180,7 +179,7 @@ mod test { let _timer = timer_subsystem.add_timer( 20, - Box::new(|| { + Box::new(move || { let mut flag = timer_flag.lock().unwrap(); *flag = true; 0 From 42035237e0e8ab0440ef4a03180d908bd1da2385 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Fri, 20 Sep 2024 23:55:32 +0200 Subject: [PATCH 7/7] update changelog --- changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/changelog.md b/changelog.md index 12808ac830..f45d2aef4c 100644 --- a/changelog.md +++ b/changelog.md @@ -3,6 +3,8 @@ when upgrading from a version of rust-sdl2 to another. ### Next +[PR #1435](https://github.com/Rust-SDL2/rust-sdl2/pull/1435) **BREAKING CHANGE** Fix some undefined behavior. Breaking changes: `mixer::Chunk`'s fields are now private and callbacks to `TimerSubsystem::add_timer` must now live for `'static`, also allowing some lifetime parameters to be removed. + [PR #1416](https://github.com/Rust-SDL2/rust-sdl2/pull/1416) Apply clippy fixes, fix deprecations and other code quality improvements. [PR #1408](https://github.com/Rust-SDL2/rust-sdl2/pull/1408) Allow comparing `Version`s, add constant with the version the bindings were compiled with.