How safely delete the error callback? #1610
Assignees
Milestone
Comments
|
We hear your concerns and are discussing potential solutions. See #1603. We are considering a method to pass a shared_ptr in AudioStreamBuilder.setErrorCallback. |
Rodrigodd
added a commit
to Rodrigodd/oboe-rs
that referenced
this issue
Aug 26, 2022
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.
|
We added an API change that makes it easier to safely delete an error callback by using a shared_ptr. See #1626 We will document this technique and then can close this issue. |
|
Documented now. |
katyo
pushed a commit
to katyo/oboe-rs
that referenced
this issue
Jan 17, 2023
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I am trying to write safe rust bindings for oboe, but I found the following situation.
For asynchronous error handling, ones must pass a pointer to a
AudioStreamErrorCallbacktoAudioStreamBuilder::setErrorCallback, which is later passed to theAudioStream. In this scenario, the caller holds ownership of the callback (to allow sharing the error callback between multiple Streams, I suppose), and so it must delete it.But I cannot find a safe way of doing it.
When a
AudioStreamreceives an error, the error handler spawns a new thread that holds a reference to theAudioStreamusing ashared_ptr, and from that stream gets a raw pointer to theErrorCallback(as seem here). But in the meantime, the main thread could have started closing and deleting the stream and callback.The
shared_ptrsolves the problem of deleting theAudioStreamfrom another thread (as discussed in #820 and #821), but I cannot find a way to guarantee that theErrorCallbackis not being used by the error callback thread when I try to delete it in the main thread.I have thought of:
onErrorAfterClose, but an error may never happen, and the callback would leak.The only safe way that I find of handling this situation is to leak the callback, but this is not ideal.
The text was updated successfully, but these errors were encountered: