Fix ub and shrink unsafe blocks

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.

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

@@ -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 {

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