1181204 - heap-use-after-free in mozilla::MediaFormatReader::VideoIsHardwareAccelerated()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: asan_log.txt

I have seen this issue a few times now and I can reproduce it with my fuzzer in about an hour. I'll keep trying to get a reliable test case.

==33111==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040000c31d0 at pc 0x7f1a7c91223c bp 0x7f1a3b694330 sp 0x7f1a3b694328
READ of size 8 at 0x6040000c31d0 thread T66 (MediaPl~back #4)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: asan_log2.txt

Comment 2

[(Away)]

We're also seeing this in the wild as poison-crashes. It's one of the top crashes in aurora 41 and beta 40 is affected to a lesser degree.

1fccf9e0 11049e3e xul!mozilla::H264Converter::IsHardwareAccelerated+0xa
1fccf9e4 110316db xul!mozilla::MediaFormatReader::VideoIsHardwareAccelerated+0xf
1fccf9ec 110342b3 xul!mozilla::MediaDecoderStateMachine::GetAmpleVideoFrames+0x2b
1fccfa08 11036cd1 xul!mozilla::MediaDecoderStateMachine::HaveEnoughDecodedVideo+0x3c
1fccfa10 1102d383 xul!mozilla::MediaDecoderStateMachine::NeedToDecodeVideo+0x59
1fccfa1c 1103b751 xul!mozilla::MediaDecoderStateMachine::DispatchVideoDecodeTaskIfNeeded+0x2e
1fccfa28 1024e2ad xul!mozilla::MediaDecoderStateMachine::OnVideoPopped+0x25

mDecoder is 0x5a5a5a5a --> the H264Converter is dead.

Comment 3

[Jean-Yves Avenard [:jya]]

The core problem here is that we have the MDSM calling code that assumes we're only on the reader's task queue.
As such, we have no locking of any kind (and this is by design).

Bobby had started to remove those type of calls, looks like this one got through.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Prevent use of the decoder outside the reader's taskqueue.

This fix a potential race when the decoder could be shutting down on the reader's taskqueue while the MediaDecoderStateMachine thread attempts to read it.

Comment 5

[Jean-Yves Avenard [:jya]]

This is a fix, but IMHO we should never call anything in the Reader on the MDSM thread.

This also assumes that the MediaDataDecoder::IsHardwareAccelerated code is thread-safe. Currently it's probably fine as all implementations use a boolean that is only modified during MediaDataDecoder::Init().

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> This also assumes that the MediaDataDecoder::IsHardwareAccelerated code is
> thread-safe. Currently it's probably fine as all implementations use a
> boolean that is only modified during MediaDataDecoder::Init().

There are occasions where we need to get the result immediately like AudioSink::GetPosition where promise doesn't help. I think making the function thread-safe is the right way to go.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I'd date to say impossible

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? 37, but seems that the crashes are only seen in 41.
The MP4Reader has the same problem.

If not all supported branches, which bug introduced the flaw? 1128380

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? low. Need similar patch for MP4Reader if we are to support version < 39

How likely is this patch to cause regressions; how much testing does it need? very little. Having said that, the same level of confidence is what caused the problem in the first place :)

Comment 8

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

Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Sec-approval+ for trunk. Let's get Aurora and Beta patches made and nominated as well.

Comment 9

[Jean-Yves Avenard [:jya]]

So can I commit this?

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Approval Request Comment
[Feature/regressing bug #]: 1128380
[User impact if declined]: heap-use-after-free
[Describe test coverage new/current, TreeHerder]: in m-c. Locally tested.
[Risks and why]: Very low
[String/UUID change made/needed]: None

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8631922 [details] [diff] [review]
Prevent use of the decoder outside the reader's taskqueue.

Let's take this in beta6 and verify that the crash signature is not in 40. Beta+ Aurora+

Comment 12

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

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> So can I commit this?

sec-approval+ means you can commit to trunk. Aurora and Beta approvals mean you can commit to those branches.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6d27aa240d1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This is going to need a rebased patch for Beta.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/47e626a4275a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting to esr38/b2g37 now that you've done the MP4Reader rebasing anyway?

Comment 18

[Jean-Yves Avenard [:jya]]

Apparently not. The regression was introduced in 39.