MoofParser broken for some CENC files

RESOLVED FIXED in Firefox 37

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: eflores)

Tracking

(Blocks 2 bugs)

unspecified
mozilla38
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37 fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Bug 1110608 broke parsing of some fragmented CENC files. I'm disabling MoofParser for encrypted files in bug 1118593, but we still want to use MoofParser for encrypted files, since it fixes other issues.

So, this bug is to track fixing MoofParser for encrypted files.

STR:
1. Get gmp-clearkey from https://github.com/cpearce/gmp-clearkey
2. Load http://people.mozilla.org/~cpearce/eme/
3. Observe crash; the GMPDecryptor asserts that it expected the sample to have valid crypto data, but it doesn't.

In debugging this, it looked like the auxInfoType of the saiz box was 'sinf', and so we didn't setup crypto data. We set the default auxInfoType of the saiz box to "sinf", but ISO/IEC 23001-7:2012(E) says that for CENC files a auxInfoType of 'cenc' should be assumed. I still crashed when I changed the default, so that's not the only problem.
Assignee: nobody → edwin
Posted patch 1118597.patch (obsolete) — Splinter Review
Attachment #8551531 - Flags: review?(jyavenard)
Depends on: 1123507
Comment on attachment 8551531 [details] [diff] [review]
1118597.patch

Review of attachment 8551531 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a fan a cascade of parseXXX that you have now.

The existing style was to have constructor taking a Box and being "self-aware".

::: media/libstagefright/binding/Box.cpp
@@ +27,5 @@
> +    return 78;
> +  } else if (aType == AtomType("stsd")) {
> +    return FULLBOX_OFFSET + 4;
> +  }
> +

please document where those values come from

@@ +79,4 @@
>    }
>  
> +  mType = BigEndian::readUint32(&header[4]);
> +  mChildOffset = mBodyOffset + BoxOffset(mType);

probably should check that child offset is still within the box range and abort if not.

::: media/libstagefright/binding/Index.cpp
@@ +108,5 @@
>    }
>  
>    if (!s->mCencRange.IsNull()) {
> +    MoofParser* parser = mIndex->mMoofParser.get();
> +    MOZ_ASSERT(parser && parser->mSinf.IsValid());

can't we just exit and stop there. File won't play but at least it won't crash

@@ +116,4 @@
>      // The size comes from an 8 bit field
>      nsAutoTArray<uint8_t, 256> cenc;
>      cenc.SetLength(s->mCencRange.Length());
>      if (!mIndex->mSource->ReadAt(s->mCencRange.mStart, &cenc[0], cenc.Length(),

it's a carry-on from earlier, but I dislike the use of &cenc[0] ; this will assert if the array is empty.
cenc.Elements() won't

@@ +122,5 @@
>      }
>      ByteReader reader(cenc);
>      sample->crypto.valid = true;
> +    sample->crypto.iv_size = ivSize;
> +    reader.ReadArray(sample->crypto.iv, ivSize);

test return value of ReadArray

::: media/libstagefright/binding/MoofParser.cpp
@@ +203,5 @@
> +    // Some MP4 files have been found to have multiple sinf boxes in the same
> +    // enc* box. This does not match spec anyway, so just choose the first
> +    // one that parses properly.
> +    if (box.IsType("sinf") && !mSinf.IsValid()) {
> +      ParseSinf(box);

can't you simply break the loop instead? no need to loop when we're not going to do anything later

@@ +209,5 @@
> +  }
> +}
> +
> +static void
> +ParseSchm(Box& aBox, Sinf& aSinf)

a bit inconsistent with the rest of the code, make them private member of MoofReader

@@ +212,5 @@
> +static void
> +ParseSchm(Box& aBox, Sinf& aSinf)
> +{
> +  BoxReader reader(aBox);
> +  uint32_t flags = reader->ReadU32(); // ignore

test that we have 8 bytes available in BoxReader

@@ +220,5 @@
> +  reader->DiscardRemaining();
> +}
> +
> +static void
> +ParseTenc(Box& aBox, Sinf& aSinf)

and here

@@ +226,5 @@
> +  BoxReader reader(aBox);
> +  uint32_t flags = reader->ReadU32(); // ignore
> +
> +  uint32_t isEncrypted = reader->ReadU24();
> +  aSinf.mDefaultIVSize = reader->ReadU8();

test that we have 4 + 3 + 1 + 16...

@@ +229,5 @@
> +  uint32_t isEncrypted = reader->ReadU24();
> +  aSinf.mDefaultIVSize = reader->ReadU8();
> +
> +  memcpy(aSinf.mDefaultKeyID, reader->Read(16), 16);
> +}

The construction of the Sinf should be done in a Sinf constructor that takes a Box& argument (in line with the rest of the code)

@@ +232,5 @@
> +  memcpy(aSinf.mDefaultKeyID, reader->Read(16), 16);
> +}
> +
> +static void
> +ParseSchi(Box& aBox, Sinf& aSinf)

