diff --git a/mkvparser.cpp b/mkvparser.cpp index 46dfcde..6132d09 100644 --- a/mkvparser.cpp +++ b/mkvparser.cpp @@ -829,6 +829,11 @@ long long Segment::ParseHeaders() { long long pos = m_pos; const long long element_start = pos; + // Avoid rolling over pos when very close to LONG_LONG_MAX. + unsigned long long rollover_check = pos + 1ULL; + if (rollover_check > LONG_LONG_MAX) + return E_FILE_FORMAT_INVALID; + if ((pos + 1) > available) return (pos + 1); @@ -838,8 +843,10 @@ long long Segment::ParseHeaders() { if (result < 0) // error return result; - if (result > 0) // underflow (weird) + if (result > 0) { + // MkvReader doesn't have enough data to satisfy this read attempt. return (pos + 1); + } if ((segment_stop >= 0) && ((pos + len) > segment_stop)) return E_FILE_FORMAT_INVALID; @@ -850,8 +857,13 @@ long long Segment::ParseHeaders() { const long long idpos = pos; const long long id = ReadUInt(m_pReader, idpos, len); - if (id < 0) // error + if (id < 0 || len < 1 || len > 4) { + // TODO(tomfinegan): A helper that is intended for reading IDs would be + // handy; even better if its aware of the max ID size from the EBML + // header. It could easily enforce the rules here, instead of duping this + // check everytime an ID is read. return id; + } if (id == 0x0F43B675) // Cluster ID break; @@ -867,8 +879,10 @@ long long Segment::ParseHeaders() { if (result < 0) // error return result; - if (result > 0) // underflow (weird) + if (result > 0) { + // MkvReader doesn't have enough data to satisfy this read attempt. return (pos + 1); + } if ((segment_stop >= 0) && ((pos + len) > segment_stop)) return E_FILE_FORMAT_INVALID; @@ -878,11 +892,19 @@ long long Segment::ParseHeaders() { const long long size = ReadUInt(m_pReader, pos, len); - if (size < 0) // error + if (size < 0 || len < 1 || len > 8) { + // TODO(tomfinegan): ReadUInt should return an error when len is < 1 or + // len > 8 is true instead of checking this _everywhere_. return size; + } pos += len; // consume length of size of element + // Avoid rolling over pos when very close to LONG_LONG_MAX. + rollover_check = static_cast(pos) + size; + if (rollover_check > LONG_LONG_MAX) + return E_FILE_FORMAT_INVALID; + const long long element_size = size + pos - element_start; // Pos now points to start of payload