Commit f7b33b92 authored by Dan Sanders's avatar Dan Sanders Committed by Commit Bot

[media] Eliminate (bool* err) pattern in MSE mp4 parsing.

Change-Id: Ib3db1d2c3b4ff7d0fabbbe7f5c952a0b1ecffad3
Reviewed-on: https://chromium-review.googlesource.com/729705Reviewed-by: default avatarMax Moroz <mmoroz@chromium.org>
Reviewed-by: default avatarMatthew Wolenetz <wolenetz@chromium.org>
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512301}
parent 771998f4
......@@ -75,8 +75,11 @@ source_set("formats") {
"mp4/box_reader.h",
"mp4/es_descriptor.cc",
"mp4/es_descriptor.h",
"mp4/fourccs.h",
"mp4/mp4_stream_parser.cc",
"mp4/mp4_stream_parser.h",
"mp4/parse_result.h",
"mp4/rcheck.h",
"mp4/sample_to_group_iterator.cc",
"mp4/sample_to_group_iterator.h",
"mp4/track_run_iterator.cc",
......
......@@ -8,7 +8,6 @@
#include <string.h>
#include <algorithm>
#include <memory>
#include <set>
#include "media/formats/mp4/box_definitions.h"
......@@ -115,44 +114,31 @@ BoxReader::~BoxReader() {
}
// static
BoxReader* BoxReader::ReadTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
bool* err) {
ParseResult BoxReader::ReadTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
std::unique_ptr<BoxReader>* out_reader) {
DCHECK(out_reader);
std::unique_ptr<BoxReader> reader(
new BoxReader(buf, buf_size, media_log, false));
if (!reader->ReadHeader(err))
return NULL;
// BoxReader::ReadHeader is expected to return false if box_size > buf_size.
// This may happen if a partial mp4 box is appended, or if the box header is
// corrupt.
CHECK(reader->box_size() <= static_cast<uint64_t>(buf_size));
if (!IsValidTopLevelBox(reader->type(), media_log)) {
*err = true;
return NULL;
}
return reader.release();
RCHECK_OK_PARSE_RESULT(reader->ReadHeader());
if (!IsValidTopLevelBox(reader->type(), media_log))
return ParseResult::kError;
*out_reader = std::move(reader);
return ParseResult::kOk;
}
// static
bool BoxReader::StartTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
FourCC* type,
size_t* box_size,
bool* err) {
BoxReader reader(buf, buf_size, media_log, false);
if (!reader.ReadHeader(err)) return false;
if (!IsValidTopLevelBox(reader.type(), media_log)) {
*err = true;
return false;
}
*type = reader.type();
*box_size = reader.box_size();
return true;
ParseResult BoxReader::StartTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
FourCC* out_type,
size_t* out_box_size) {
std::unique_ptr<BoxReader> reader;
RCHECK_OK_PARSE_RESULT(ReadTopLevelBox(buf, buf_size, media_log, &reader));
*out_type = reader->type();
*out_box_size = reader->box_size();
return ParseResult::kOk;
}
// static
......@@ -206,16 +192,16 @@ bool BoxReader::ScanChildren() {
DCHECK(!scanned_);
scanned_ = true;
bool err = false;
while (pos_ < box_size_) {
BoxReader child(&buf_[pos_], box_size_ - pos_, media_log_, is_EOS_);
if (!child.ReadHeader(&err)) break;
if (child.ReadHeader() != ParseResult::kOk)
return false;
children_.insert(std::pair<FourCC, BoxReader>(child.type(), child));
pos_ += child.box_size();
}
return !err && pos_ == box_size_;
DCHECK(pos_ == box_size_);
return true;
}
bool BoxReader::HasChild(Box* child) {
......@@ -249,57 +235,50 @@ bool BoxReader::ReadFullBoxHeader() {
return true;
}
bool BoxReader::ReadHeader(bool* err) {
ParseResult BoxReader::ReadHeader() {
uint64_t box_size = 0;
*err = false;
if (!HasBytes(8)) {
// If EOS is known, then this is an error. If not, additional data may be
// appended later, so this is a soft error.
*err = is_EOS_;
return false;
}
CHECK(Read4Into8(&box_size) && ReadFourCC(&type_));
if (!HasBytes(8))
return is_EOS_ ? ParseResult::kError : ParseResult::kNeedMoreData;
CHECK(Read4Into8(&box_size));
CHECK(ReadFourCC(&type_));
if (box_size == 0) {
if (is_EOS_) {
// All the data bytes are expected to be provided.
box_size = base::checked_cast<uint64_t>(buf_size_);
// TODO(sandersd): The whole |is_EOS_| feature seems to exist just for
// this special case (and is used only for PSSH parsing). Can we get rid
// of it? The caller can treat kNeedMoreData as an error, and the only
// difference would be lack of support for |box_size == 0|.
box_size = base::strict_cast<uint64_t>(buf_size_);
} else {
MEDIA_LOG(DEBUG, media_log_)
<< "ISO BMFF boxes that run to EOS are not supported";
*err = true;
return false;
return ParseResult::kError;
}
} else if (box_size == 1) {
if (!HasBytes(8)) {
// If EOS is known, then this is an error. If not, it's a soft error.
*err = is_EOS_;
return false;
}
if (!HasBytes(8))
return is_EOS_ ? ParseResult::kError : ParseResult::kNeedMoreData;
CHECK(Read8(&box_size));
}
// Implementation-specific: support for boxes larger than 2^31 has been
// removed.
if (box_size < static_cast<uint64_t>(pos_) ||
if (box_size < base::strict_cast<uint64_t>(pos_) ||
box_size > static_cast<uint64_t>(std::numeric_limits<int32_t>::max())) {
*err = true;
return false;
return ParseResult::kError;
}
// Make sure the buffer contains at least the expected number of bytes.
// Since the data may be appended in pieces, this is only an error if EOS.
if (box_size > base::checked_cast<size_t>(buf_size_)) {
*err = is_EOS_;
return false;
}
if (box_size > base::strict_cast<uint64_t>(buf_size_))
return is_EOS_ ? ParseResult::kError : ParseResult::kNeedMoreData;
// Note that the pos_ head has advanced to the byte immediately after the
// header, which is where we want it.
box_size_ = base::checked_cast<size_t>(box_size);
box_size_known_ = true;
return true;
return ParseResult::kOk;
}
} // namespace mp4
......
......@@ -9,6 +9,7 @@
#include <limits>
#include <map>
#include <memory>
#include <vector>
#include "base/compiler_specific.h"
......@@ -17,6 +18,7 @@
#include "media/base/media_export.h"
#include "media/base/media_log.h"
#include "media/formats/mp4/fourccs.h"
#include "media/formats/mp4/parse_result.h"
#include "media/formats/mp4/rcheck.h"
namespace media {
......@@ -93,29 +95,26 @@ class MEDIA_EXPORT BoxReader : public BufferReader {
BoxReader(const BoxReader& other);
~BoxReader();
// Create a BoxReader from a buffer. Note that this function may return NULL
// if an intact, complete box was not available in the buffer. If |*err| is
// set, there was a stream-level error when creating the box; otherwise, NULL
// values are only expected when insufficient data is available.
// Create a BoxReader from a buffer. If the result is kOk, then |out_reader|
// will be set, otherwise |out_reader| will be unchanged.
//
// |buf| is retained but not owned, and must outlive the BoxReader instance.
static BoxReader* ReadTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
bool* err);
// Read the box header from the current buffer. This function returns true if
// there is enough data to read the header and the header is sane; that is, it
// does not check to ensure the entire box is in the buffer before returning
// true. The semantics of |*err| are the same as above.
static ParseResult ReadTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
std::unique_ptr<BoxReader>* out_reader)
WARN_UNUSED_RESULT;
// Read the box header from the current buffer, and return its type and size.
// This function returns kNeedMoreData if the box is incomplete, even if the
// box header is complete.
//
// |buf| is not retained.
static bool StartTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
FourCC* type,
size_t* box_size,
bool* err) WARN_UNUSED_RESULT;
static ParseResult StartTopLevelBox(const uint8_t* buf,
const size_t buf_size,
MediaLog* media_log,
FourCC* out_type,
size_t* out_box_size) WARN_UNUSED_RESULT;
// Create a BoxReader from a buffer. |buf| must be the complete buffer, as
// errors are returned when sufficient data is not available. |buf| can start
......@@ -193,14 +192,8 @@ class MEDIA_EXPORT BoxReader : public BufferReader {
MediaLog* media_log,
bool is_EOS);
// Must be called immediately after init. If the return is false, this
// indicates that the box header and its contents were not available in the
// stream or were nonsensical, and that the box must not be used further. In
// this case, if |*err| is false, the problem was simply a lack of data, and
// should only be an error condition if some higher-level component knows that
// no more data is coming (i.e. EOS or end of containing box). If |*err| is
// true, the error is unrecoverable and the stream should be aborted.
bool ReadHeader(bool* err);
// Must be called immediately after init.
ParseResult ReadHeader() WARN_UNUSED_RESULT;
// Read all children, optionally checking FourCC. Returns true if all
// children are successfully parsed and, if |check_box_type|, have the
......@@ -275,11 +268,10 @@ bool BoxReader::ReadAllChildrenInternal(std::vector<T>* children,
// Must know our box size before attempting to parse child boxes.
RCHECK(box_size_known_);
bool err = false;
while (pos_ < box_size_) {
BoxReader child_reader(&buf_[pos_], box_size_ - pos_, media_log_, is_EOS_);
if (!child_reader.ReadHeader(&err))
if (child_reader.ReadHeader() != ParseResult::kOk)
return false;
T child;
......@@ -289,7 +281,7 @@ bool BoxReader::ReadAllChildrenInternal(std::vector<T>* children,
pos_ += child_reader.box_size();
}
return !err;
return true;
}
} // namespace mp4
......
......@@ -12,6 +12,7 @@
#include "base/logging.h"
#include "media/base/mock_media_log.h"
#include "media/formats/mp4/box_definitions.h"
#include "media/formats/mp4/parse_result.h"
#include "media/formats/mp4/rcheck.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -93,11 +94,11 @@ class BoxReaderTest : public testing::Test {
void TestTopLevelBox(const uint8_t* data, size_t data_size, uint32_t fourCC) {
std::vector<uint8_t> buf(data, data + data_size);
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_FALSE(err);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
EXPECT_EQ(fourCC, reader->type());
EXPECT_EQ(reader->box_size(), data_size);
......@@ -125,10 +126,10 @@ class BoxReaderTest : public testing::Test {
base::IsValueInRangeForNumericType<uint8_t>(buffer_wrapper.size()));
buffer_wrapper[3] = buffer_wrapper.size();
bool err;
std::unique_ptr<BoxReader> reader(BoxReader::ReadTopLevelBox(
&buffer_wrapper[0], buffer_wrapper.size(), &media_log_, &err));
EXPECT_FALSE(err);
std::unique_ptr<BoxReader> reader;
ParseResult result = BoxReader::ReadTopLevelBox(
&buffer_wrapper[0], buffer_wrapper.size(), &media_log_, &reader);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
EXPECT_EQ(FOURCC_EMSG, reader->type());
......@@ -153,11 +154,11 @@ class BoxReaderTest : public testing::Test {
TEST_F(BoxReaderTest, ExpectedOperationTest) {
std::vector<uint8_t> buf = GetBuf();
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
EXPECT_FALSE(err);
EXPECT_TRUE(reader.get());
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
SkipBox box;
EXPECT_TRUE(box.Parse(reader.get()));
......@@ -179,24 +180,25 @@ TEST_F(BoxReaderTest, ExpectedOperationTest) {
TEST_F(BoxReaderTest, OuterTooShortTest) {
std::vector<uint8_t> buf = GetBuf();
bool err;
// Create a soft failure by truncating the outer box.
std::unique_ptr<BoxReader> r(
BoxReader::ReadTopLevelBox(&buf[0], buf.size() - 2, &media_log_, &err));
std::unique_ptr<BoxReader> r;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size() - 2, &media_log_, &r);
EXPECT_FALSE(err);
EXPECT_FALSE(r.get());
EXPECT_EQ(result, ParseResult::kNeedMoreData);
EXPECT_FALSE(r);
}
TEST_F(BoxReaderTest, InnerTooLongTest) {
std::vector<uint8_t> buf = GetBuf();
bool err;
// Make an inner box too big for its outer box.
buf[25] = 1;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_EQ(result, ParseResult::kOk);
SkipBox box;
EXPECT_FALSE(box.Parse(reader.get()));
......@@ -204,7 +206,6 @@ TEST_F(BoxReaderTest, InnerTooLongTest) {
TEST_F(BoxReaderTest, WrongFourCCTest) {
std::vector<uint8_t> buf = GetBuf();
bool err;
// Set an unrecognized top-level FourCC.
buf[4] = 0x44;
......@@ -214,18 +215,20 @@ TEST_F(BoxReaderTest, WrongFourCCTest) {
EXPECT_MEDIA_LOG(HasSubstr("Unrecognized top-level box type DALE"));
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
EXPECT_FALSE(reader.get());
EXPECT_TRUE(err);
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_FALSE(reader);
EXPECT_EQ(result, ParseResult::kError);
}
TEST_F(BoxReaderTest, ScanChildrenTest) {
std::vector<uint8_t> buf = GetBuf();
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader->SkipBytes(16) && reader->ScanChildren());
FreeBox free;
......@@ -246,9 +249,10 @@ TEST_F(BoxReaderTest, ReadAllChildrenTest) {
std::vector<uint8_t> buf = GetBuf();
// Modify buffer to exclude its last 'free' box
buf[3] = 0x38;
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &media_log_, &reader);
EXPECT_EQ(result, ParseResult::kOk);
std::vector<PsshBox> kids;
EXPECT_TRUE(reader->SkipBytes(16) && reader->ReadAllChildren(&kids));
......@@ -304,11 +308,11 @@ TEST_F(BoxReaderTest, NestedBoxWithHugeSize) {
0x00, 0x01, 0x00, 0xff, 0xff, 0x00, 0x3b, 0x03, // random data for rest
0x00, 0x01, 0x00, 0x03, 0x00, 0x03, 0x00, 0x04, 0x05, 0x06, 0x07, 0x08};
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &reader);
EXPECT_FALSE(err);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
EXPECT_EQ(FOURCC_EMSG, reader->type());
EXPECT_FALSE(reader->ScanChildren());
......@@ -329,11 +333,11 @@ TEST_F(BoxReaderTest, ScanChildrenWithInvalidChild) {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &reader);
EXPECT_FALSE(err);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
EXPECT_EQ(FOURCC_EMSG, reader->type());
EXPECT_TRUE(reader->ScanChildren());
......@@ -350,11 +354,11 @@ TEST_F(BoxReaderTest, ReadAllChildrenWithChildLargerThanParent) {
0x00, 0x00, 0x00, 0x10, 'p', 's', 's', 'h', // nested box
};
bool err;
std::unique_ptr<BoxReader> reader(
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &err));
std::unique_ptr<BoxReader> reader;
ParseResult result =
BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &reader);
EXPECT_FALSE(err);
EXPECT_EQ(result, ParseResult::kOk);
EXPECT_TRUE(reader);
EXPECT_EQ(FOURCC_SKIP, reader->type());
......
This diff is collapsed.
......@@ -17,6 +17,7 @@
#include "media/base/media_export.h"
#include "media/base/stream_parser.h"
#include "media/formats/common/offset_byte_queue.h"
#include "media/formats/mp4/parse_result.h"
#include "media/formats/mp4/track_run_iterator.h"
namespace media {
......@@ -52,7 +53,7 @@ class MEDIA_EXPORT MP4StreamParser : public StreamParser {
kError
};
bool ParseBox(bool* err);
ParseResult ParseBox();
bool ParseMoov(mp4::BoxReader* reader);
bool ParseMoof(mp4::BoxReader* reader);
......@@ -75,7 +76,7 @@ class MEDIA_EXPORT MP4StreamParser : public StreamParser {
bool PrepareAACBuffer(const AAC& aac_config,
std::vector<uint8_t>* frame_buf,
std::vector<SubsampleEntry>* subsamples) const;
bool EnqueueSample(BufferQueueMap* buffers, bool* err);
ParseResult EnqueueSample(BufferQueueMap* buffers);
bool SendAndFlushSamples(BufferQueueMap* buffers);
void Reset();
......
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef MEDIA_FORMATS_MP4_PARSE_RESULT_H_
#define MEDIA_FORMATS_MP4_PARSE_RESULT_H_
namespace media {
namespace mp4 {
enum class ParseResult {
kOk, // Parsing was successful.
kError, // The data is invalid (usually unrecoverable).
kNeedMoreData, // More data is required to parse successfully.
};
// Evaluate |expr| once. If the result is not ParseResult::kOk, (early) return
// it from the containing function.
#define RCHECK_OK_PARSE_RESULT(expr) \
do { \
::media::mp4::ParseResult result = (expr); \
if (result == ::media::mp4::ParseResult::kError) \
DLOG(ERROR) << "Failure while parsing MP4: " #expr; \
if (result != ::media::mp4::ParseResult::kOk) \
return result; \
} while (0)
} // namespace mp4
} // namespace media
#endif // MEDIA_FORMATS_MP4_PARSE_RESULT_H_
......@@ -8,15 +8,19 @@
#include "base/logging.h"
#include "media/base/media_log.h"
#define RCHECK_MEDIA_LOGGED(condition, log_cb, msg) \
do { \
if (!(condition)) { \
DLOG(ERROR) << "Failure while parsing MP4: " #condition; \
MEDIA_LOG(ERROR, log_cb) << "Failure parsing MP4: " << (msg); \
return false; \
} \
// Evaluate |condition| once. If the result is false, log |msg| to |media_log|,
// and (early) return false from the containing function.
#define RCHECK_MEDIA_LOGGED(condition, media_log, msg) \
do { \
if (!(condition)) { \
DLOG(ERROR) << "Failure while parsing MP4: " #condition; \
MEDIA_LOG(ERROR, media_log) << "Failure parsing MP4: " << (msg); \
return false; \
} \
} while (0)
// Evaluate |condition| once. If the result is false, (early) return false from
// the containing function.
// TODO(wolenetz,chcunningham): Where appropriate, replace usage of this macro
// in favor of RCHECK_MEDIA_LOGGED. See https://crbug.com/487410.
#define RCHECK(condition) \
......
......@@ -8,6 +8,7 @@
#include <memory>
#include "base/logging.h"
#include "base/macros.h"
#include "media/formats/mp4/box_reader.h"
class NullMediaLog : public media::MediaLog {
......@@ -23,10 +24,12 @@ class NullMediaLog : public media::MediaLog {
// Entry point for LibFuzzer.
extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
bool err;
NullMediaLog media_log;
std::unique_ptr<media::mp4::BoxReader> reader(
media::mp4::BoxReader::ReadTopLevelBox(data, static_cast<int>(size),
&media_log, &err));
return !err && reader && reader->ScanChildren() ? 0 : 0;
std::unique_ptr<media::mp4::BoxReader> reader;
if (media::mp4::BoxReader::ReadTopLevelBox(data, size, &media_log, &reader) ==
media::mp4::ParseResult::kOk) {
CHECK(reader);
ignore_result(reader->ScanChildren());
}
return 0;
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment