Closed
Bug 1181651
Opened 9 years ago
Closed 8 years ago
crash in CmpInstructions
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox40 | + | wontfix |
firefox41 | + | fixed |
firefox42 | + | fixed |
firefox43 | + | fixed |
firefox-esr38 | 41+ | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | affected |
b2g-v2.2r | --- | affected |
b2g-master | --- | fixed |
People
(Reporter: away, Assigned: jya)
References
(Blocks 1 open bug)
Details
(Keywords: crash, sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])
Crash Data
Attachments
(2 files, 1 obsolete file)
6.45 KB,
patch
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
7.24 KB,
patch
|
lizzard
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-f09852df-ab9d-4f18-bd12-e2f962150705. ============================================================= Moderately high crash on 40b1. Thanks to COMDAT folding, there is a substitute "cmp" function from IonMonkey, but rest assured this is media code. It looks like an out-of-bounds read on the array, since the addresses are often just over a page boundary. 0a53f394 6b2994f0 xul!CmpInstructions+0xf 0a53f3c0 6b2a14ff xul!stagefright::VectorImpl::sort+0x4d 0a53f44c 6b2a051c xul!stagefright::MPEG4Source::exportIndex+0x1b7 0a53f478 6b29eef4 xul!mp4_demuxer::MP4Metadata::ReadTrackIndex+0x52 0a53f4b0 6bc1d5f1 xul!mp4_demuxer::MP4Demuxer::Init+0xb8 0a53f4ec 6bc1eb72 xul!mozilla::InvokeAndRetry<mozilla::MP4Reader,bool>+0x37 0a53f520 6bb980de xul!mozilla::MP4Reader::ReadMetadata+0x3e 0a53f540 6bba32cd xul!mozilla::MediaDecoderReader::AsyncReadMetadata+0x9e 0a53f554 6bbae33c xul!mozilla::detail::MethodCallWithNoArgs<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>,enum mozilla::ReadMetadataFailureReason,1>,mozilla::MediaDecoderReader>::Invoke+0x11 0a53f564 6bb8c3a7 xul!mozilla::detail::ProxyRunnable<mozilla::MediaPromise<nsRefPtr<mozilla::MetadataHolder>,enum mozilla::ReadMetadataFailureReason,1> >::Run+0x12 0a53f580 6bbafad5 xul!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run+0x4f 0a53f5cc 6ad7b899 xul!mozilla::MediaTaskQueue::Runner::Run+0xb7 0a53f5f4 6aabf149 xul!nsThreadPool::Run+0x238
Flags: needinfo?(cpearce)
Comment 1•9 years ago
|
||
Who's working on the MP4 demuxer these days?
Flags: needinfo?(cpearce) → needinfo?(ajones)
Assignee | ||
Comment 2•9 years ago
|
||
All this code is disabled in 40 and later. Though it could likely occur with MediaFormatReader and the MP4Demuxer. David, any chance to have in the trace which media they were playing, a URL of some kind ?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dmajor)
Most of the URLs are... not appropriate to post here. :) I've sent them to you in email (I bet it went to spam).
Flags: needinfo?(dmajor)
On 40b6 the signature moved back to the "real" one.
Crash Signature: [@ CmpInstructions] → [@ CmpInstructions]
[@ stagefright::compositionOrder ]
Assignee | ||
Comment 6•9 years ago
|
||
I wonder if this is related to bug 1185115. The size could have an overflow while reallocating the array.
Flags: needinfo?(jyavenard)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jyavenard
Assignee | ||
Updated•9 years ago
|
Group: core-security
Assignee | ||
Updated•9 years ago
|
Depends on: CVE-2015-4479
Assignee | ||
Comment 7•9 years ago
|
||
stagefright::Vector is too crap. Too difficult to fix ; so it's much easier to simply not use it. This is the primary use we make of it. We make the allocation fallible so no more crashes, and the sort will be much faster than the terrible bubble sort they used.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8641607 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray forgot to submit this for review...
Attachment #8641607 -
Flags: review?(ajones)
> stagefright::Vector is too crap. Too difficult to fix ; so it's much easier > to simply not use it. Would it be worthwhile to remove stagefright::Vector from the codebase completely (as a separate bug)? I see only about 30 uses of it: https://dxr.mozilla.org/mozilla-central/search?q=type-ref%3Astagefright%3A%3AVector&redirect=true&case=true&limit=76&offset=0
Assignee | ||
Comment 10•8 years ago
|
||
I've already submitted a patch doing just that. It's been r+ already. It's in bug 1185115
Updated•8 years ago
|
Attachment #8641607 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8641607 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray [Security approval request comment] >How easily could an exploit be constructed based on the patch? I don't know how this actually happens. So probably hard to reproduce. >Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. > Which older supported branches are affected by this flaw? 35 and all using libstagefright, affects ESR38. > If not all supported branches, which bug introduced the flaw? We have several bugs at play, introducing different flaws. libstagefright was introduced in bug 908503. However it was only made active by default on windows for FF 35 (bug 1057879) > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? May require some special backports as this size of stagefright has changed a bit over time thanks to all the security fixes that have been applied at different stage. > How likely is this patch to cause regressions; how much testing does it need? Can't think of any.
Attachment #8641607 -
Flags: sec-approval?
Comment 12•8 years ago
|
||
Let's check this in two weeks into the cycle on August 25. We'll want Aurora, Beta, and ESR38 patches for this.
status-firefox40:
--- → wontfix
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
tracking-firefox43:
--- → +
tracking-firefox-esr38:
--- → ?
Keywords: sec-high
Whiteboard: [checkin on 8/25]
Updated•8 years ago
|
Attachment #8641607 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
tracking-firefox40:
--- → +
Comment 13•8 years ago
|
||
I'm going to override Al and ask that this be checked in now. Replacing the class used is not so obvious, and more testing is better.
Flags: needinfo?(jyavenard)
Whiteboard: [checkin on 8/25]
Assignee | ||
Comment 14•8 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/966aa9c1ca25
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e758736c0c6
https://hg.mozilla.org/mozilla-central/rev/6e758736c0c6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Since this landed a few days ago, and from comment 12, would you like to nominate this for uplift now? Thanks!
Flags: needinfo?(jyavenard)
Comment 18•8 years ago
|
||
We still want this uplifted for 41 but the 40 ship has sailed.
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8641607 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray Approval Request Comment [Feature/regressing bug #]:1034444 [User impact if declined]:Sec-high but most likely crashes [Describe test coverage new/current, TreeHerder]: In central [Risks and why]: Low, this is code we control, we just use our own safe nsTArray object in place of the unsafe stagefright::vector [String/UUID change made/needed]: None This will likely require tweaks to uplift to beta (or esr38 as the flaw is there too)
Flags: needinfo?(jyavenard)
Attachment #8641607 -
Flags: approval-mozilla-beta?
Attachment #8641607 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-v2.2r:
--- → affected
status-b2g-master:
--- → fixed
Comment on attachment 8641607 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray This fix has been in Nightly for a week, let's uplift to Aurora and Beta. Also since this fixes a crash/sec-high issue.
Attachment #8641607 -
Flags: approval-mozilla-beta?
Attachment #8641607 -
Flags: approval-mozilla-beta+
Attachment #8641607 -
Flags: approval-mozilla-aurora?
Attachment #8641607 -
Flags: approval-mozilla-aurora+
Comment 23•8 years ago
|
||
As expected, esr38/b2g37 are going to need some rebasing.
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 24•8 years ago
|
||
patch for esr38 and earlier
Assignee | ||
Comment 25•8 years ago
|
||
for some reason beta and earlier can't be build on my machine any longer, so I can't test. surprised it applied cleanly on beta.
Flags: needinfo?(jyavenard)
Comment 26•8 years ago
|
||
Comment 2 says that this is fixed in 40 and higher but then we marked 40 was "won't fix" and checked in for 41 and higher. Was 40 affected?
Comment 27•8 years ago
|
||
Comment on attachment 8653264 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu See comment 19.
Attachment #8653264 -
Flags: approval-mozilla-esr38?
Attachment #8653264 -
Flags: approval-mozilla-b2g37?
Updated•8 years ago
|
Attachment #8653264 -
Flags: approval-mozilla-b2g37?
Updated•8 years ago
|
Group: core-security → core-security-release
Comment on attachment 8653264 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu Approved for uplift to ESR38
Attachment #8653264 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Comment 30•8 years ago
|
||
Backed out from esr38 for bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=26470&repo=mozilla-esr38 https://hg.mozilla.org/releases/mozilla-esr38/rev/fcdc0a58d7c3
Flags: needinfo?(jyavenard)
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Assignee | ||
Comment 31•8 years ago
|
||
Using FallibleTArray instead: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25cc67413d6
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 32•8 years ago
|
||
hopefully it will be green this time (still no idea why I can't compile anything but >= 42) https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc07eabacffc
Assignee | ||
Comment 33•8 years ago
|
||
Attachment #8659000 -
Flags: review?(nfroyd)
Assignee | ||
Comment 34•8 years ago
|
||
Seems my patch didn't upload properly yesterday. This patch changes the rvalue copy constructor to be implicit just like the equivalent nsTArray is. I explicitly didn't want to convert the FallibleTArray into a nsTArray upon returning the variable as we had OOM issues on this code which is why we moved from infallible code to fallible in the first place. The other approach was to use an out-param but that would make the change significantly different between what's in >= 41 and esr38 and would cause later problem if any more need to be uplifted (and with stagefright it's rather a given)
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8659000 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, r=nfroyd, a=lizzard After discussing with gerald, will simply return a nsTArray
Attachment #8659000 -
Attachment is obsolete: true
Attachment #8659000 -
Flags: review?(nfroyd)
Assignee | ||
Comment 36•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/06735210ad2e
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 37•8 years ago
|
||
bugger... gcc doesn't like it :(
Assignee | ||
Comment 38•8 years ago
|
||
backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/7df0233a019d
Assignee | ||
Comment 39•8 years ago
|
||
try is finally green. https://hg.mozilla.org/releases/mozilla-esr38/rev/fd47fd82d25f
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
Comment 41•8 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #40) > https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f9afa7166832 had to back this out since this caused a bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=171275&repo=mozilla-b2g37_v2_2
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 42•8 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #41) > (In reply to Carsten Book [:Tomcat] from comment #40) > > https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f9afa7166832 > > had to back this out since this caused a bustage like > https://treeherder.mozilla.org/logviewer.html#?job_id=171275&repo=mozilla- > b2g37_v2_2 you probably need to use the same patch as the one for esr38...
Flags: needinfo?(jyavenard)
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•