1181651 - crash in CmpInstructions

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(Away)]

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

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Who's working on the MP4 demuxer these days?

Comment 2

[Jean-Yves Avenard [:jya]]

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 ?

Comment 3

[(Away)]

Most of the URLs are... not appropriate to post here. :)

I've sent them to you in email (I bet it went to spam).

Comment 4

[(Away)]

On 40b6 the signature moved back to the "real" one.

Comment 5

[(Away)]

Any luck on the investigation?

Comment 6

[Jean-Yves Avenard [:jya]]

I wonder if this is related to bug 1185115.

The size could have an overflow while reallocating the array.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Replace use of stagefright::Vector with nsTArray

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 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8641607 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray

forgot to submit this for review...

Comment 9

[(Away)]

> 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

Comment 10

[Jean-Yves Avenard [:jya]]

I've already submitted a patch doing just that. It's been r+ already. 

It's in bug 1185115

Comment 11

[Jean-Yves Avenard [:jya]]

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.

Comment 12

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

Let's check this in two weeks into the cycle on August 25.

We'll want Aurora, Beta, and ESR38 patches for this.

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/966aa9c1ca25

Comment 15

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e758736c0c6

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e758736c0c6

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Since this landed a few days ago, and from comment 12, would you like to nominate this for uplift now? Thanks!

Comment 18

[Daniel Veditz [:dveditz]]

We still want this uplifted for 41 but the 40 ship has sailed.

Comment 19

[Jean-Yves Avenard [:jya]]

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 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08739e933fa8

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df21a7977994

Comment 23

[Ryan VanderMeulen [:RyanVM]]

As expected, esr38/b2g37 are going to need some rebasing.

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

patch for esr38 and earlier

Comment 25

[Jean-Yves Avenard [:jya]]

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 26

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

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

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8653264 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

See comment 19.

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8653264 [details] [diff] [review]
Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, a=ritu

Approved for uplift to ESR38

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/83451ec94212

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Jean-Yves Avenard [:jya]]

Using FallibleTArray instead:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25cc67413d6

Comment 32

[Jean-Yves Avenard [:jya]]

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

Comment 33

[Jean-Yves Avenard [:jya]]

Attachment: Replace use of stagefright::Vector with nsTArray. r=kentuckyfriedtakahe, r=nfroyd, a=lizzard

Comment 34

[Jean-Yves Avenard [:jya]]

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 35

[Jean-Yves Avenard [:jya]]

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

Comment 36

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/06735210ad2e

Comment 37

[Jean-Yves Avenard [:jya]]

bugger... gcc doesn't like it :(

Comment 38

[Jean-Yves Avenard [:jya]]

backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/7df0233a019d

Comment 39

[Jean-Yves Avenard [:jya]]

try is finally green.

https://hg.mozilla.org/releases/mozilla-esr38/rev/fd47fd82d25f

Comment 40

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f9afa7166832

Comment 41

[Carsten Book [:Tomcat]]

(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

Comment 42

[Jean-Yves Avenard [:jya]]

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