Skip to content
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.

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

[WIP] [CI] Fix unsoundess #49

Merged
merged 24 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Stop holding the audio callback in Rust side
Before, the code in Rust side were resposible of freeing the callback.
But now that oboe's API receives a shared_ptr, that does automatic
memory management, we no longer need to hold the callback in the Rust
side.
  • Loading branch information
Rodrigodd committed Jan 13, 2023
commit ce797bc1fe4ccf797442eda6a1f9b5e8f063bc7f
14 changes: 6 additions & 8 deletions src/audio_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ use std::{
};

use super::{
audio_stream_base_fmt, wrap_result, wrap_status, AudioApi, AudioCallbackWrapper,
AudioStreamBase, FrameTimestamp, Input, IsFrameType, Output, RawAudioInputStream,
RawAudioOutputStream, RawAudioStream, RawAudioStreamBase, Result, Status, StreamState,
NANOS_PER_MILLISECOND,
audio_stream_base_fmt, wrap_result, wrap_status, AudioApi, AudioStreamBase, FrameTimestamp,
Input, IsFrameType, Output, RawAudioInputStream, RawAudioOutputStream, RawAudioStream,
RawAudioStreamBase, Result, Status, StreamState, NANOS_PER_MILLISECOND,
};

/**
Expand Down Expand Up @@ -629,7 +628,7 @@ impl<'s> RawAudioOutputStream for AudioStreamRef<'s, Output> {}
*/
pub struct AudioStreamAsync<D, F> {
raw: AudioStreamHandle,
callback: AudioCallbackWrapper<D, F>,
_phantom: PhantomData<(D, F)>,
}

