From 0ca64698e97776ef1754da97b4856bc6ef9190ad Mon Sep 17 00:00:00 2001 From: Matthew Collins Date: Sun, 19 Aug 2018 23:39:26 +0100 Subject: [PATCH 1/2] Fix a segfault when `get_callback` panics --- src/sdl2/audio.rs | 63 ++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index c4bc2612732..ca020dee1d4 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -373,13 +373,15 @@ extern "C" fn audio_callback_marshall use std::slice::from_raw_parts_mut; use std::mem::size_of; unsafe { - let cb_userdata: &mut CB = &mut *(userdata as *mut CB); + let cb_userdata: &mut Option = &mut *(userdata as *mut _); let buf: &mut [CB::Channel] = from_raw_parts_mut( stream as *mut CB::Channel, len as usize / size_of::() ); - cb_userdata.callback(buf); + if let Some(cb) = cb_userdata { + cb.callback(buf); + } } } @@ -394,14 +396,13 @@ pub struct AudioSpecDesired { } impl AudioSpecDesired { - fn convert_to_ll(freq: F, channels: C, samples: S, userdata: *mut CB) -> sys::SDL_AudioSpec + fn convert_to_ll(freq: F, channels: C, samples: S, userdata: *mut Option) -> sys::SDL_AudioSpec where CB: AudioCallback, F: Into>, C: Into>, S: Into>, { - use std::mem::transmute; let freq = freq.into(); let channels = channels.into(); @@ -413,22 +414,20 @@ impl AudioSpecDesired { // A value of 0 means "fallback" or "default". - unsafe { - sys::SDL_AudioSpec { - freq: freq.unwrap_or(0), - format: ::audio_format().to_ll(), - channels: channels.unwrap_or(0), - silence: 0, - samples: samples.unwrap_or(0), - padding: 0, - size: 0, - callback: Some(audio_callback_marshall:: - as extern "C" fn - (arg1: *mut c_void, - arg2: *mut uint8_t, - arg3: c_int)), - userdata: transmute(userdata) - } + sys::SDL_AudioSpec { + freq: freq.unwrap_or(0), + format: ::audio_format().to_ll(), + channels: channels.unwrap_or(0), + silence: 0, + samples: samples.unwrap_or(0), + padding: 0, + size: 0, + callback: Some(audio_callback_marshall:: + as extern "C" fn + (arg1: *mut c_void, + arg2: *mut uint8_t, + arg3: c_int)), + userdata: userdata as *mut _, } } @@ -596,7 +595,7 @@ pub struct AudioDevice { device_id: AudioDeviceID, spec: AudioSpec, /// Store the callback to keep it alive for the entire duration of `AudioDevice`. - userdata: Box + userdata: Box> } impl AudioDevice { @@ -607,14 +606,8 @@ impl AudioDevice { D: Into>, { - // SDL_OpenAudioDevice needs a userdata pointer, but we can't initialize the - // callback without the obtained AudioSpec. - // Create an uninitialized box that will be initialized after SDL_OpenAudioDevice. - let userdata: *mut CB = unsafe { - let b: Box = Box::new(mem::uninitialized()); - mem::transmute(b) - }; - let desired = AudioSpecDesired::convert_to_ll(spec.freq, spec.channels, spec.samples, userdata); + let mut userdata: Box> = Box::new(None); + let desired = AudioSpecDesired::convert_to_ll(spec.freq, spec.channels, spec.samples, &mut *userdata); let mut obtained = unsafe { mem::uninitialized::() }; unsafe { @@ -636,10 +629,8 @@ impl AudioDevice { id => { let device_id = AudioDeviceID::PlaybackDevice(id); let spec = AudioSpec::convert_from_ll(obtained); - let mut userdata: Box = mem::transmute(userdata); - let garbage = mem::replace(&mut userdata as &mut CB, get_callback(spec)); - mem::forget(garbage); + *userdata = Some(get_callback(spec)); Ok(AudioDevice { subsystem: a.clone(), @@ -713,7 +704,7 @@ impl AudioDevice { /// but the callback data will be dropped. pub fn close_and_get_callback(self) -> CB { drop(self.device_id); - *self.userdata + self.userdata.expect("Missing callback") } } @@ -725,11 +716,11 @@ pub struct AudioDeviceLockGuard<'a, CB> where CB: AudioCallback, CB: 'a { impl<'a, CB: AudioCallback> Deref for AudioDeviceLockGuard<'a, CB> { type Target = CB; - fn deref(&self) -> &CB { &self.device.userdata } + fn deref(&self) -> &CB { (*self.device.userdata).as_ref().expect("Missing callback") } } impl<'a, CB: AudioCallback> DerefMut for AudioDeviceLockGuard<'a, CB> { - fn deref_mut(&mut self) -> &mut CB { &mut self.device.userdata } + fn deref_mut(&mut self) -> &mut CB { (*self.device.userdata).as_mut().expect("Missing callback") } } impl<'a, CB: AudioCallback> Drop for AudioDeviceLockGuard<'a, CB> { @@ -835,7 +826,7 @@ mod test { assert_eq!(new_buffer.len(), new_buffer_expected.len(), "capacity must be exactly equal to twice the original vec size"); // // this has been commented, see https://discourse.libsdl.org/t/change-of-behavior-in-audiocvt-sdl-convertaudio-from-2-0-5-to-2-0-6/24682 - // // to maybe re-enable it someday + // // to maybe re-enable it someday // assert_eq!(new_buffer, new_buffer_expected); } } From a0734fd3d8ed70bdfafb9d29338d2c23d983dd99 Mon Sep 17 00:00:00 2001 From: Matthew Collins Date: Sun, 19 Aug 2018 23:48:58 +0100 Subject: [PATCH 2/2] Fix undefined behavior with AudioStatus's FromPrimitive Transmuting from a value not in the enum is undefined, plus transmuting is generally not great to do. --- src/sdl2/audio.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index ca020dee1d4..6c87f0b09a6 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -60,7 +60,6 @@ use std::ops::{Deref, DerefMut}; use std::path::Path; use std::marker::PhantomData; use std::mem; -use std::mem::transmute; use std::ptr; use AudioSubsystem; @@ -209,12 +208,16 @@ pub enum AudioStatus { impl FromPrimitive for AudioStatus { fn from_i64(n: i64) -> Option { use self::AudioStatus::*; - let n = n as u32; - Some( match unsafe { transmute::(n) } { - sys::SDL_AudioStatus::SDL_AUDIO_STOPPED => Stopped, - sys::SDL_AudioStatus::SDL_AUDIO_PLAYING => Playing, - sys::SDL_AudioStatus::SDL_AUDIO_PAUSED => Paused, + const STOPPED: i64 = sys::SDL_AudioStatus::SDL_AUDIO_STOPPED as i64; + const PLAYING: i64 = sys::SDL_AudioStatus::SDL_AUDIO_PLAYING as i64; + const PAUSED: i64 = sys::SDL_AudioStatus::SDL_AUDIO_PAUSED as i64; + + Some(match n { + STOPPED => Stopped, + PLAYING => Playing, + PAUSED => Paused, + _ => return None, }) }