and here

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
@@ +119,5 @@
> +
> +  uint8_t mDefaultIVSize;
> +  AtomType mDefaultEncryptionType;
> +  uint8_t mDefaultKeyID[16];
> +};

Build the Sinf in a Sinf(Box& aBox) constructor, similar to all other atoms.

@@ +133,5 @@
>  
>  class Saiz
>  {
>  public:
> +  explicit Saiz(Box& aBox, Sinf& aSinf);

if you takes two arguments, you don't need explicit.

However, I think it should take the info type as argument rather than the Sinf; so we untangle what is starting to become messy. Keep it abstracted so a Saiz and Saio box don't need to be aware of the internal implementation of the Sinf

@@ +143,5 @@
>  
>  class Saio
>  {
>  public:
> +  explicit Saio(Box& aBox, Sinf& aSinf);

same as above, take the encryption type as argument
Attachment #8551531 - Flags: review?(jyavenard) → review-
Attachment #8551532 - Flags: review?(jyavenard) → review+
> +    if (box.IsType("sinf") && !mSinf.IsValid()) {
> +      ParseSinf(box);

I meant:
    if (box.IsType("sinf")) {
       ParseSinf(box);
       if (mSinf.IsValid()) {
         break;
       }

though ParseSinf should return a boolean that it has succeeded.
(Reporter)

Updated

4 years ago
Blocks: 1123498
Posted patch 1118597.patch v2 (obsolete) — Splinter Review
Addressed review comments and IRC comments.
Attachment #8551531 - Attachment is obsolete: true
Attachment #8551613 - Flags: review?(jyavenard)
Posted patch 1118597.patch v2.1 (obsolete) — Splinter Review
Actually address review comments and IRC comments.
Attachment #8551613 - Attachment is obsolete: true
Attachment #8551614 - Flags: review?(jyavenard)
Comment on attachment 8551614 [details] [diff] [review]
1118597.patch v2.1

Review of attachment 8551614 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: media/libstagefright/binding/Index.cpp
@@ +136,5 @@
>        uint16_t count = reader.ReadU16();
>        for (size_t i = 0; i < count; i++) {
>          sample->crypto.plain_sizes.AppendElement(reader.ReadU16());
>          sample->crypto.encrypted_sizes.AppendElement(reader.ReadU32());
>        }

existing problem, but as this patch is a fix, should test the cenc contains the right amount of bytes and is well formed (otherwise this will assert).

::: media/libstagefright/binding/MoofParser.cpp
@@ +173,5 @@
> +  for (Box box = aBox.FirstChild(); box.IsAvailable(); box = box.Next()) {
> +    // Some MP4 files have been found to have multiple sinf boxes in the same
> +    // enc* box. This does not match spec anyway, so just choose the first
> +    // one that parses properly.
> +    if (box.IsType("sinf") && !mSinf.IsValid()) {

mSinf.IsValid() isn't required as as soon it is valid we exit the loop

@@ +272,5 @@
>          tfdt = Tfdt(box);
>        } else if (box.IsType("trun")) {
>          ParseTrun(box, tfhd, tfdt, aMdhd, aEdts);
>        } else if (box.IsType("saiz")) {
> +        mSaizs.AppendElement(Saiz(box, aSinf));

mSaizs.AppendElement(Saiz(box, aSinf.GetEncryptionType())

::: media/libstagefright/binding/SinfParser.cpp
@@ +25,5 @@
> +      ParseSchi(box);
> +    }
> +  }
> +
> +  mSinf.CheckValid();

not required if you simply override IsValid()

@@ +37,5 @@
> +  if (reader->Remaining() < 8) {
> +    return;
> +  }
> +
> +  uint32_t flags = reader->ReadU32(); // ignore

warning unused variables...

@@ +62,5 @@
> +  if (reader->Remaining() < 24) {
> +    return;
> +  }
> +
> +  uint32_t flags = reader->ReadU32(); // ignore

warning unused variables...

::: media/libstagefright/binding/include/mp4_demuxer/Box.h
@@ +58,4 @@
>    uint64_t mChildOffset;
>    AtomType mType;
>    const Box* mParent;
>  };

Should probably have a method to check that Box is valid, as right now any errors reading the box and setting its member won't be propagated.

This can be done in a another patch/bug #

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
@@ +136,5 @@
>  
>  class Saiz : public Atom
>  {
>  public:
> +  Saiz(Box& aBox, Sinf& aSinf);

Saiz(Box& aBox, const AtomType& aEncryptionType)
Due to the implementation of AtomType, probably can not bother with const ref

@@ +146,5 @@
>  
>  class Saio : public Atom
>  {
>  public:
> +  Saio(Box& aBox, Sinf& aSinf);

Saio(Box& aBox, const AtomType& aEncryptionType)

::: media/libstagefright/binding/include/mp4_demuxer/SinfParser.h
@@ +21,5 @@
> +    , mDefaultIVSize(0)
> +  {}
> +  explicit Sinf(Box& aBox);
> +
> +  void CheckValid() { mValid = !!mDefaultIVSize && !!mDefaultEncryptionType; }

Use existing IsValid() API, no need to set mValid.
mValid is only of use if you don't have a IsValid() and you then use the default implementation

@@ +24,5 @@
> +
> +  void CheckValid() { mValid = !!mDefaultIVSize && !!mDefaultEncryptionType; }
> +
> +  uint8_t mDefaultIVSize;
> +  AtomType mDefaultEncryptionType;

something like ..
AtomType GetEncryptionType()
{
  return IsValid() ? mDefaultEncryptionType : AtomType()
}
Attachment #8551614 - Flags: review?(jyavenard) → review+
 0:03.87 Warning: -Wreorder in /Users/jyavenard/Work/Mozilla/mozilla-central/media/libstagefright/binding/include/mp4_demuxer/SinfParser.h: field 'mDefaultEncryptionType' will be initialized after field 'mDefaultIVSize'
 0:03.87 /Users/jyavenard/Work/Mozilla/mozilla-central/media/libstagefright/binding/include/mp4_demuxer/SinfParser.h:20:7: warning: field 'mDefaultEncryptionType' will be initialized after field 'mDefaultIVSize' [-Wreorder]
 0:03.87     : mDefaultEncryptionType()
 0:03.87       ^
 0:03.87 1 warning generated.

Something weird on my box:
1:05.58 ../../../dist/include/mp4_demuxer/MoofParser.h:8:10: fatal error: 'mp4_demuxer/Atom.h' file not found
 1:05.58 #include "mp4_demuxer/Atom.h"

jyapro:mozilla-central jyavenard$ ls -l media/libstagefright/binding/include/mp4_demuxer/Atom*
-rw-r--r--+ 1 jyavenard  staff  425 20 Jan 17:39 media/libstagefright/binding/include/mp4_demuxer/Atom.h
-rw-r--r--+ 1 jyavenard  staff  833 20 Jan 17:39 media/libstagefright/binding/include/mp4_demuxer/AtomType.h

it's there!
Oh..

It's missing the required entry in moz.build...
Comment on attachment 8551614 [details] [diff] [review]
1118597.patch v2.1

Review of attachment 8551614 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, on second look

Where are 

MoofParser::ParseMinf
MoofParser::ParseStbl
MoofParset::ParseStsd
MoofParser::ParseSinf

?
Attachment #8551614 - Flags: review+ → review-
Re-added missing stuff, whipped hg into line, addressed comments.
Attachment #8551614 - Attachment is obsolete: true
Attachment #8552063 - Flags: review?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> something like ..
> AtomType GetEncryptionType()
> {
>   return IsValid() ? mDefaultEncryptionType : AtomType()
> }

This shouldn't really matter. We only set mSinf if we have a valid Sinf, otherwise mDefaultEncryptionType is 0; further we don't bother with getters elsewhere in MoofParser.
Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

Review of attachment 8552063 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some nits.

::: media/libstagefright/binding/Index.cpp
@@ +131,5 @@
> +    if (!reader.ReadArray(sample->crypto.iv, ivSize)) {
> +      return nullptr;
> +    }
> +
> +    if (reader.CanReadType<uint16_t>()) {

CanRead16 ?

::: media/libstagefright/binding/SinfParser.cpp
@@ +14,5 @@
> +  SinfParser parser(aBox);
> +  if (parser.GetSinf().IsValid()) {
> +    *this = parser.GetSinf();
> +  }
> +}

as we can use mSinf.mDefaultEncryptionType without mSinf being valid, it needs to be initialized to 0 regardless.

::: media/libstagefright/binding/include/mp4_demuxer/SinfParser.h
@@ +17,5 @@
> +{
> +public:
> +  Sinf()
> +    : mDefaultEncryptionType()
> +    , mDefaultIVSize(0)

mDefaultIVSize must be initialized before mDefaultEncryptionType (cause a compilation warning otherwise)
Attachment #8552063 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/0f3a324e645d
https://hg.mozilla.org/mozilla-central/rev/06648aa9a5c4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

Requesting 37 uplift of just this patch.

Approval Request Comment
[Feature/regressing bug #]: MSE.
[User impact if declined]: Less consistent testing, harder to port critical fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c some time ago.
[Risks and why]: This is an EME patch, not an MSE one, but it does no harm and simplifies porting important MSE. Risk is minimal.
[String/UUID change made/needed]: None.
Attachment #8552063 - Flags: approval-mozilla-aurora?
Comment on attachment 8552063 [details] [diff] [review]
1118597.patch v3

I don't have much concern about this one as it has been on m-c for some time already. Let's make things easier on ourselves for MSE in 37. Aurora+
Attachment #8552063 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8551532 - Flags: approval-mozilla-aurora+
Flags: needinfo?(ajones)
Trying again. I added an include to work around the (linux32 debug only!) build failure in Sinfparser.cpp.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed269dd0a199
I don't see a crash when following these steps with Aurora, but I don't see any media either.
Flags: needinfo?(ajones)
You need to log in before you can comment on or make changes to this bug.