impl<D, F> fmt::Debug for AudioStreamAsync<D, F> {
Expand All @@ -639,15 +638,14 @@ impl<D, F> fmt::Debug for AudioStreamAsync<D, F> {
}

impl<D, F> AudioStreamAsync<D, F> {
// SAFETY: `raw`, `shared_ptr` and `callback` must be valid.
// SAFETY: `raw` and `shared_ptr` must be valid.
pub(crate) unsafe fn wrap_raw(
raw: *mut ffi::oboe_AudioStream,
shared_ptr: *mut c_void,
callback: AudioCallbackWrapper<D, F>,
) -> Self {
Self {
raw: AudioStreamHandle(raw, shared_ptr),
callback,
_phantom: PhantomData,
}
}
}
Expand Down
40 changes: 17 additions & 23 deletions src/audio_stream_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ use std::{
ops::{Deref, DerefMut},
};

use crate::{set_input_callback, set_output_callback};

use super::{
audio_stream_base_fmt, wrap_status, AudioApi, AudioCallbackWrapper, AudioInputCallback,
AudioOutputCallback, AudioStreamAsync, AudioStreamSync, ContentType, Input, InputPreset,
IsChannelCount, IsDirection, IsFormat, IsFrameType, Mono, Output, PerformanceMode,
RawAudioStreamBase, Result, SampleRateConversionQuality, SessionId, SharingMode, Stereo,
Unspecified, Usage,
audio_stream_base_fmt, wrap_status, AudioApi, AudioInputCallback, AudioOutputCallback,
AudioStreamAsync, AudioStreamSync, ContentType, Input, InputPreset, IsChannelCount,
IsDirection, IsFormat, IsFrameType, Mono, Output, PerformanceMode, RawAudioStreamBase, Result,
SampleRateConversionQuality, SessionId, SharingMode, Stereo, Unspecified, Usage,
};

#[repr(transparent)]
Expand Down Expand Up @@ -511,10 +512,9 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Input, C, T> {
(T, C): IsFrameType,
{
let mut raw = self.destructs();
let callback = AudioCallbackWrapper::<Input, F>::set_callback(&mut raw, stream_callback);
set_input_callback(&mut raw, stream_callback);
AudioStreamBuilderAsync {
raw: ManuallyDrop::new(raw),
callback: ManuallyDrop::new(callback),
_phantom: PhantomData,
}
}
Expand Down Expand Up @@ -545,10 +545,9 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Output, C, T> {
(T, C): IsFrameType,
{
let mut raw = self.destructs();
let callback = AudioCallbackWrapper::<Output, F>::set_callback(&mut raw, stream_callback);
set_output_callback(&mut raw, stream_callback);
AudioStreamBuilderAsync {
raw: ManuallyDrop::new(raw),
callback: ManuallyDrop::new(callback),
_phantom: PhantomData,
}
}
Expand All @@ -559,16 +558,13 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Output, C, T> {
*/
pub struct AudioStreamBuilderAsync<D, F> {
raw: ManuallyDrop<AudioStreamBuilderHandle>,
callback: ManuallyDrop<AudioCallbackWrapper<D, F>>,
_phantom: PhantomData<(D, F)>,
}

impl<D, F> Drop for AudioStreamBuilderAsync<D, F> {
fn drop(&mut self) {
// SAFETY: raw and callback are only droped here, or taken in Self::destructs, which don't
// drop self.
// SAFETY: self.raw is only droped here, or taken in Self::destructs, which don't drop self.
unsafe {
ManuallyDrop::drop(&mut self.callback);
ManuallyDrop::drop(&mut self.raw);
}
}
Expand All @@ -591,16 +587,14 @@ impl<D, F> RawAudioStreamBase for AudioStreamBuilderAsync<D, F> {
}

impl<D, F> AudioStreamBuilderAsync<D, F> {
/// Descontructs self into its handle and audio callback, without calling drop.
fn destructs(mut self) -> (AudioStreamBuilderHandle, AudioCallbackWrapper<D, F>) {
// Safety: the std::mem::forget prevents `raw` and `callback` from being dropped by
// Self::drop.
/// Descontructs self into its handle without calling drop.
fn destructs(mut self) -> AudioStreamBuilderHandle {
// Safety: the std::mem::forget prevents `raw` from being dropped by Self::drop.
let raw = unsafe { ManuallyDrop::take(&mut self.raw) };
let callback = unsafe { ManuallyDrop::take(&mut self.callback) };

std::mem::forget(self);

(raw, callback)
raw
}
}

Expand All @@ -611,7 +605,7 @@ impl<F: AudioInputCallback + Send> AudioStreamBuilderAsync<Input, F> {
pub fn open_stream(self) -> Result<AudioStreamAsync<Input, F>> {
let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit();
let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit();
let (mut raw, callback) = self.destructs();
let mut raw = self.destructs();

let stream = wrap_status(unsafe {
ffi::oboe_AudioStreamBuilder_openStreamShared(
Expand All @@ -621,7 +615,7 @@ impl<F: AudioInputCallback + Send> AudioStreamBuilderAsync<Input, F> {
)
})
.map(|_| unsafe {
AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback)
AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init())
});

drop(raw);
Expand All @@ -637,7 +631,7 @@ impl<F: AudioOutputCallback + Send> AudioStreamBuilderAsync<Output, F> {
pub fn open_stream(self) -> Result<AudioStreamAsync<Output, F>> {
let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit();
let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit();
let (mut raw, callback) = self.destructs();
let mut raw = self.destructs();

let stream = wrap_status(unsafe {
ffi::oboe_AudioStreamBuilder_openStreamShared(
Expand All @@ -647,7 +641,7 @@ impl<F: AudioOutputCallback + Send> AudioStreamBuilderAsync<Output, F> {
)
})
.map(|_| unsafe {
AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback)
AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init())
});

drop(raw);
Expand Down
104 changes: 36 additions & 68 deletions src/audio_stream_callback.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::{
ffi::c_void,
marker::PhantomData,
slice::{from_raw_parts, from_raw_parts_mut},
};

Expand All @@ -10,7 +9,7 @@ use num_traits::FromPrimitive;

use super::{
AudioInputStreamSafe, AudioOutputStreamSafe, AudioStreamBuilderHandle, AudioStreamRef,
DataCallbackResult, Error, Input, IsFrameType, Output,
DataCallbackResult, Error, IsFrameType,
};

/**
Expand Down Expand Up @@ -207,75 +206,44 @@ pub trait AudioOutputCallback {
) -> DataCallbackResult;
}

pub(crate) struct AudioCallbackWrapper<D, T> {
raw: *mut c_void,
_phantom: PhantomData<(D, T)>,
}

impl<D, T> Drop for AudioCallbackWrapper<D, T> {
fn drop(&mut self) {
// SAFETY: As long as `self` was created at AudioCallbackwrapper::set_callback, self.raw is
// a valid pointer, and this call is safe.
unsafe {
ffi::oboe_AudioStreamCallbackWrapper_delete(self.raw);
}
}
}

impl<T> AudioCallbackWrapper<Input, T>
where
T: AudioInputCallback,
{
pub(crate) fn set_callback(builder: &mut AudioStreamBuilderHandle, callback: T) -> Self {
let callback = Box::into_raw(Box::new(callback));

// SAFETY: `callback` has the same type as the first argument of each function, and each
// function follows the C ABI.
let raw = unsafe {
ffi::oboe_AudioStreamBuilder_setCallback(
&mut **builder as *mut ffi::oboe_AudioStreamBuilder,
callback.cast(),
Some(drop_context::<T>),
Some(on_audio_ready_input_wrapper::<T>),
Some(on_error_before_close_input_wrapper::<T>),
Some(on_error_after_close_input_wrapper::<T>),
)
};

let wrapper = Self {
raw,
_phantom: PhantomData,
};
wrapper
pub(crate) fn set_input_callback<T: AudioInputCallback>(
builder: &mut AudioStreamBuilderHandle,
callback: T,
) {
let callback = Box::into_raw(Box::new(callback));

// SAFETY: `callback` has the same type as the first argument of each function, and each
// function follows the C ABI.
unsafe {
ffi::oboe_AudioStreamBuilder_setCallback(
&mut **builder as *mut ffi::oboe_AudioStreamBuilder,
callback.cast(),
Some(drop_context::<T>),
Some(on_audio_ready_input_wrapper::<T>),
Some(on_error_before_close_input_wrapper::<T>),
Some(on_error_after_close_input_wrapper::<T>),
);
}
}

impl<T> AudioCallbackWrapper<Output, T>
where
T: AudioOutputCallback,
{
pub(crate) fn set_callback(builder: &mut AudioStreamBuilderHandle, callback: T) -> Self {
let callback = Box::new(callback);
let callback = Box::into_raw(callback);

// SAFETY: `callback` has the same type as the first argument of each function, and each
// function follows the C ABI.
let raw = unsafe {
ffi::oboe_AudioStreamBuilder_setCallback(
&mut **builder as *mut ffi::oboe_AudioStreamBuilder,
callback.cast(),
Some(drop_context::<T>),
Some(on_audio_ready_output_wrapper::<T>),
Some(on_error_before_close_output_wrapper::<T>),
Some(on_error_after_close_output_wrapper::<T>),
)
};

let wrapper = Self {
raw,
_phantom: PhantomData,
};
wrapper
pub(crate) fn set_output_callback<T: AudioOutputCallback>(
builder: &mut AudioStreamBuilderHandle,
callback: T,
) {
let callback = Box::new(callback);
let callback = Box::into_raw(callback);

// SAFETY: `callback` has the same type as the first argument of each function, and each
// function follows the C ABI.
unsafe {
ffi::oboe_AudioStreamBuilder_setCallback(
&mut **builder as *mut ffi::oboe_AudioStreamBuilder,
callback.cast(),
Some(drop_context::<T>),
Some(on_audio_ready_output_wrapper::<T>),
Some(on_error_before_close_output_wrapper::<T>),
Some(on_error_after_close_output_wrapper::<T>),
);
}
}

Expand Down
14 changes: 6 additions & 8 deletions sys/oboe-ext/include/oboe/OboeExt.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,14 @@ namespace oboe {
const ErrorCloseHandler _after_close;
};

void AudioStreamCallbackWrapper_delete(void *callback);

AudioStreamBuilder *AudioStreamBuilder_new();
void AudioStreamBuilder_delete(AudioStreamBuilder *builder);
void* AudioStreamBuilder_setCallback(AudioStreamBuilder *builder,
void *context,
const DropContextHandler drop_context,
const AudioReadyHandler audio_ready,
const ErrorCloseHandler before_close,
const ErrorCloseHandler after_close);
void AudioStreamBuilder_setCallback(AudioStreamBuilder *builder,
void *context,
const DropContextHandler drop_context,
const AudioReadyHandler audio_ready,
const ErrorCloseHandler before_close,
const ErrorCloseHandler after_close);

AudioApi AudioStreamBuilder_getAudioApi(const AudioStreamBuilder *builder);
void AudioStreamBuilder_setAudioApi(AudioStreamBuilder *builder, AudioApi api);
Expand Down
25 changes: 11 additions & 14 deletions sys/oboe-ext/src/AudioStreamBuilderWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,22 @@ namespace oboe {
builder->setAudioApi(api);
}

/// Returns a pointer to a shared_ptr<AudioStreamCallbackWrapper> that must be deleted with
/// AudioStreamCallbackWrapper_delete.
void* AudioStreamBuilder_setCallback(AudioStreamBuilder *builder,
/// Takes ownership of context (drop_context will be called to free it).
void AudioStreamBuilder_setCallback(AudioStreamBuilder *builder,
void *context,
const DropContextHandler drop_context,
const AudioReadyHandler audio_ready,
const ErrorCloseHandler before_close,
const ErrorCloseHandler after_close) {
auto *s = new std::shared_ptr<AudioStreamCallbackWrapper>(
new AudioStreamCallbackWrapper(
context,
drop_context,
audio_ready,
before_close,
after_close));
builder->setDataCallback(*s);
builder->setErrorCallback(*s);

return (void*) s;
auto s = std::make_shared<AudioStreamCallbackWrapper>(
context,
drop_context,
audio_ready,
before_close,
after_close);

builder->setDataCallback(s);
builder->setErrorCallback(s);
}

AudioStreamBase* AudioStreamBuilder_getBase(AudioStreamBuilder *builder) {
Expand Down
5 changes: 0 additions & 5 deletions sys/oboe-ext/src/AudioStreamCallbackWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,4 @@ namespace oboe {
Result error) {
_after_close(_context, oboeStream, error);
}

void AudioStreamCallbackWrapper_delete(void *callback) {
std::shared_ptr<AudioStreamCallbackWrapper> *s = (std::shared_ptr<AudioStreamCallbackWrapper> *)callback;
delete s;
}
}
6 changes: 1 addition & 5 deletions sys/src/bindings_aarch64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1649,10 +1649,6 @@ extern "C" {
error: oboe_Result,
);
}
extern "C" {
#[link_name = "\u{1}_ZN4oboe33AudioStreamCallbackWrapper_deleteEPv"]
pub fn oboe_AudioStreamCallbackWrapper_delete(callback: *mut ::std::os::raw::c_void);
}
extern "C" {
#[link_name = "\u{1}_ZN4oboe22AudioStreamBuilder_newEv"]
pub fn oboe_AudioStreamBuilder_new() -> *mut oboe_AudioStreamBuilder;
Expand All @@ -1670,7 +1666,7 @@ extern "C" {
audio_ready: oboe_AudioReadyHandler,
before_close: oboe_ErrorCloseHandler,
after_close: oboe_ErrorCloseHandler,
) -> *mut ::std::os::raw::c_void;
);
}
extern "C" {
#[link_name = "\u{1}_ZN4oboe30AudioStreamBuilder_getAudioApiEPKNS_18AudioStreamBuilderE"]
Expand Down