Fix ub and shrink unsafe blocks (#1435)

* fix transmuting to invalid value

* fix public access to raw pointer

* add `repr(transparent)` to safely pointer-cast

between `FRect` and `sys::SDL_FRect`, and `FPoint` and`sys::SDL_FPoint`.

* add null pointer check before calling `CStr::from_ptr`

also shrink unsafe blocks

* return pointer with mutable provenance from `FRect::raw_mut`

before this commit writing to the pointer returned by `FRect::raw_mut`
was undefined behavior.

* fix use after free and unwind through FFI boundary in timer.rs

* update changelog

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 #1461](https://github.com/Rust-SDL2/rust-sdl2/pull/1461) **BREAKING CHANGE** Added new SensorType variants: LeftGyroscope, RightGyroscope, LeftAccelerometer, RightAccelerometer
 
 [PR #1461](https://github.com/Rust-SDL2/rust-sdl2/pull/1461) **BREAKING CHANGE** Sensor::get_data now returns correct SensorData::Gyro enum

src/sdl2/audio.rs

@@ -337,10 +337,15 @@ impl TryFrom<u32> 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(()),
         })
     }
 }

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<String, String> {
-    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<String, PrefPathError> {
     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)
+        }
     }
 }

src/sdl2/mixer/mod.rs

@@ -278,8 +278,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 {

src/sdl2/rect.rs

@@ -984,6 +984,9 @@ impl std::iter::Sum for Point {
 /// recommended to use `Option<FRect>`, 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,
@@ -1357,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")]
@@ -1600,6 +1603,9 @@ impl BitOr<FRect> 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,

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<dyn FnMut() -> u32 + 'a + Send>;
+pub type TimerCallback = Box<dyn FnMut() -> u32 + 'static + Send>;
 
-pub struct Timer<'b, 'a> {
-    callback: Option<Box<TimerCallback<'a>>>,
+pub struct Timer<'a> {
+    callback: Option<Box<TimerCallback>>,
     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::<TimerCallback>();
+        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