Fix some unsoundess and crash issues #48
Conversation
This mainly fix issue katyo#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 katyo#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.
|
@katyo any updates? It would be nice to have this PR for cpal :). |
This allows updating glutin to 0.29, making the oboe-demo work on Android without using a patched version of glutin. Could have updated to 0.30, but that would imply in fixing breaking changes.
This class were already deprecated in oboe 1.5.0, and was splitted in AudioStreamErrorCallback and AudioStreamDataCallback. Removing the use of this class is one step in using the methods setErrorCallback and setDataCallback introduced in the last version, that will allow fixing the workaround were the callback is leaked.
The previous API for setErrorCallback didn't allow soundly deleting the errorCallback, which led oboe-rs to leak the callback. The new API receives holds a shared_ptr, fixing the issue.
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.
|
@katyo With the release of oboe 1.7.0, google/oboe#1610 was fixed, so I update the version and fixed the callback leaking. I also pull the changes you made in #49, and I got the CI working (see it working here Rodrigodd#1, in my fork). I have updated the version of glutin of the demo to fix the compiler errors on Android, so I also needed to update the ndk. So this PR is also superseding #51. |
| @@ -537,24 +534,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); | |||
There was a problem hiding this comment.
What about accessing to AudioStream via shared pointer here?
There was a problem hiding this comment.
Do you mean using something like *mut shared_ptr<oboe_AudioStream> instead of *mut c_void (and maybe ideally without the pointer indirection)?
I had no experience in creating bindings between Rust and C++, so I was not sure how to pass a shared_ptr between them. If you simply put std::shared_ptr<AudioStream> in oboe_AudioStreamBuilder_openStreamShared at C++ side, bindgen would use an opaque [u64; 2] type.
Maybe that was okay, or I could wrap the shared_ptr in another class or something (thinking back, that would allow accessing AudioStream through a getter, instead of keeping two pointers in Rust side). But I opted for allocating the shared_ptr on the heap, and passing a type-erased pointer to Rust side. This seemed to be less error-prone for me. But if anyone has a better idea on how to do it, I can change it.
There was a problem hiding this comment.
I mean I would like to avoid storing two pointers in AudioStreamHandle.
Typical shared_ptr<T> layout in C++ looks as follows:
|----------------------|
| shared_ptr sPtr1 |
|----------------------|
|Pointer to the Object |---|-------> Object
| | |
|Pointer to the Ctr blk|---|---|
|----------------------| | |---> Ctl block
| | (Pointer to allocator, deleter, ...)
|----------------------| | | (Shared reference counter)
| shared_ptr sPtr2 | | | (Weak reference counter + offset 1)
|----------------------| | |
|Pointer to the Object |---| |
| | |
|Pointer to the Ctr blk|-------|
|----------------------|
Seems opaque type that bindgen propose is that we need (actually it is a [*mut c_void; 2]).
To make things totally safe we may get raw pointer via function which we implement on C++ side.
|
Superseded by #49 I was too busy last year and I haven't maintained this crate in appropriate state. |
This mainly fix issue #41, that causes crashes when a
AudioStreamwas drop. That happen because theAudioStreamwas not closed on Drop, but was deleted, causing a use-after-free by the not closedonDataCallbackthread.Also, the method
AudioStreamBuilder::open_streamwas using the deprecated methodopenStream(AudioStream*), that do not allow deleting theAudioStreamsafely. Replaced it byopenStream(shared_ptr<AudioStream>). The deprecated function allowed a use-after-free by theonErrorCallbackthread.I implement this by creating a wrapper function,
oboe_AudioStreamBuilder_openStreamShared, that returns the pointer toAudioStreamand ac_voidpointer to a heap allocated shared_ptr (because std types have no stable ABI as far as I know), but I don't have much experience with ffi to tell if this is the best solution. Theshared_ptris deleted byoboe_AudioStream_deleteShared.Also, as noted by issue #45, the bindings for
AudioStream::close()were wrongly bound to the concrete implementation of the base class, instead of calling the virtual method. Apparently the bindgen crate does not generate bindings to virtual methods (see rust-lang/rust-bindgen#27), so other virtual methods, likestart,stop, etc. also has the same problem, but they implementation are not overridden, so they are not a real problem for the time being.Also note that currently there is no safe way to delete the
onErrorCallbackof aAudioStreamin oboe (see google/oboe#1610), so instead the current implementation leaks the callback on drop.Also, remove some unsound
Dropimplementations and replace them by explicit unsafe delete methods.