Properly verify that input data is just 'pssh' boxes

Properly verify that input data is just 'pssh' boxes

ReadAllChildren() doesn't check the box type, so add a check into
ProtectionSystemSpecificHeader::Parse() that does.

BUG=501612
TEST=new test passes

Committed: https://crrev.com/a9db3fe426268a67d808fe964d4fc9f28271fd17
Cr-Commit-Position: refs/heads/master@{#338171}

[jrummell, 2015-06-18 01:29:39]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[jrummell, 2015-06-18 01:29:40]

PTAL.

[sandersd (OOO until July 31), 2015-06-22 20:12:48]

lgtm

[ddorwin, 2015-06-25 23:08:17]

https://codereview.chromium.org/1189073004/diff/1/media/cdm/cenc_utils_unitte...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1189073004/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:519: 'f',  'r',  'e',  'e'
Perhaps p s s g, to ensure all bytes are being checked.

https://codereview.chromium.org/1189073004/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1189073004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:29: // Don't bother validating the box's
contents other than type().
It's not "don't bother" - it's Fail if not the correct type.

This seems like something that should be consistent for all box parsers.

Should they all contain this? Would that be a DCHECK in most cases? (We would do
that in a separate CL after branch cut.) It seems odd to have PSSH be the
exception as appears to currently be the case.

[jrummell, 2015-06-30 00:06:03]

Updated.

https://codereview.chromium.org/1189073004/diff/1/media/cdm/cenc_utils_unitte...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1189073004/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:519: 'f',  'r',  'e',  'e'
On 2015/06/25 23:08:17, ddorwin wrote:
> Perhaps p s s g, to ensure all bytes are being checked.

Done.

https://codereview.chromium.org/1189073004/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1189073004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:29: // Don't bother validating the box's
contents other than type().
On 2015/06/25 23:08:17, ddorwin wrote:
> It's not "don't bother" - it's Fail if not the correct type.
> 
> This seems like something that should be consistent for all box parsers.
> 
> Should they all contain this? Would that be a DCHECK in most cases? (We would
do
> that in a separate CL after branch cut.) It seems odd to have PSSH be the
> exception as appears to currently be the case.

The "don't bother" is in relation to all the other fields in the pssh box that
could be validated but aren't -- version, flags, KID_count, KIDs, datasize,
data.

There are a few places where it reads boxes that can have different types (see
SampleDescription::Parse, line 291 below that calls
ReadAllChildren(&audio_entries) and then AudioSampleEntry::Parse @ 568). So
adding the test in ReadAllChildren() (or all Boxes) would possibly break them.

Most of the parsing is done by calling ScanChildren() and then using ReadChild()
to read a particular type. So ReadAllChildren() is different. I could make a
variation (or add an additional parameter) for the validation case -- would that
be clearer?

[jrummell, 2015-06-30 20:50:35]

Updated to add ReadAllChildrenAndCheckFourCC(). Also did a rebase to pick up the
latest changes in cenc_util before making these new changes (affects the
cenc_util_test only).

[ddorwin, 2015-07-01 20:22:35]

LGTM with comments. Thanks!

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
File media/formats/mp4/box_reader.h (right):

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:143: // Note that this method is mutually
exclusive with ScanChildren().
and ReadAllChildrenAndCheckFourCC()

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:145: bool ReadAllChildren(std::vector<T>*
children) WARN_UNUSED_RESULT {
Given the comment about this being called in one place, it seems we should
rename this to discourage use. Maybe in a separate CL.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:150: // Note that this method is mutually
exclusive with ScanChildren().
and ReadAllChildren()

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:151: template <typename T>
// Returns true if all children are successfully parsed(?) and have the correct
box type for |T|.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:184: template <typename T>
// Returns true if all children are successfully parsed(?) and, if
|check_box_type|,  have the correct box type for |T|.

[jrummell, 2015-07-01 20:58:00]

Thanks for the reviews.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
File media/formats/mp4/box_reader.h (right):

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:143: // Note that this method is mutually
exclusive with ScanChildren().
On 2015/07/01 20:22:35, ddorwin wrote:
> and ReadAllChildrenAndCheckFourCC()

Done.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:145: bool ReadAllChildren(std::vector<T>*
children) WARN_UNUSED_RESULT {
On 2015/07/01 20:22:35, ddorwin wrote:
> Given the comment about this being called in one place, it seems we should
> rename this to discourage use. Maybe in a separate CL.

Acknowledged.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:150: // Note that this method is mutually
exclusive with ScanChildren().
On 2015/07/01 20:22:35, ddorwin wrote:
> and ReadAllChildren()

Done.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:151: template <typename T>
On 2015/07/01 20:22:35, ddorwin wrote:
> // Returns true if all children are successfully parsed(?) and have the
correct
> box type for |T|.

Done.

https://codereview.chromium.org/1189073004/diff/40001/media/formats/mp4/box_r...
media/formats/mp4/box_reader.h:184: template <typename T>
On 2015/07/01 20:22:35, ddorwin wrote:
> // Returns true if all children are successfully parsed(?) and, if
> |check_box_type|,  have the correct box type for |T|.

Done.

[jrummell, 2015-07-01 20:58:06]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-07-01 20:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1189073004/#ps60001
(title: "expand comments")

[commit-bot: I haz the power, 2015-07-01 20:58:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1189073004/60001

[commit-bot: I haz the power, 2015-07-01 21:09:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-01 21:09:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[jrummell, 2015-07-01 23:28:28]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-07-01 23:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
ddorwin@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1189073004/#ps50004
(title: "Android compile issues")

[commit-bot: I haz the power, 2015-07-01 23:29:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1189073004/50004

[commit-bot: I haz the power, 2015-07-02 01:35:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-02 01:35:55]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[jrummell, 2015-07-09 21:15:01]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-07-09 21:17:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1189073004/50004

[commit-bot: I haz the power, 2015-07-09 22:34:58]

Committed patchset #5 (id:50004)

[commit-bot: I haz the power, 2015-07-09 22:35:55]

Patchset 5 (id:??) landed as
https://crrev.com/a9db3fe426268a67d808fe964d4fc9f28271fd17
Cr-Commit-Position: refs/heads/master@{#338171}