Skip to content

Audio safety fixes #789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 9, 2018
Merged

Audio safety fixes #789

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ca6469
Fix a segfault when `get_callback` panics
Thinkofname Aug 19, 2018
a0734fd
Fix undefined behavior with AudioStatus's FromPrimitive
Thinkofname Aug 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 36 additions & 42 deletions src/sdl2/audio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -209,12 +208,16 @@ pub enum AudioStatus {
impl FromPrimitive for AudioStatus {
fn from_i64(n: i64) -> Option<AudioStatus> {
use self::AudioStatus::*;
let n = n as u32;

Some( match unsafe { transmute::<u32, sys::SDL_AudioStatus>(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,
})
}

Expand Down Expand Up @@ -373,13 +376,15 @@ extern "C" fn audio_callback_marshall<CB: AudioCallback>
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<CB> = &mut *(userdata as *mut _);
let buf: &mut [CB::Channel] = from_raw_parts_mut(
stream as *mut CB::Channel,
len as usize / size_of::<CB::Channel>()
);

cb_userdata.callback(buf);
if let Some(cb) = cb_userdata {
cb.callback(buf);
}
}
}

Expand All @@ -394,14 +399,13 @@ pub struct AudioSpecDesired {
}

impl AudioSpecDesired {
fn convert_to_ll<CB, F, C, S>(freq: F, channels: C, samples: S, userdata: *mut CB) -> sys::SDL_AudioSpec
fn convert_to_ll<CB, F, C, S>(freq: F, channels: C, samples: S, userdata: *mut Option<CB>) -> sys::SDL_AudioSpec
where
CB: AudioCallback,
F: Into<Option<i32>>,
C: Into<Option<u8>>,
S: Into<Option<u16>>,
{
use std::mem::transmute;

let freq = freq.into();
let channels = channels.into();
Expand All @@ -413,22 +417,20 @@ impl AudioSpecDesired {

// A value of 0 means "fallback" or "default".

unsafe {
sys::SDL_AudioSpec {
freq: freq.unwrap_or(0),
format: <CB::Channel as AudioFormatNum>::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::<CB>
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: <CB::Channel as AudioFormatNum>::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::<CB>
as extern "C" fn
(arg1: *mut c_void,
arg2: *mut uint8_t,
arg3: c_int)),
userdata: userdata as *mut _,
}
}

Expand Down Expand Up @@ -596,7 +598,7 @@ pub struct AudioDevice<CB: AudioCallback> {
device_id: AudioDeviceID,
spec: AudioSpec,
/// Store the callback to keep it alive for the entire duration of `AudioDevice`.
userdata: Box<CB>
userdata: Box<Option<CB>>
}

impl<CB: AudioCallback> AudioDevice<CB> {
Expand All @@ -607,14 +609,8 @@ impl<CB: AudioCallback> AudioDevice<CB> {
D: Into<Option<&'a str>>,
{

// 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<CB> = Box::new(mem::uninitialized());
mem::transmute(b)
};
let desired = AudioSpecDesired::convert_to_ll(spec.freq, spec.channels, spec.samples, userdata);
let mut userdata: Box<Option<CB>> = Box::new(None);
let desired = AudioSpecDesired::convert_to_ll(spec.freq, spec.channels, spec.samples, &mut *userdata);

let mut obtained = unsafe { mem::uninitialized::<sys::SDL_AudioSpec>() };
unsafe {
Expand All @@ -636,10 +632,8 @@ impl<CB: AudioCallback> AudioDevice<CB> {
id => {
let device_id = AudioDeviceID::PlaybackDevice(id);
let spec = AudioSpec::convert_from_ll(obtained);
let mut userdata: Box<CB> = 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(),
Expand Down Expand Up @@ -713,7 +707,7 @@ impl<CB: AudioCallback> AudioDevice<CB> {
/// 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")
}
}

Expand All @@ -725,11 +719,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> {
Expand Down Expand Up @@ -835,7 +829,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);
}
}