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

Don't put nullptr into bus details pointer, fixes segfault on volume change #52187

Closed
wants to merge 1 commit into from

Conversation

ellenhp
Copy link
Contributor

@ellenhp ellenhp commented Aug 28, 2021

I think this fixes an issue brought up by @AndreaCatania in the comments of #51296. It was introduced by me in that PR, and this is my first shot at fixing it.

It was simpler for me to think about the process of updating the bus and volume information of a stream playback when the audio thread took complete ownership of the bus details structure that was active, but I couldn't figure out how to fix this bug in a reasonable way without rearchitecting that a bit. So now instead of inserting nullptr into the bus details pointer to prevent the main thread from freeing the bus details struct, the audio thread makes a local copy of the bus details. This does mean that the audio thread is technically vulnerable to a use after free if the system scheduler decides to sleep it between lines 385 and 387 for long enough that the the bus details struct gets updated and the old version gets free'd on the main thread during SafeList cleanup.

Speaking as someone generally who's very into safety at all costs, I really don't think it's worth planning for that happening.

@AndreaCatania
Copy link
Contributor

Thanks for the PR, I can confirm that this fix the issue. As momentary fix is great, however it would be great if you could improve it afterwards. Anyway, thanks a lot!!

@ellenhp
Copy link
Contributor Author

ellenhp commented Aug 30, 2021

Cool. Basically the idea here is that we're preserving the behavior where the bus details structure is never modified once it's applied to a playback, only replaced. It's a bit less obvious that it has that behavior now, but I'm not sure there are many obvious ways to improve it. I think that the potential use-after-free if the system sleeps the audio thread at the exact right moment for nearly an entire frame is an absolute non-issue. There are so many places where things like that could cause problems but it's just so unlikely.

@ellenhp ellenhp marked this pull request as ready for review August 30, 2021 14:03
@ellenhp
Copy link
Contributor Author

ellenhp commented Aug 30, 2021

I'm actually going to make this a draft because I want to investigate whether I could just replace all of my bus details pointers with values. I think when I built this to use pointers it was because the bus details struct was not trivially copiable, but I think it is now. No point in having all these pointers when I could probably just have an atomic that holds the entire structure.

@ellenhp ellenhp marked this pull request as draft August 30, 2021 14:07
@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 1, 2021

I take it back, the bus details struct is not trivially copiable. Marking as ready for review.

@ellenhp ellenhp marked this pull request as ready for review September 1, 2021 00:03
@mhilbrunner
Copy link
Member

cc @reduz

@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 5, 2021

This may be superseded by #52237. I made the same changes there.

I can split them up but it made sense to fix all outstanding bugs in the polyphony PR to make testing easy.

@ellenhp ellenhp marked this pull request as draft September 6, 2021 07:38
@ellenhp
Copy link
Contributor Author

ellenhp commented Sep 11, 2021

Superseded by #52237

@ellenhp ellenhp closed this Sep 11, 2021
@ellenhp ellenhp deleted the fix_bus_details_segfault branch September 11, 2021 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants