1308423 - [App Verifier] Critical section not initialized in cubeb

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: App verifier error report

In xul!`anonymous namespace'::wasapi_stream_init+231 (e:\hg\mozilla-central\media\libcubeb\src\cubeb_wasapi.cpp @ 1675), cubeb copies owned_critical_section(), which is a wrapper around the windows CRITICAL_SECTION,

where on MSDN, "A critical section object cannot be moved or copied.": https://msdn.microsoft.com/zh-tw/library/windows/desktop/ms683472(v=vs.85).aspx

The danger of copying a CRITICAL_SECTION is that the temporary object is destroyed with DELETE_CRITICAL_SECTION, with its internal implementation containing HANDLEs and pointers copied to stm->stream_reset_lock. This may result in some use-after-free of pointers or system calls using invalid HANDLES.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

I am going to fix this in upstream.

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

For more information, owned_critical_section is copied in https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/media/libcubeb/src/cubeb_wasapi.cpp#1645

The posix version of owned_critical_section() has the same problem.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Bug 1308423 - Don't copy owned_critical_section in cubeb.

Review commit: https://reviewboard.mozilla.org/r/84218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84218/

Comment 4

[Matthew Gregan [:kinetik]]

Thanks, this looks good.  Would you mind submitting it as a PR against https://github.com/kinetiknz/cubeb so we can merge it there and then update in Gecko?

It's probably a good idea to make the copy constructor private on owned_critical_section to prevent other users from making the same mistake, too.

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Don't copy owned_critical_section

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=144d43d335a9

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 8799627 [details] [review]
Don't copy owned_critical_section

I merged this in https://github.com/kinetiknz/cubeb/commit/22557d466eceb6ff6ba70ae30d2dcd87648cde0b

Thanks for the PR!

Comment 8

[Matthew Gregan [:kinetik]]

Cervantes merged fix was uplifted into Gecko in bug 1308418.