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

Thread safety gaps in AudioStreamAAudio #1180

Closed
philburk opened this issue Feb 1, 2021 · 2 comments · Fixed by #1200
Closed

Thread safety gaps in AudioStreamAAudio #1180

philburk opened this issue Feb 1, 2021 · 2 comments · Fixed by #1200
Assignees
Labels
bug P1 high priority
Milestone

Comments

@philburk
Copy link
Collaborator

philburk commented Feb 1, 2021

There are numerous method, eg. getTimestamp(), that reference the underlying native audio stream.
But that stream could get closed by an AAudio error callback that happened simultaneously.

AAudioStream *stream = mAAudioStream.load();
// <-- If close() occurred here then the stream would get used but the pointer is invalid!
if (stream != nullptr) {

We need to lock around references to mAAudioStream.

@philburk philburk added bug P1 high priority labels Feb 1, 2021
@philburk philburk self-assigned this Feb 1, 2021
@philburk
Copy link
Collaborator Author

philburk commented Feb 2, 2021

These may be called from a callback. A lock could hang the join() from the close().
So we need to do a trylock against a special mutex that is only held when closing the stream.
If the try lock fails then return the same error as if the stream was already closed.

@philburk
Copy link
Collaborator Author

A proper solution for real-time audio requires that the get() methods use a std::shared_mutex, which is C++17.

philburk added a commit that referenced this issue Feb 25, 2021
Protect native stream pointer with a shared_lock.

The race may have never ended badly.
But it was theoretically possible. So this prevents it.

Note that this requires that Oboe be built with C++17.
But apps can still use C++14.

Fixes #1180
philburk added a commit that referenced this issue Feb 25, 2021
Protect native stream pointer with a shared_lock.

The race may have never ended badly.
But it was theoretically possible. So this prevents it.

Note that this requires that Oboe be built with C++17.
But apps can still use C++14.

Fixes #1180
@philburk philburk added this to the v1.5.2 milestone Feb 26, 2021
philburk added a commit that referenced this issue Mar 9, 2021
* oboe: fix possible race on close

Protect native stream pointer with a shared_lock.

The race may have never ended badly.
But it was theoretically possible. So this prevents it.

Note that this requires that Oboe be built with C++17.
But apps can still use C++14.

Fixes #1180

* Add some comments about the DEBUG code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug P1 high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant