790854 - Invalid write [@ mozilla::MediaStream::Destroy] with mozCaptureStream, onloadedmetadata

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Veditz [:dveditz]]

Is this in old media code, or something that might have been tweaked by recent WebRTC changes?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Very likely related to the MediaStream changes (Bug 664918 and the follow-on)

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Assigning to Randell, please reassign as necessary.

Comment 5

[Robert Kaiser]

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

[Randell Jesup [:jesup] (needinfo me)]

Yeah, that MediaStream::Destroy is something else (Beta channel, 2 weeks ago, not during CC)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15c4d6334107

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out due to test failure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/988a9fdc1294

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=16472644&tree=Mozilla-Inbound&full=1

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem here is just that by setting mMainThreadDestroyed before calling AppendMessage, we're triggering the assertion in AppendMessage that mMainThreadDestroyed is not set.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

You were right all along, Randell :-)

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d21b8f2c4cb6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d21b8f2c4cb6

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 675365 [details] [diff] [review]
fix v2

sec-critical, been on trunk a few days, approving for uplift to branches.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86537d2d97a9
https://hg.mozilla.org/releases/mozilla-beta/rev/9ddea1403eef

Comment 25

[Al Billings [:abillings - ex-MoCo]]

(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.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know why dveditz marked this as affecting FF16. I don't think it is.

Comment 27

[Daniel Veditz [:dveditz]]

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.