6.45 KB, patch
|Details | Diff | Splinter Review|
7.24 KB, patch
|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
Who's working on the MP4 demuxer these days?
Flags: needinfo?(cpearce) → needinfo?(ajones)
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 ?
4 years ago
Most of the URLs are... not appropriate to post here. :) I've sent them to you in email (I bet it went to spam).
On 40b6 the signature moved back to the "real" one.
Crash Signature: [@ CmpInstructions] → [@ CmpInstructions] [@ stagefright::compositionOrder ]
Any luck on the investigation?
I wonder if this is related to bug 1185115. The size could have an overflow while reallocating the array.
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.
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
I've already submitted a patch doing just that. It's been r+ already. It's in bug 1185115
4 years ago
Attachment #8641607 - Flags: review?(ajones) → review+
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?
Let's check this in two weeks into the cycle on August 25. We'll want Aurora, Beta, and ESR38 patches for this.
Whiteboard: [checkin on 8/25]
Attachment #8641607 - Flags: sec-approval? → sec-approval+
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.
Whiteboard: [checkin on 8/25]
Since this landed a few days ago, and from comment 12, would you like to nominate this for uplift now? Thanks!
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)
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.
As expected, esr38/b2g37 are going to need some rebasing.
patch for esr38 and earlier
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.
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 on attachment 8653264 [details] [diff] [review] Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu See comment 19.
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+
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
Using FallibleTArray instead: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25cc67413d6
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
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)
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
bugger... gcc doesn't like it :(
try is finally green. https://hg.mozilla.org/releases/mozilla-esr38/rev/fd47fd82d25f
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
(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
(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...
You need to log in before you can comment on or make changes to this bug.