Closed Bug 1451681 Opened 6 years ago Closed 6 years ago

Crash in InvalidArrayIndex_CRASH | mozilla::AnnexB::ConvertSampleToAnnexB

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
59 Branch
Platform:
Unspecified
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: philipp, Assigned: jya)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

Bug 1451681 - Handle case where crypto plain size definition didn't exist.
59 bytes, text/x-review-board-request
cpearce
: review+
jcristau
: approval-mozilla-beta+
Details 
This bug was filed from the Socorro interface and is
report bp-926f58a4-7c82-4def-b5b4-5b6160180320.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.dylib MOZ_CrashPrintf mfbt/Assertions.cpp:50
1 XUL InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:26
2 XUL mozilla::AnnexB::ConvertSampleToAnnexB xpcom/ds/nsTArray.h:1017
3 XUL mozilla::H264Converter::Decode dom/media/platforms/wrappers/H264Converter.cpp:124
4 XUL mozilla::MediaFormatReader::DecoderFactory::Wrapper::Decode dom/media/MediaFormatReader.cpp:701
5 XUL mozilla::MediaFormatReader::DecodeDemuxedSamples dom/media/MediaFormatReader.cpp:2374
6 XUL mozilla::MediaFormatReader::HandleDemuxedSamples dom/media/MediaFormatReader.cpp:2488
7 XUL mozilla::MediaFormatReader::Update dom/media/MediaFormatReader.cpp:2875
8 XUL mozilla::detail::RunnableMethodImpl<mozilla::MediaFormatReader*, void  xpcom/threads/nsThreadUtils.h:1142
9 XUL mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:214

=============================================================

these content crashes are starting to show up on win/mac in firefox 59 - perhaps related to bug 1417795. The reports all come with MOZ_CRASH Reason 	ElementAt(aIndex = 0, aLength = 0) added in bug 1159244.

the comment to this report might give clues what's going wrong:
"Was attempting to play widevine EME content. 192kbps 29.97 CMAF video segments, with aac-64kbps CMAF audio segments. Tab crashed fairly immediately."
status-firefox59: affected → wontfix
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
The only place we're attempting to do a ElementAt(0) in this code is there:
https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/media/platforms/agnostic/bytestreams/AnnexB.cpp#25

if (!IsAVCC(aSample))

which calls:
https://searchfox.org/mozilla-central/source/dom/media/platforms/agnostic/bytestreams/AnnexB.cpp#322
 return aSample->Size() >= 3 && aSample->mExtraData &&
    aSample->mExtraData->Length() >= 7 && (*aSample->mExtraData)[0] == 1;

so we attempt to call ElementAt(0) on an array that is at least 7 bytes long...

so it makes no sense, or the stack trace is rubbish
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

https://reviewboard.mozilla.org/r/234872/#review240796

r+ with patch rewritten; we still need to adjust mPlainSizes when mPlainSizes is emtpy.

