Fix some unsoundess and crash issues

This mainly fix issue #41, that causes crashes when a `AudioStream` was
drop. That happen because the `AudioStream` was not closed on Drop, but
was deleted, causing a use-after-free by the not closed `onDataCallback`
thread.

Also, the method `AudioStreamBuilder::open_stream` was using the
deprecated method `openStream(AudioStream*)`, that do not allow deleting
the `AudioStream` safely. Replaced it by
`openStream(shared_ptr<AudioStream>)`.  The deprecated function allowed
a use-after-free by the `onErrorCallback` thread.

Also, as noted by issue #45, the bindings for `AudioStream::close()` was
wrongly bound to the concrete implementation of the base class, instead
of calling the virtual method.

Also note that currently there is no safe way to delete the
`onErrorCallback` of a `AudioStream` in oboe (see
google/oboe#1610), so instead the current
implementation leaks the callback on drop.

Also, remove some unsound `Drop` implementations and replace them by
explicit unsafe delete methods.

src/audio_stream.rs

@@ -411,9 +411,7 @@ impl<T: RawAudioStream + RawAudioStreamBase> AudioStream for T {
     }
 
     fn close(&mut self) -> Status {
-        wrap_status(unsafe {
-            ffi::oboe_AudioStream_close(self._raw_stream_mut() as *mut _ as *mut c_void)
-        })
+        wrap_status(unsafe { ffi::oboe_AudioStream_close1(self._raw_stream_mut()) })
     }
 
     fn start_with_timeout(&mut self, timeout_nanoseconds: i64) -> Status {
@@ -537,24 +535,30 @@ pub(crate) fn audio_stream_fmt<T: AudioStreamSafe>(
     '\n'.fmt(f)
 }
 
-#[repr(transparent)]
-struct AudioStreamHandle(*mut ffi::oboe_AudioStream);
+struct AudioStreamHandle(*mut ffi::oboe_AudioStream, *mut c_void);
 
-impl From<*mut ffi::oboe_AudioStream> for AudioStreamHandle {
-    fn from(raw: *mut ffi::oboe_AudioStream) -> Self {
-        Self(raw)
+impl AudioStreamHandle {
+    fn new(raw: *mut ffi::oboe_AudioStream, shared_ptr: *mut c_void) -> Self {
+        Self(raw, shared_ptr)
     }
-}
 
-impl Default for AudioStreamHandle {
-    fn default() -> Self {
-        Self(null_mut())
+    /// SAFETY: `self.0` and `self.1` must be valid pointers.
+    pub(crate) unsafe fn delete(&mut self) {
+        assert!(!self.0.is_null());
+        assert!(!self.1.is_null());
+
+        // The error callback could be holding a shared_ptr, so don't delete AudioStream
+        // directly, but only its shared_ptr.
+        ffi::oboe_AudioStream_deleteShared(self.1);
+
+        self.0 = null_mut();
+        self.1 = null_mut();
     }
 }
 
-impl Drop for AudioStreamHandle {
-    fn drop(&mut self) {
-        unsafe { ffi::oboe_AudioStream_delete(self.0) }
+impl Default for AudioStreamHandle {
+    fn default() -> Self {
+        Self(null_mut(), null_mut())
     }
 }
 
@@ -625,9 +629,6 @@ impl<'s> RawAudioOutputStream for AudioStreamRef<'s, Output> {}
  */
 pub struct AudioStreamAsync<D, F> {
     raw: AudioStreamHandle,
-
-    // Needed to keep callback alive
-    #[allow(dead_code)]
     callback: AudioCallbackWrapper<D, F>,
 }
 
@@ -638,17 +639,37 @@ impl<D, F> fmt::Debug for AudioStreamAsync<D, F> {
 }
 
 impl<D, F> AudioStreamAsync<D, F> {
-    pub(crate) fn wrap_raw(
+    // SAFETY: `raw`, `shared_ptr` and `callback` 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: raw.into(),
+            raw: AudioStreamHandle(raw, shared_ptr),
             callback,
         }
     }
 }
 
+impl<D, F> Drop for AudioStreamAsync<D, F> {
+    fn drop(&mut self) {
+        // SAFETY: As long as the conditions on Self::wrap_raw are guaranteed on the creation of
+        // self, this is safe.
+        unsafe {
+            self.close();
+            self.raw.delete();
+
+            // NOTE: Currently there is no safe way to delete the AudioStreamCallback, so we are
+            // leaking it here.
+            // see https://github.com/google/oboe/issues/1610 and https://github.com/google/oboe/issues/1603
+
+            // replace this by `self.callback.delete()` when a fix upstream appear.
+            let _ = &self.callback;
+        }
+    }
+}
+
 impl<D, T> RawAudioStreamBase for AudioStreamAsync<D, T> {
     fn _raw_base(&self) -> &ffi::oboe_AudioStreamBase {
         unsafe { &*ffi::oboe_AudioStream_getBase(self.raw.0) }
@@ -688,14 +709,29 @@ impl<D, F> fmt::Debug for AudioStreamSync<D, F> {
 }
 
 impl<D, F> AudioStreamSync<D, F> {
-    pub(crate) fn wrap_raw(raw: *mut ffi::oboe_AudioStream) -> Self {
+    // SAFETY: `raw`, `shared_ptr` must be valid, because they will be deleted on drop.
+    pub(crate) unsafe fn wrap_raw(
+        raw: *mut ffi::oboe_AudioStream,
+        shared_ptr: *mut c_void,
+    ) -> Self {
         Self {
-            raw: raw.into(),
+            raw: AudioStreamHandle::new(raw, shared_ptr),
             _phantom: PhantomData,
         }
     }
 }
 
+impl<D, F> Drop for AudioStreamSync<D, F> {
+    fn drop(&mut self) {
+        // SAFETY: As long as the conditions on Self::wrap_raw are guaranteed on the creation of
+        // self, this is safe.
+        unsafe {
+            self.close();
+            self.raw.delete();
+        }
+    }
+}
+
 impl<D, T> RawAudioStreamBase for AudioStreamSync<D, T> {
     fn _raw_base(&self) -> &ffi::oboe_AudioStreamBase {
         unsafe { &*ffi::oboe_AudioStream_getBase(self.raw.0) }

src/audio_stream_builder.rs

@@ -1,9 +1,10 @@
 use num_traits::FromPrimitive;
 use oboe_sys as ffi;
 use std::{
+    ffi::c_void,
     fmt,
     marker::PhantomData,
-    mem::MaybeUninit,
+    mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
 };
 
@@ -49,10 +50,19 @@ impl DerefMut for AudioStreamBuilderHandle {
  */
 #[repr(transparent)]
 pub struct AudioStreamBuilder<D, C, T> {
-    raw: AudioStreamBuilderHandle,
+    raw: ManuallyDrop<AudioStreamBuilderHandle>,
     _phantom: PhantomData<(D, C, T)>,
 }
 
+impl<D, C, T> Drop for AudioStreamBuilder<D, C, T> {
+    fn drop(&mut self) {
+        // SAFETY: self.raw is only drop here, or taken in Self::destructs, which don't drop self.
+        unsafe {
+            ManuallyDrop::drop(&mut self.raw);
+        }
+    }
+}
+
 impl<D, C, T> fmt::Debug for AudioStreamBuilder<D, C, T> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         audio_stream_base_fmt(self, f)
@@ -81,16 +91,10 @@ impl Default for AudioStreamBuilder<Output, Unspecified, Unspecified> {
     }
 }
 
-impl<D, C, T> From<AudioStreamBuilder<D, C, T>> for AudioStreamBuilderHandle {
-    fn from(builder: AudioStreamBuilder<D, C, T>) -> Self {
-        builder.raw
-    }
-}
-
 impl<D, C, T> AudioStreamBuilder<D, C, T> {
     fn convert<D1, C1, T1>(self) -> AudioStreamBuilder<D1, C1, T1> {
         AudioStreamBuilder {
-            raw: self.into(),
+            raw: ManuallyDrop::new(self.destructs()),
             _phantom: PhantomData,
         }
     }
@@ -222,7 +226,7 @@ impl<D, C, T> AudioStreamBuilder<D, C, T> {
      * returns true. Otherwise __OpenSL ES__ will be used.
      */
     pub fn get_audio_api(&self) -> AudioApi {
-        FromPrimitive::from_i32(unsafe { ffi::oboe_AudioStreamBuilder_getAudioApi(&*self.raw) })
+        FromPrimitive::from_i32(unsafe { ffi::oboe_AudioStreamBuilder_getAudioApi(&**self.raw) })
             .unwrap()
     }
 
@@ -237,7 +241,7 @@ impl<D, C, T> AudioStreamBuilder<D, C, T> {
      * If the caller requests AAudio and it is supported then AAudio will be used.
      */
     pub fn set_audio_api(mut self, audio_api: AudioApi) -> Self {
-        unsafe { ffi::oboe_AudioStreamBuilder_setAudioApi(&mut *self.raw, audio_api as i32) }
+        unsafe { ffi::oboe_AudioStreamBuilder_setAudioApi(&mut **self.raw, audio_api as i32) }
         self
     }
 
@@ -444,6 +448,16 @@ impl<D, C, T> AudioStreamBuilder<D, C, T> {
         (audio_api == AudioApi::AAudio && Self::is_aaudio_supported())
             || (audio_api == AudioApi::Unspecified && Self::is_aaudio_recommended())
     }
+
+    /// 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) };
+
+        std::mem::forget(self);
+
+        raw
+    }
 }
 
 impl<D: IsDirection, C: IsChannelCount, T: IsFormat> AudioStreamBuilder<D, C, T> {
@@ -452,12 +466,23 @@ impl<D: IsDirection, C: IsChannelCount, T: IsFormat> AudioStreamBuilder<D, C, T>
      */
     pub fn open_stream(self) -> Result<AudioStreamSync<D, (T, C)>> {
         let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit();
-        let Self { mut raw, .. } = self;
-
-        wrap_status(unsafe {
-            ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr())
+        let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit();
+        let mut raw = self.destructs();
+
+        let stream = wrap_status(unsafe {
+            ffi::oboe_AudioStreamBuilder_openStreamShared(
+                &mut *raw,
+                stream.as_mut_ptr(),
+                shared_ptr.as_mut_ptr(),
+            )
         })
-        .map(|_| AudioStreamSync::wrap_raw(unsafe { stream.assume_init() }))
+        .map(|_| unsafe {
+            AudioStreamSync::wrap_raw(stream.assume_init(), shared_ptr.assume_init())
+        });
+
+        drop(raw);
+
+        stream
     }
 }
 
@@ -486,13 +511,13 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Input, C, T> {
         (T, C): IsFrameType,
     {
         let mut callback = AudioCallbackWrapper::<Input, F>::wrap(stream_callback);
-        let Self { mut raw, .. } = self;
+        let mut raw = self.destructs();
         unsafe {
             ffi::oboe_AudioStreamBuilder_setCallback(&mut *raw, callback.raw_callback());
         }
         AudioStreamBuilderAsync {
-            raw,
-            callback,
+            raw: ManuallyDrop::new(raw),
+            callback: ManuallyDrop::new(callback),
             _phantom: PhantomData,
         }
     }
@@ -523,13 +548,13 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Output, C, T> {
         (T, C): IsFrameType,
     {
         let mut callback = AudioCallbackWrapper::<Output, F>::wrap(stream_callback);
-        let Self { mut raw, .. } = self;
+        let mut raw = self.destructs();
         unsafe {
             ffi::oboe_AudioStreamBuilder_setCallback(&mut *raw, callback.raw_callback());
         }
         AudioStreamBuilderAsync {
-            raw,
-            callback,
+            raw: ManuallyDrop::new(raw),
+            callback: ManuallyDrop::new(callback),
             _phantom: PhantomData,
         }
     }
@@ -539,11 +564,25 @@ impl<C: IsChannelCount, T: IsFormat> AudioStreamBuilder<Output, C, T> {
  * Factory for an audio stream.
  */
 pub struct AudioStreamBuilderAsync<D, F> {
-    raw: AudioStreamBuilderHandle,
-    callback: AudioCallbackWrapper<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: the stream has not yet been open (Self::drop is not called after open_stream),
+        // so there is no data thread or error thread using this callback yet.
+        unsafe {
+            self.callback.delete();
+        }
+        // SAFETY: self.raw is only drop here, or taken in Self::destructs, which don't drop self.
+        unsafe {
+            ManuallyDrop::drop(&mut self.raw);
+        }
+    }
+}
+
 impl<D, F> fmt::Debug for AudioStreamBuilderAsync<D, F> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         audio_stream_base_fmt(self, f)
@@ -560,20 +599,43 @@ 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.
+        let raw = unsafe { ManuallyDrop::take(&mut self.raw) };
+        let callback = unsafe { ManuallyDrop::take(&mut self.callback) };
+
+        std::mem::forget(self);
+
+        (raw, callback)
+    }
+}
+
 impl<F: AudioInputCallback + Send> AudioStreamBuilderAsync<Input, F> {
     /**
      * Create and open an asynchronous (callback-driven) input stream based on the current settings.
      */
     pub fn open_stream(self) -> Result<AudioStreamAsync<Input, F>> {
         let mut stream = MaybeUninit::<*mut ffi::oboe_AudioStream>::uninit();
-        let Self {
-            mut raw, callback, ..
-        } = self;
-
-        wrap_status(unsafe {
-            ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr())
+        let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit();
+        let (mut raw, callback) = self.destructs();
+
+        let stream = wrap_status(unsafe {
+            ffi::oboe_AudioStreamBuilder_openStreamShared(
+                &mut *raw,
+                stream.as_mut_ptr(),
+                shared_ptr.as_mut_ptr(),
+            )
         })
-        .map(|_| AudioStreamAsync::wrap_raw(unsafe { stream.assume_init() }, callback))
+        .map(|_| unsafe {
+            AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback)
+        });
+
+        drop(raw);
+
+        stream
     }
 }
 
@@ -583,13 +645,22 @@ 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 Self {
-            mut raw, callback, ..
-        } = self;
-
-        wrap_status(unsafe {
-            ffi::oboe_AudioStreamBuilder_openStream(&mut *raw, stream.as_mut_ptr())
+        let mut shared_ptr = MaybeUninit::<*mut c_void>::uninit();
+        let (mut raw, callback) = self.destructs();
+
+        let stream = wrap_status(unsafe {
+            ffi::oboe_AudioStreamBuilder_openStreamShared(
+                &mut *raw,
+                stream.as_mut_ptr(),
+                shared_ptr.as_mut_ptr(),
+            )
         })
-        .map(|_| AudioStreamAsync::wrap_raw(unsafe { stream.assume_init() }, callback))
+        .map(|_| unsafe {
+            AudioStreamAsync::wrap_raw(stream.assume_init(), shared_ptr.assume_init(), callback)
+        });
+
+        drop(raw);
+
+        stream
     }
 }