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

Will oboe use raw error callback when the client release shared pointer? #2007

Open
flamme opened this issue Apr 26, 2024 · 4 comments
Open
Assignees
Labels
bug P1 high priority

Comments

@flamme
Copy link
Collaborator

flamme commented Apr 26, 2024

From code AudioStreamAAudio::internalErrorCallback, it will try to promote the weak pointer to a shared pointer when there is an error callback. When the shared pointer exists, it will use the shared pointer for the error callback. Otherwise, it uses the raw pointer for the error pointer. My question here is that will there be a case that the client release the shared pointer before the error callback promote from the cached weak pointer? If that is the case, it looks like there can be crash happen as the raw pointer is also invalid.

@flamme flamme added the bug label Apr 26, 2024
@philburk philburk added the P0 very high priority label Apr 27, 2024
@philburk
Copy link
Collaborator

Interesting point. I think that code was designed to handle the case where an app did not use the shared_ptr interface to open the stream. But that branch may also be taken if someone used the shared pointer but closed the stream from the app before the error callback occured!

@philburk
Copy link
Collaborator

philburk commented Apr 30, 2024

I tried to force a race condition by adding a 4 second sleep in internalErrorCallback()
right before I promote the weak pointer to a strong pointer.
Then I play a stream in TEST OUTPUT,
unplugged the USB headset,
then hit the CLOSE button.

But instead of taking the RAW pointer branch it too this branch:

} else if (stream != oboeStream->getUnderlyingStream()) {
    LOGW("%s() stream already closed or closing", __func__); 

That is because the close() call nulled out the underlying stream.

We should write a test that verifies that the raw ptr branch is only taken when the app does not use the shared_ptr when opening a stream.
Then we can maybe remove the API for creating a stream using anything other than the shared_ptr.

@philburk philburk added P1 high priority and removed P0 very high priority labels Apr 30, 2024
@flamme
Copy link
Collaborator Author

flamme commented May 1, 2024

I took another look into the framework's code. And I noticed that we had the protection from framework side that it only called the error callback from the data callback thread. In that case, this should be safe if the client call close before freeing the shared pointer as that the stream can only be closed after joining the data callback thread.

It is not able to reproduce such a crash as the code is pretty well written for OboeTester. Looking into OboeTester's code, it closes the stream before freeing the shared pointer. However, I was able to cause a crash by removing the close. Of course, for causing a crash, the sleep in the oboe error callback is also added in my local code to simulate a race condition.

@philburk
Copy link
Collaborator

it only called the error callback from the data callback thread.

Is that true for both MMAP and Legacy streams?

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

No branches or pull requests

2 participants