::: dom/media/platforms/agnostic/bytestreams/AnnexB.cpp:76
(Diff revision 1)
>  
>      // Prepending the NAL with SPS/PPS will mess up the encryption subsample
>      // offsets. So we need to account for the extra bytes by increasing
>      // the length of the first clear data subsample. Otherwise decryption
>      // will fail.
> -    if (aSample->mCrypto.mValid) {
> +    if (aSample->mCrypto.mValid &&

Section 7 of ISO/IEC 23001-7:2012(E) states:

"If sub-sample encryption is not used (sample_info_size equals IV_size), then the entire sample is encrypted"

So if mPlainSizes.Length() == 0, we need to assume the entire sample is encrypted. Which means we still need to do this step of updating mPlainSizes[0], else we'll end up with the CDM trying to decrypt the annexB header.

So I think what we need to do here is wherever we first construct the CryptoSample object here, if there are no mPlainSizes/mEncryptedSizes, we need to insert 0 into mPlainSizes and the length of the buffer into mEncryptedSizes, so that it's valid.

Or we could handle an empty mPlainSizes here, but it seems more robust to do the former.
Attachment #8966140 - Flags: review?(cpearce) → review+
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

https://reviewboard.mozilla.org/r/234872/#review240796

> Section 7 of ISO/IEC 23001-7:2012(E) states:
> 
> "If sub-sample encryption is not used (sample_info_size equals IV_size), then the entire sample is encrypted"
> 
> So if mPlainSizes.Length() == 0, we need to assume the entire sample is encrypted. Which means we still need to do this step of updating mPlainSizes[0], else we'll end up with the CDM trying to decrypt the annexB header.
> 
> So I think what we need to do here is wherever we first construct the CryptoSample object here, if there are no mPlainSizes/mEncryptedSizes, we need to insert 0 into mPlainSizes and the length of the buffer into mEncryptedSizes, so that it's valid.
> 
> Or we could handle an empty mPlainSizes here, but it seems more robust to do the former.

the samples are constructed in multiple demuxers.

seems easier to me to just handle that in the conversion.
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

https://reviewboard.mozilla.org/r/234872/#review240796

I would have r- that then :)
Would feel more confident if you had another look Chris.
thank you
Flags: needinfo?(cpearce)
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

https://reviewboard.mozilla.org/r/234872/#review241146

::: dom/media/platforms/agnostic/bytestreams/AnnexB.cpp:80
(Diff revision 2)
>      // will fail.
>      if (aSample->mCrypto.mValid) {
> -      MOZ_ASSERT(samplewriter->mCrypto.mPlainSizes.Length() > 0);
> +      if (samplewriter->mCrypto.mPlainSizes.Length() > 0) {
> -      samplewriter->mCrypto.mPlainSizes[0] += annexB->Length();
> +        samplewriter->mCrypto.mPlainSizes[0] += annexB->Length();
> +      } else {
> +        samplewriter->mCrypto.mPlainSizes.AppendElement(annexB->Length());

In theory, we shouldn't encounter CryptoSamples with mPlainSizes.Length()==0 with the change you made above... And if we actually do encounter an empty mPlainSizs and we insert into mPlainSizes but leave an empty mEncryptedSizes we could confuse downstream users, so maybe you want to return an error here instead, as we effectively have invalid input here?

Maybe there's somewhere we can add an assertion that if mCrypto is valid we must have non-empty mPlainSizes and mEncryptedSizes? Maybe in SampleIterator::GetNext()?
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

https://reviewboard.mozilla.org/r/234872/#review241146

> In theory, we shouldn't encounter CryptoSamples with mPlainSizes.Length()==0 with the change you made above... And if we actually do encounter an empty mPlainSizs and we insert into mPlainSizes but leave an empty mEncryptedSizes we could confuse downstream users, so maybe you want to return an error here instead, as we effectively have invalid input here?
> 
> Maybe there's somewhere we can add an assertion that if mCrypto is valid we must have non-empty mPlainSizes and mEncryptedSizes? Maybe in SampleIterator::GetNext()?

there are tests everywhere that mPlainSizes.Length() == mEncryptedSizes.Length() so I don't think it would be a problem...
Flags: needinfo?(cpearce)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2198e282bdcf
Handle case where crypto plain size definition didn't exist. r=cpearce
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5159724ccfc6
Handle case where crypto plain size definition didn't exist. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/5159724ccfc6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

Approval Request Comment
[Feature/Bug causing the regression]: 1341894 
[User impact if declined]: Encrypted content will not play (including Facebook videos)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not directly, no crash in nightly since fix landed
[Needs manual test from QE? If yes, steps to reproduce]: Hard to say how to reproduce
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: we ensure that we don't access an array when its size is 0
[String changes made/needed]: none
Flags: needinfo?(jyavenard)
Attachment #8966140 - Flags: approval-mozilla-beta?
This seems to be too low volume for nightly to prove anything.  I guess when you say "Encrypted content will not play" it's a small subset of encrypted content, since we don't have STR?
(In reply to Jean-Yves Avenard [:jya] from comment #17)
> Comment on attachment 8966140 [details]
> Bug 1451681 - Handle case where crypto plain size definition didn't exist.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: 1341894 
> [User impact if declined]: Encrypted content will not play (including
> Facebook videos)

I wasn't aware that Facebook used EME? But however, I think the videos that this will crash on are videos which have encryption data which encode the number of clear/cipher subsamples a sample contains as 0. Normally this is represented in encrypted video by omitting the subsamples box, rather than having a box containing with a subsample count of 0.

I think most encrypted media isn't encoded this way, but there must be a subset out there that is (hence this low volume crash).

> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: not directly, no crash in nightly
> since fix landed
> [Needs manual test from QE? If yes, steps to reproduce]: Hard to say how to
> reproduce

To repro, we'd need to find a video encoded this way. Maybe there's URLs in the crash reports?
Most reports include no URL, but the ones that do are from tv.youtube.com
Comment on attachment 8966140 [details]
Bug 1451681 - Handle case where crypto plain size definition didn't exist.

let's get this on beta for 60.0b14
Attachment #8966140 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: