Closed Bug 1308423 Opened 5 years ago Closed 5 years ago

[App Verifier] Critical section not initialized in cubeb

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: cyu, Assigned: cyu)

References

(Blocks 1 open bug)

Details

(Whiteboard: app_verifier)

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
Attachment #8798823 - Flags: review?(kinetik)
Rank: 10
Priority: -- → P1
Attachment #8798823 - Attachment is obsolete: true
Attachment #8799627 - Flags: review?(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!
Attachment #8799627 - Flags: review?(kinetik) → review+
Cervantes merged fix was uplifted into Gecko in bug 1308418.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.