Closed
Bug 790854
Opened 12 years ago
Closed 12 years ago
Invalid write [@ mozilla::MediaStream::Destroy] with mozCaptureStream, onloadedmetadata
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
Tracking | Status | |
---|---|---|
firefox16 | - | unaffected |
firefox17 | + | fixed |
firefox18 | + | fixed |
firefox19 | --- | fixed |
firefox-esr10 | --- | unaffected |
People
(Reporter: jruderman, Assigned: roc)
References
Details
(5 keywords, Whiteboard: [asan][adv-track-main17-])
Attachments
(3 files, 1 obsolete file)
216 bytes,
text/html
|
Details | |
1.76 KB,
text/plain
|
Details | |
1.22 KB,
patch
|
jesup
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1. Build Firefox with Address Sanitizer.
https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer
2. Load the testcase.
3. Quit Firefox.
Result: During a shutdown CC, ASan reports an invalid write to a freed heap object.
Reporter | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
Is this in old media code, or something that might have been tweaked by recent WebRTC changes?
status-firefox-esr10:
--- → ?
status-firefox18:
--- → affected
tracking-firefox18:
--- → +
Keywords: csec-uaf
Comment 3•12 years ago
|
||
Very likely related to the MediaStream changes (Bug 664918 and the follow-on)
Comment 4•12 years ago
|
||
Assigning to Randell, please reassign as necessary.
Assignee: nobody → rjesup
Updated•12 years ago
|
status-firefox16:
--- → affected
status-firefox17:
--- → affected
tracking-firefox16:
--- → -
tracking-firefox17:
--- → +
![]() |
||
Comment 5•12 years ago
|
||
FYI, the only crash in the wild during the last 4 weeks that contains mozilla::MediaStream::Destroy that I could find is bp-8378d53a-7474-4b6b-9562-7c08a2120928
Comment 6•12 years ago
|
||
Yeah, that MediaStream::Destroy is something else (Beta channel, 2 weeks ago, not during CC)
Comment 7•12 years ago
|
||
This was back on Alder with a much older overall codeset and older MediaStream impl. The crash line appears to be: " GraphImpl()->AppendMessage(new Message(this));" Probably GraphImpl had gone away already. This would have been rev 104778 of MediaStreamGraph.cpp (Aug 29th).
Any thoughts roc?
Updated•12 years ago
|
Flags: needinfo?(roc)
Assignee | ||
Comment 8•12 years ago
|
||
During a forced shutdown, after the MediaStreamGraph thread has been stopped, AppendMessage will synchronously process messages (via RunDuringShutdown()). In MediaStream::Destroy, the AppendMessage actually destroys the MediaStream. So we need to set mMainThreadDestroyed before AppendMessage.
Comment 9•12 years ago
|
||
Comment on attachment 674444 [details] [diff] [review]
fix
Review of attachment 674444 [details] [diff] [review]:
-----------------------------------------------------------------
That works. Can't the function have a nsRefPtr<> to itself to keep 'this' from going away until the function exits?
Attachment #674444 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #9)
> That works. Can't the function have a nsRefPtr<> to itself to keep 'this'
> from going away until the function exits?
I suppose so, but I hope the comment suffices.
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 674444 [details] [diff] [review]
fix
[Security approval request comment]
How easily can the security issue be deduced from the patch?
Very easily.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.
Which older supported branches are affected by this flaw?
Just trunk, Aurora and Beta.
If not all supported branches, which bug introduced the flaw?
Bug 779721.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply easily to all affected branches.
How likely is this patch to cause regressions; how much testing does it need?
Very unlikely to cause regressions; it's a very small change.
Attachment #674444 -
Flags: sec-approval?
Comment 12•12 years ago
|
||
Comment on attachment 674444 [details] [diff] [review]
fix
Let's get this in now. Please prepare patches for Aurora and Beta and nominate for those releases. We should fix this across the board.
Attachment #674444 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 674444 [details] [diff] [review]
fix
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): none yet
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Attachment #674444 -
Flags: approval-mozilla-beta?
Attachment #674444 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•12 years ago
|
||
Backed out due to test failure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/988a9fdc1294
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
The problem here is just that by setting mMainThreadDestroyed before calling AppendMessage, we're triggering the assertion in AppendMessage that mMainThreadDestroyed is not set.
Assignee | ||
Comment 18•12 years ago
|
||
You were right all along, Randell :-)
Attachment #674444 -
Attachment is obsolete: true
Attachment #674444 -
Flags: approval-mozilla-beta?
Attachment #674444 -
Flags: approval-mozilla-aurora?
Attachment #675365 -
Flags: review?(rjesup)
Updated•12 years ago
|
Attachment #675365 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 675365 [details] [diff] [review]
fix v2
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): a couple of days on m-c
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 675365 [details] [diff] [review]
fix v2
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): a couple of days on m-c
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Attachment #675365 -
Flags: approval-mozilla-beta?
Attachment #675365 -
Flags: approval-mozilla-aurora?
Comment 23•12 years ago
|
||
Comment on attachment 675365 [details] [diff] [review]
fix v2
sec-critical, been on trunk a few days, approving for uplift to branches.
Attachment #675365 -
Flags: approval-mozilla-beta?
Attachment #675365 -
Flags: approval-mozilla-beta+
Attachment #675365 -
Flags: approval-mozilla-aurora?
Attachment #675365 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•12 years ago
|
||
Comment 25•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Bug caused by (feature/regressing bug #): 779721
If this is caused by 779721, which came in during August, why is Firefox 16 affected? I only see a Mozilla Central checkin (series) on August 9, 2012.
Assignee | ||
Comment 26•12 years ago
|
||
I don't know why dveditz marked this as affecting FF16. I don't think it is.
Comment 27•12 years ago
|
||
I marked it based on your guess in comment 3 that it was related to bug 664918 (landed in Fx 15) and it wasn't updated based on the information in comment 11.
Keywords: regression
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [asan] → [asan][adv-track-main17-]
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•