VPLAY-12333 mp4demux hardening #830
Conversation
Reason for Change: bounds checks and support for 64 bit size Signed-off-by: Philip Stroffolino <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request adds hardening to the MP4 demuxer by implementing comprehensive bounds checking and support for 64-bit box sizes. The changes aim to prevent buffer overruns and handle malformed or malicious MP4 data more safely.
Changes:
- Added
endPtr,mdatStart, andmdatEndmember variables to track buffer boundaries for validation - Implemented bounds checking in
ReadBytes()to prevent reading beyond buffer limits - Enhanced PSSH box parsing to handle version 1 format with KIDs and improved data size validation
- Strengthened SAIO box parsing with entry count validation and bounds checking
- Added sample data validation in TRUN parsing against mdat range
- Implemented 64-bit box size support in
DemuxHelper()for large boxes - Made sample description box parsing more tolerant of multiple entries
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| mp4demux/MP4Demux.h | Added endPtr, mdatStart, mdatEnd member variables and updated Parse() documentation |
| mp4demux/MP4Demux.cpp | Implemented bounds checking, 64-bit size support, enhanced PSSH/SAIO/TRUN parsing, and mdat range tracking |
Comments suppressed due to low confidence (1)
mp4demux/MP4Demux.cpp:674
- The validation logic at lines 661-674 has inverted semantics. When mdatStart and mdatEnd are both set (lines 661-667), only a warning is logged if dataPtr is outside the range. However, when they are NOT set (lines 668-674), a parse error is raised. This suggests that having mdat tracking is considered mandatory, yet the warning for out-of-range dataPtr should likely be an error instead. Consider making the out-of-range condition an error to maintain consistency with the intent of bounds checking.
// If we tracked an mdat range, validate dataPtr lies within it
if (mdatStart && mdatEnd)
{
if (dataPtr < mdatStart || dataPtr > mdatEnd)
{
MP4_LOG_WARN("TRUN dataPtr outside mdat range");
}
}
else
{
// mandatory field? should never reach here
parseError = MP4_PARSE_ERROR_MISSING_DATA_OFFSET;
MP4_LOG_ERR("Missing data offset in TRUN box");
return;
}
Signed-off-by: Philip Stroffolino <[email protected]>
Signed-off-by: Philip Stroffolino <[email protected]>
| case MultiChar_Constant("mdat"): // Movie Data (under file box) | ||
| // Track mdat payload range for TRUN validation | ||
| mdatStart = ptr; // start of payload (after header bytes) | ||
| mdatEnd = next; // end of payload | ||
| ptr = next; // skip payload | ||
| break; |
There was a problem hiding this comment.
The mdat tracking updates both mdatStart and mdatEnd on every mdat box encountered during parsing. If multiple mdat boxes are present in a segment, only the most recent one's range will be tracked. Consider whether this is the intended behavior or if all mdat ranges should be tracked. Add a comment documenting this behavior if it's intentional.
| ParseProtectionSchemeInfo(); | ||
| SkipBytes(4); // aux_info_type_parameter | ||
| } | ||
| SkipBytes(4); // entry_count | ||
|
|
||
| if( version == 0 ) | ||
| // entry_count | ||
| if (ptr + 4 > endPtr) | ||
| { | ||
| parseError = MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH; | ||
| return; | ||
| } | ||
| uint32_t entryCount = ReadU32(); | ||
| if (entryCount == 0) | ||
| { | ||
| parseError = MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH; | ||
| MP4_LOG_ERR("SAIO entry_count == 0"); | ||
| return; | ||
| } | ||
| // Read the first offset; if multiple, warn and consume others | ||
| if (version == 0) | ||
| { | ||
| auxiliaryInformationOffset = ReadU32(); | ||
| if( parseError == MP4_PARSE_OK ) | ||
| { | ||
| for (uint32_t i = 1; i < entryCount; ++i) | ||
| { | ||
| (void)ReadU32(); | ||
| if( parseError != MP4_PARSE_OK ) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| gotAuxiliaryInformationOffset = true; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| auxiliaryInformationOffset = ReadU64(); | ||
| if( parseError == MP4_PARSE_OK ) | ||
| { | ||
| for (uint32_t i = 1; i < entryCount; ++i) | ||
| { | ||
| (void)ReadU64(); | ||
| if( parseError != MP4_PARSE_OK ) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| gotAuxiliaryInformationOffset = true; | ||
| } | ||
| } | ||
| gotAuxiliaryInformationOffset = true; | ||
| } |
There was a problem hiding this comment.
The SAIO parsing changes that handle multiple entries (entryCount > 1) need test coverage. Add tests for:
- SAIO boxes with entryCount > 1 for both version 0 and version 1
- Boundary conditions where reading all entries would exceed buffer
- Verify that gotAuxiliaryInformationOffset is only set when parseError is MP4_PARSE_OK
- Edge case where entryCount == 0 triggers error
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@pstroffolino I've opened a new pull request, #831, to work on those changes. Once the pull request is ready, I'll request review from you. |
Signed-off-by: Philip Stroffolino <[email protected]>
Signed-off-by: Philip Stroffolino <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
mp4demux/MP4Demux.cpp:775
- The loop in ParseTrackRun calls multiple Read functions (ReadU32, ReadI32) that can fail and set parseError, but the loop continues without checking parseError status between iterations. If a Read operation fails partway through processing samples, the code will continue accessing potentially invalid data and creating malformed samples. Consider adding parseError checks after each Read operation or at the start of each loop iteration to exit early on error.
for (auto i = 0u; i < sampleCount; i++)
{
samples.emplace_back();
// Get reference to newly added sample
AampMediaSample& newSample = samples.back();
uint32_t sampleLen = defaultSampleSize;
uint32_t sampleDuration = defaultSampleDuration;
if (flags & TRUN_SAMPLE_DURATION_PRESENT)
{
sampleDuration = ReadU32();
}
if (flags & TRUN_SAMPLE_SIZE_PRESENT)
{
sampleLen = ReadU32();
}
if (flags & TRUN_SAMPLE_FLAGS_PRESENT)
{ // rarely present?
sampleFlags = ReadU32();
}
int32_t sampleCompositionTimeOffset = 0;
if (flags & TRUN_SAMPLE_COMPOSITION_TIME_OFFSET_PRESENT)
{ // for samples where pts and dts differ (overriding 'trex')
sampleCompositionTimeOffset = ReadI32();
}
// Guard: sample buffer must not overrun endPtr (or mdat)
const uint8_t* hardEnd = mdatEnd?mdatEnd:endPtr;
if ( dataPtr + sampleLen > hardEnd )
{
setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH );
return;
}
newSample.mData.AppendBytes(dataPtr, sampleLen);
dataPtr += sampleLen;
newSample.mDts = dts / (double)timeScale;
newSample.mPts = (dts + sampleCompositionTimeOffset) / (double)timeScale;
newSample.mDuration = sampleDuration / (double)timeScale;
dts += sampleDuration;
}
| uint64_t size = ReadU32(); | ||
| uint32_t type = ReadU32(); |
There was a problem hiding this comment.
After reading the initial size and type (both via ReadU32), the code should check if parseError is MP4_PARSE_OK before proceeding to use these values. If ReadU32 failed due to bounds checking, the size and type values will be 0, but the code continues to process them, potentially leading to incorrect behavior or confusion about the actual error.
| uint64_t size = ReadU32(); | |
| uint32_t type = ReadU32(); | |
| uint64_t size = ReadU32(); | |
| if (parseError != MP4_PARSE_OK) | |
| { | |
| return; | |
| } | |
| uint32_t type = ReadU32(); | |
| if (parseError != MP4_PARSE_OK) | |
| { | |
| return; | |
| } |
|
@pstroffolino I've opened a new pull request, #832, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@pstroffolino I've opened a new pull request, #833, to work on those changes. Once the pull request is ready, I'll request review from you. |
mp4demux/MP4Demux.cpp
Outdated
| uint32_t kidCount = ReadU32(); | ||
| if( kidCount > SIZE_MAX / 16 ) | ||
| { | ||
| setParseError( MP4_PARSE_ERROR_INVALID_KID ); |
There was a problem hiding this comment.
Coverity Issue - Logically dead code
Execution cannot reach this statement: "this->setParseError(MP4_PAR...".
Medium Impact, CWE-561
DEADCODE
mp4demux/MP4Demux.cpp
Outdated
| return; | ||
| } | ||
| uint32_t kidCount = ReadU32(); | ||
| if( kidCount > SIZE_MAX / 16 ) |
There was a problem hiding this comment.
Coverity Issue - Operands don't affect result
"kidCount > 1152921504606846975UL /* 18446744073709551615UL / 16 */" is always false regardless of the values of its operands. This occurs as the logical operand of "if".
Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT
Signed-off-by: Philip Stroffolino <[email protected]>
| TEST(Mp4Demux_Gaps, TrunOverrunDetection) { | ||
| std::vector<uint8_t> buf; | ||
| size_t moofStartIdx; | ||
| { Box moof(buf, "moof"); moofStartIdx = moof.start; | ||
| { Box traf(buf, "traf"); | ||
| { Box tfhd(buf, "tfhd"); writeFullBoxHeader(buf,0, 0x00008 | 0x00010); // default dur+size present | ||
| write32be(buf, 1); write32be(buf, 90000/30); write32be(buf, 10); tfhd.close(); | ||
| } | ||
| { Box tfdt(buf, "tfdt"); writeFullBoxHeader(buf,0,0); write32be(buf,0); tfdt.close(); } | ||
| { Box trun(buf, "trun"); writeFullBoxHeader(buf,0, 0x0001 | 0x0200); // data_offset + sample_size | ||
| write32be(buf, 1); write32be(buf, 0); // placeholder data_offset | ||
| write32be(buf, 12); // sample size larger than payload | ||
| trun.close(); | ||
| } | ||
| traf.close(); | ||
| } | ||
| moof.close(); | ||
| } | ||
| // small mdat | ||
| //size_t mdatHdr = buf.size(); | ||
| write32be(buf, 16); write4cc(buf, "mdat"); // 8 header + 8 payload | ||
| size_t mdatPayload = buf.size(); for (int i=0;i<8;++i) buf.push_back(uint8_t(i)); | ||
| // patch trun data_offset to point into mdat payload | ||
| size_t trunPos=0; for (size_t i=0;i+3<buf.size(); ++i) if (buf[i]=='t'&&buf[i+1]=='r'&&buf[i+2]=='u'&&buf[i+3]=='n'){ trunPos=i-4; break; } | ||
| ASSERT_NE(trunPos, 0u); | ||
| size_t dataOffsetPos = trunPos + 4 + 4 + 4 + 4; | ||
| int32_t dataOffset = int32_t(mdatPayload - moofStartIdx); | ||
| buf[dataOffsetPos+0] = uint8_t((dataOffset>>24)&0xFF); | ||
| buf[dataOffsetPos+1] = uint8_t((dataOffset>>16)&0xFF); | ||
| buf[dataOffsetPos+2] = uint8_t((dataOffset>>8)&0xFF); | ||
| buf[dataOffsetPos+3] = uint8_t((dataOffset>>0)&0xFF); | ||
|
|
||
| Mp4Demux d; | ||
| bool ok = d.Parse(buf.data(), buf.size()); | ||
| EXPECT_FALSE(ok); | ||
| EXPECT_EQ(d.GetLastError(), MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); | ||
| } |
There was a problem hiding this comment.
The test name 'TrunOverrunDetection' expects the parse to fail, but the test implementation appears to validate TRUN overrun detection which is a positive test scenario (detecting an error condition). The test name should more clearly indicate what is being tested, such as 'TrunSampleSizeExceedsMdatPayloadDetected' or 'TrunOverrunCausesParseFailure' to make the test intent clearer.
| void Mp4Demux::ParseTrackEncryptionBox() | ||
| { | ||
| ReadHeader(); | ||
|
|
||
| // Skip: reserved | ||
| ptr++; | ||
| uint8_t pattern = *ptr++; | ||
| if (schemeType == CIPHER_TYPE_CBCS) | ||
| { | ||
| cryptByteBlock = (pattern >> 4) & 0xf; | ||
| skipByteBlock = pattern & 0xf; | ||
| // Be robust to both layouts: | ||
| // 1) Standard CENC tenc (common): [reserved(2)] [isEncrypted(1)] [ivSize(1)] [KID(16)] [v1: constIV] | ||
| // 2) Legacy/variant with a "pattern" byte after one reserved byte (older cbcs flows) | ||
| const uint8_t* save = ptr; | ||
|
|
||
| // Try STANDARD layout first | ||
| bool ok = true; | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| ptr += 2; // reserved(2) | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| uint8_t isEncrypted = *ptr++; | ||
| uint8_t ivSz = *ptr++; | ||
| if (ivSz != 8 && ivSz != 16) { | ||
| ok = false; // fall back to pattern layout below | ||
| } else if (ptr + TENC_BOX_KEY_ID_SIZE > endPtr) { | ||
| setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); | ||
| return; | ||
| } | ||
| codecInfo.mIsEncrypted = *ptr++; | ||
| // This is used to ensure encrypted caps are persisted even if its clear samples | ||
| handledEncryptedSamples = true; | ||
| ivSize = *ptr++; | ||
|
|
||
| defaultKid = std::vector<uint8_t>(ptr, ptr + TENC_BOX_KEY_ID_SIZE); | ||
|
|
||
| if (!ok) { | ||
| // Fallback: one reserved + pattern + isEncrypted + ivSize + KID [ + const IV for v1 ] | ||
| ptr = save; | ||
| if (ptr + 1 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| ptr++; // reserved(1) | ||
| if (ptr + 1 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| uint8_t pattern = *ptr++; | ||
| if (schemeType == CIPHER_TYPE_CBCS) { | ||
| cryptByteBlock = (pattern >> 4) & 0xF; | ||
| skipByteBlock = pattern & 0xF; | ||
| } | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| isEncrypted = *ptr++; | ||
| ivSz = *ptr++; | ||
| if (ivSz != 8 && ivSz != 16) { | ||
| setParseError(MP4_PARSE_ERROR_INVALID_IV_SIZE); // best available error code | ||
| return; | ||
| } | ||
| if (ptr + TENC_BOX_KEY_ID_SIZE > endPtr) {setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| } | ||
|
|
||
| codecInfo.mIsEncrypted = isEncrypted; | ||
| handledEncryptedSamples = true; // keep encrypted caps if any encrypted samples were seen | ||
| ivSize = ivSz; | ||
| defaultKid.assign(ptr, ptr + TENC_BOX_KEY_ID_SIZE); | ||
| ptr += TENC_BOX_KEY_ID_SIZE; | ||
| // Version 1 adds constant IV | ||
| if (version == 1) | ||
| { | ||
|
|
||
| if (version == 1) { | ||
| if (ptr + 1 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| constantIvSize = *ptr++; | ||
| if (constantIvSize != 8 && constantIvSize != 16) | ||
| { | ||
| parseError = MP4_PARSE_ERROR_INVALID_CONSTANT_IV_SIZE; | ||
| MP4_LOG_ERR("Invalid constant IV size: %u, expected 8 or 16", constantIvSize); | ||
| if (constantIvSize != 8 && constantIvSize != 16) { | ||
| setParseError( MP4_PARSE_ERROR_INVALID_IV_SIZE ); | ||
| return; | ||
| } | ||
| constantIv = std::vector<uint8_t>(ptr, ptr + constantIvSize); | ||
| if (ptr + constantIvSize > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH);return; } | ||
| constantIv.assign(ptr, ptr + constantIvSize); | ||
| ptr += constantIvSize; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling the two tenc box layouts is flawed. After trying the standard layout and setting ok=false on line 388, the code unconditionally enters the fallback block on line 394 regardless of whether the standard layout actually succeeded. The condition 'if (!ok)' should guard the entire fallback block, and if the standard layout succeeds (ivSz is 8 or 16 and bounds check passes), the code should skip the fallback and proceed directly to line 415. Currently, even successful standard layout parsing will be overwritten by the fallback attempt.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@pstroffolino I've opened a new pull request, #835, to work on those changes. Once the pull request is ready, I'll request review from you. |
Signed-off-by: Philip Stroffolino <[email protected]>
Signed-off-by: Philip Stroffolino <[email protected]>
| * @return Length value | ||
| */ | ||
| int ReadLen(); | ||
| uint32_t ReadLen(); |
There was a problem hiding this comment.
The ReadLen function's return type changes from int to uint32_t, which is a breaking API change. While this is an improvement (length should be unsigned), any calling code that stores the result in an int variable may experience issues. Ensure all callers have been reviewed and updated appropriately.
| uint32_t ReadLen(); | |
| int ReadLen(); |
| if( mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd) ) | ||
| { | ||
| // mandatory field? should never reach here | ||
| parseError = MP4_PARSE_ERROR_MISSING_DATA_OFFSET; | ||
| MP4_LOG_ERR("Missing data offset in TRUN box"); | ||
| setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic for checking TRUN data offset bounds appears incomplete. The check if( mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd) ) only validates the starting position of the data, but doesn't verify that dataPtr plus the sample length stays within mdat bounds. This validation happens later at line 801, but if the data_offset field itself points outside mdat, the error could be caught earlier and with a more specific message.
| uint32_t Mp4Demux::ReadLen() | ||
| { | ||
| int rc = 0; | ||
| for (;;) | ||
| if( ptr && endPtr ) | ||
| { | ||
| unsigned char octet = *ptr++; | ||
| rc <<= 7; | ||
| rc |= octet & 0x7f; | ||
| if ((octet & 0x80) == 0) return rc; | ||
| uint32_t rc = 0; | ||
| int bits = 0; | ||
| while( ptr<endPtr ) | ||
| { // accumulate variable length integer | ||
| unsigned char octet = *ptr++; | ||
| rc = (rc << 7) + (octet & 0x7f); | ||
| bits += 7; | ||
| if( bits > 32 ) | ||
| { | ||
| setParseError( MP4_PARSE_ERROR_VARIABLE_LENGTH_OVERFLOW ); | ||
| return 0; | ||
| } | ||
| if ((octet & 0x80) == 0) | ||
| { | ||
| return rc; | ||
| } | ||
| } | ||
| } | ||
| setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH ); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
In the new ReadLen() implementation, when returning 0 on error, the function has already called setParseError. However, callers of ReadLen() may not distinguish between a legitimate length value of 0 and an error condition. Consider documenting that callers must check parseError after calling ReadLen(), or ensure all call sites properly handle this case.
mp4demux/MP4Demux.cpp
Outdated
| bool ok = true; | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| ptr += 2; // reserved(2) | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| uint8_t isEncrypted = *ptr++; | ||
| uint8_t ivSz = *ptr++; | ||
| if (ivSz != 8 && ivSz != 16) { | ||
| ok = false; // fall back to pattern layout below | ||
| } else if (ptr + TENC_BOX_KEY_ID_SIZE > endPtr) { | ||
| setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); | ||
| return; | ||
| } | ||
| codecInfo.mIsEncrypted = *ptr++; | ||
| // This is used to ensure encrypted caps are persisted even if its clear samples | ||
| handledEncryptedSamples = true; | ||
| ivSize = *ptr++; | ||
|
|
||
| defaultKid = std::vector<uint8_t>(ptr, ptr + TENC_BOX_KEY_ID_SIZE); | ||
|
|
||
| if (!ok) { |
There was a problem hiding this comment.
The variable name ok is used to track validity but is never explicitly checked after being set to false on line 388. The code relies on the fallback path, but it would be clearer to use a more descriptive name like isStandardLayout or validStandardLayout to make the control flow more explicit.
mp4demux/MP4Demux.cpp
Outdated
| { | ||
| for (int i = 0; i < n; i++) | ||
| { // accumulate bytes | ||
| rc = (rc<<8) + (*ptr++); |
There was a problem hiding this comment.
The accumulation expression rc = (rc<<8) + (*ptr++) changes the original logic from rc = (rc<<8) | (*ptr++). While mathematically equivalent for the intended use case (building a big-endian integer from bytes), using addition instead of bitwise OR is unconventional for byte packing operations and could be confusing to readers familiar with the standard idiom. Consider keeping the bitwise OR for consistency with typical byte-packing patterns.
| rc = (rc<<8) + (*ptr++); | |
| rc = (rc<<8) | (*ptr++); |
| static void write32be(std::vector<uint8_t>& b, uint32_t v) { | ||
| b.push_back((v >> 24) & 0xFF); | ||
| b.push_back((v >> 16) & 0xFF); | ||
| b.push_back((v >> 8) & 0xFF); | ||
| b.push_back((v >> 0) & 0xFF); | ||
| } | ||
|
|
||
| static void write64be(std::vector<uint8_t>& b, uint64_t v) { | ||
| for (int i = 7; i >= 0; --i) b.push_back(uint8_t((v >> (8*i)) & 0xFF)); | ||
| } | ||
| static void write4cc(std::vector<uint8_t>& b, const char t[4]) { | ||
| b.insert(b.end(), t, t+4); | ||
| } | ||
| struct Box { | ||
| std::vector<uint8_t>& buf; size_t start{}; bool extended{}; | ||
| Box(std::vector<uint8_t>& b, const char type[4], bool forceExtended=false) | ||
| : buf(b) { | ||
| start = buf.size(); | ||
| write32be(buf, 0); write4cc(buf, type); | ||
| extended = forceExtended; | ||
| if (extended) { buf[start+3] = 1; write64be(buf, 0); } | ||
| } | ||
| void close() { | ||
| uint64_t total = buf.size() - start; | ||
| if (extended) { | ||
| size_t p = start + 8; | ||
| for (int i = 7; i >= 0; --i) buf[p + (7-i)] = uint8_t((total >> (8*i)) & 0xFF); | ||
| } else { | ||
| uint32_t t32 = static_cast<uint32_t>(total); | ||
| buf[start+0] = (t32 >> 24) & 0xFF; buf[start+1] = (t32 >> 16) & 0xFF; | ||
| buf[start+2] = (t32 >> 8) & 0xFF; buf[start+3] = (t32 >> 0) & 0xFF; | ||
| } | ||
| } | ||
| }; | ||
| static void writeFullBoxHeader(std::vector<uint8_t>& b, uint8_t v, uint32_t f) { | ||
| b.push_back(v); b.push_back(uint8_t((f>>16)&0xFF)); b.push_back(uint8_t((f>>8)&0xFF)); b.push_back(uint8_t(f&0xFF)); | ||
| } | ||
|
|
||
| // helper for 16-bit BE | ||
| static inline void write16be(std::vector<uint8_t>& b, uint16_t v) { | ||
| b.push_back(uint8_t((v>>8)&0xFF)); | ||
| b.push_back(uint8_t(v&0xFF)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The helper functions write32be, write64be, write4cc, write16be, and the Box struct are defined with internal linkage but lack static keyword or anonymous namespace. According to C++ best practices and the C++ Core Guidelines (C.42), file-scope helper functions should be in an anonymous namespace or marked static to prevent external linkage and potential ODR violations.
Signed-off-by: Philip Stroffolino <[email protected]>
Signed-off-by: Philip Stroffolino <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
mp4demux/MP4Demux.cpp:1365
- The default case in the DemuxHelper switch statement at line 1363 contains only a break, which means unknown box types are silently skipped without advancing ptr. This will cause the ptr != next check at line 1370 to fail and set MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH. While this might be intentional to catch unhandled boxes, it means that any future box types added to the MP4 spec will cause parse failures unless explicitly added to the switch. Consider whether unknown boxes should be handled more gracefully (e.g., by advancing ptr = next in the default case) to allow forward compatibility.
default:
break;
}
mp4demux/MP4Demux.cpp
Outdated
| const uint64_t remaining = static_cast<uint64_t>(fin - ptr); | ||
| if( size > remaining ) | ||
| { | ||
| parseError = MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH; |
There was a problem hiding this comment.
The line 1203 assigns directly to parseError instead of calling setParseError(). This is inconsistent with the rest of the code which uses setParseError() for logging and error state management. This direct assignment bypasses the logging provided by setParseError().
| parseError = MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH; | |
| setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH ); |
| ReadHeader(); | ||
|
|
||
| // Skip: reserved | ||
| ptr++; | ||
| uint8_t pattern = *ptr++; | ||
| if (schemeType == CIPHER_TYPE_CBCS) | ||
| { | ||
| cryptByteBlock = (pattern >> 4) & 0xf; | ||
| skipByteBlock = pattern & 0xf; | ||
|
|
||
| // Two layouts: | ||
| // 1) Standard CENC tenc (common): [reserved(2)] [isEncrypted(1)] [ivSize(1)] [KID(16)] [v1: constIV] - supported | ||
| // 2) Legacy/variant with a "pattern" byte after one reserved byte (older cbcs flows) - not supported | ||
|
|
||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| ptr += 2; // reserved(2) | ||
| if (ptr + 2 > endPtr) { setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); return; } | ||
| uint8_t isEncrypted = *ptr++; | ||
| uint8_t ivSz = *ptr++; | ||
| if (ivSz != 8 && ivSz != 16) { | ||
| setParseError(MP4_PARSE_ERROR_INVALID_IV_SIZE); | ||
| return; | ||
| } else if (ptr + TENC_BOX_KEY_ID_SIZE > endPtr) { | ||
| setParseError(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH); | ||
| return; | ||
| } |
There was a problem hiding this comment.
In the ParseTrackEncryptionBox function, the comment on lines 376-378 mentions that a legacy/variant pattern with a "pattern" byte is "not supported", but the old code on lines being removed actually handled the pattern byte for CBCS cipher. The new implementation removes this handling entirely without setting an error. If this pattern is truly unsupported and might be encountered, the function should detect it and set an appropriate parse error rather than silently proceeding with potentially incorrect parsing.
| if( mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd) ) | ||
| { | ||
| // mandatory field? should never reach here | ||
| parseError = MP4_PARSE_ERROR_MISSING_DATA_OFFSET; | ||
| MP4_LOG_ERR("Missing data offset in TRUN box"); | ||
| setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The bounds check condition at line 750 only validates dataPtr against mdat boundaries if mdatStart is set, but doesn't handle the case where mdatStart is nullptr. If mdatStart is nullptr (which happens before any mdat box is parsed), this check is skipped entirely. This could allow dataPtr to point to arbitrary memory before an mdat box has been encountered. Consider whether validation is needed in the mdatStart==nullptr case, or document why it's safe to skip.
| 0x00, 0x00, 0x00, 0x02, // sample_count = 2 | ||
| 0x00, 0x00, 0x02, 0xD5, // data_offset = 725 (from start of moof: 555 moov + 170 moof + 8 mdat header - 8 moof header) | ||
|
|
||
| 0x00,0x00,0x00,0xB2, // data_offset = 178 (from start of moof) |
There was a problem hiding this comment.
The data_offset value in the test data was changed from 0x2D5 (725) to 0xB2 (178). The comment explains this is "from start of moof" but the old comment mentioned it was calculated from moov + moof + mdat header - moof header. This appears to be a test data correction, but there's no explanation in the PR description about why the original value was incorrect. Consider adding a comment explaining why this specific value is now correct to aid future maintenance.
| 0x00,0x00,0x00,0xB2, // data_offset = 178 (from start of moof) | |
| 0x00,0x00,0x00,0xB2, // data_offset = 178 bytes from start of moof. | |
| // In this synthetic test fragment the moof is followed | |
| // immediately by an mdat; 0xB2 is the byte offset from | |
| // the beginning of the moof box to the first media | |
| // sample in that mdat. The previous value 0x2D5 was | |
| // computed using a full-file layout (moov + moof + | |
| // mdat header - moof header) and did not match the | |
| // isolated moof/mdat structure used in these tests. |
| { | ||
| uint32_t size = ReadU32(); | ||
| const uint8_t *next = ptr + size - 4; | ||
| uint64_t size = ReadU32(); |
There was a problem hiding this comment.
The check for parseError in ReadU32() is missing. After calling ReadU32() to read size and type, if parseError is set by ReadBytes(), the subsequent logic should not proceed. While there is a check after ReadU32() for type at line 1172, there is no check after the first ReadU32() for size at line 1170.
| uint64_t size = ReadU32(); | |
| uint64_t size = ReadU32(); | |
| if( parseError != MP4_PARSE_OK ) return; |
| // Guard: sample buffer must not overrun endPtr (or mdat) | ||
| const uint8_t* hardEnd = mdatEnd ? mdatEnd : endPtr; | ||
| if ( dataPtr + sampleLen > hardEnd ) | ||
| { | ||
| setParseError( MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The condition at line 786 uses a ternary operator to choose between mdatEnd and endPtr as the hard boundary. However, if mdatEnd is set but less than endPtr (which should always be the case for a valid mdat box within the buffer), using mdatEnd provides tighter bounds checking. But if mdatEnd is somehow greater than endPtr due to a malformed box, this could allow reading beyond the buffer. Consider adding an assertion or check that mdatEnd <= endPtr when mdatEnd is set to catch malformed state early.
Signed-off-by: Philip Stroffolino <[email protected]>
Signed-off-by: Philip Stroffolino <[email protected]>
| + (static_cast<uint32_t>(TEXT[1]) << 0x10) \ | ||
| + (static_cast<uint32_t>(TEXT[2]) << 0x08) \ | ||
| + (static_cast<uint32_t>(TEXT[3]) << 0x00) ) |
There was a problem hiding this comment.
The MultiChar_Constant macro has been changed from bitwise OR (|) to addition (+). While mathematically equivalent for non-overlapping bit fields, the OR operator is more semantically appropriate for combining independent bit positions and is the conventional pattern for this type of FourCC construction. Consider reverting to the OR operator for consistency with common practice.
| + (static_cast<uint32_t>(TEXT[1]) << 0x10) \ | |
| + (static_cast<uint32_t>(TEXT[2]) << 0x08) \ | |
| + (static_cast<uint32_t>(TEXT[3]) << 0x00) ) | |
| | (static_cast<uint32_t>(TEXT[1]) << 0x10) \ | |
| | (static_cast<uint32_t>(TEXT[2]) << 0x08) \ | |
| | (static_cast<uint32_t>(TEXT[3]) << 0x00) ) |
| class Mp4ParseException : public std::runtime_error | ||
| { | ||
| public: | ||
| explicit Mp4ParseException(Mp4ParseError code, const char* what) | ||
| : std::runtime_error(what), code_(code) {} | ||
| Mp4ParseError code() const noexcept { return code_; } | ||
| private: | ||
| Mp4ParseError code_; | ||
| }; |
There was a problem hiding this comment.
The Mp4ParseException class should follow modern C++ RAII and exception best practices. The 'what' string is passed as a const char* which may point to temporary storage. Consider using std::string for the 'what' parameter to ensure the message is safely stored, or document that the caller must ensure the string lifetime exceeds the exception lifetime.
| #if SIZE_MAX <= 0xffffffff | ||
| // if size_t is 32-bit or smaller, perform overflow check | ||
| if( kidCount > SIZE_MAX / 16 ) { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_INVALID_KID, "pssh[v1]: KID count overflow"); | ||
| } | ||
| #endif | ||
| size_t kidBytes = 16*static_cast<size_t>(kidCount); | ||
| if (ptr + kidBytes > next) { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH, "pssh[v1]: KIDs OOB"); | ||
| } | ||
| // Optional: store KIDs in psshData if your MediaProtectionInfo supports it | ||
| ptr += kidBytes; |
There was a problem hiding this comment.
The SIZE_MAX conditional compilation check is good defensive programming. However, on modern 64-bit systems, the else case (no overflow check) allows kidCount values up to UINT32_MAX which could still cause issues if kidCount * 16 exceeds available memory. Consider adding a sanity check even on 64-bit platforms, such as limiting kidCount to a reasonable maximum (e.g., 1000 or 10000 KIDs).
| dataPtr += dataOffset; | ||
| } | ||
| else | ||
| if( mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd) ) |
There was a problem hiding this comment.
The TRUN parsing validation checks if dataPtr is within mdat bounds, but this check only occurs if mdatStart is non-null. If an MP4 stream has a TRUN box without a preceding MDAT box (malformed but possible), dataPtr will not be validated against endPtr. Consider adding a fallback check against endPtr when mdatStart is null to ensure dataPtr is always within valid bounds.
| if( mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd) ) | |
| if( (mdatStart && (dataPtr < mdatStart || dataPtr >= mdatEnd)) || | |
| (!mdatStart && (dataPtr >= endPtr)) ) |
| uint32_t Mp4Demux::ReadLen() | ||
| { | ||
| int rc = 0; | ||
| for (;;) | ||
| { | ||
| if (!ptr || !endPtr) { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH, "ReadLen: null bounds"); | ||
| } | ||
| uint32_t rc = 0; | ||
| int bits = 0; | ||
| while (ptr < endPtr) { | ||
| unsigned char octet = *ptr++; | ||
| rc <<= 7; | ||
| rc |= octet & 0x7f; | ||
| if ((octet & 0x80) == 0) return rc; | ||
| bits += 7; | ||
| if (bits > 32) { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_VARIABLE_LENGTH_OVERFLOW, "ReadLen: overflow >32 bits"); | ||
| } | ||
| rc = (rc << 7) + (octet & 0x7f); | ||
| if ((octet & 0x80) == 0) { | ||
| return rc; | ||
| } | ||
| } | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH, "ReadLen: unterminated var int"); | ||
| } |
There was a problem hiding this comment.
The ReadLen function correctly implements overflow detection for variable-length integers. However, the overflow check happens after incrementing 'bits' by 7 each iteration. For clarity and to avoid potential edge cases, consider checking if adding 7 more bits would exceed 32 before updating 'rc', or restructure to check 'bits >= 32' before the shift-and-add operation.
| EXPECT_NEAR(samples[0].mPts, 0.0, 1e-6) << "Sample 0 PTS should be 0"; | ||
| EXPECT_NEAR(samples[0].mDts, 0.0, 1e-6) << "Sample 0 DTS should be 0"; | ||
| EXPECT_NEAR(samples[0].mDuration, 0.1, 1e-6) << "Sample 0 duration should be 0.1"; | ||
| EXPECT_FALSE(samples[0].mDrmMetadata.mIsEncrypted) << "Sample 0 should not be encrypted"; | ||
|
|
||
| // Validate Sample 1 | ||
| EXPECT_EQ(samples[1].mData.GetLen(), 64) << "Sample 1 should be 64 bytes"; | ||
| EXPECT_EQ(samples[1].mPts, 0.1) << "Sample 1 PTS should be 0.1"; | ||
| EXPECT_EQ(samples[1].mDts, 0.1) << "Sample 1 DTS should be 0.1"; | ||
| EXPECT_EQ(samples[1].mDuration, 0.1) << "Sample 1 duration should be 0.1"; | ||
| EXPECT_NEAR(samples[1].mPts, 0.1, 1e-6) << "Sample 1 PTS should be 0.1"; | ||
| EXPECT_NEAR(samples[1].mDts, 0.1, 1e-6) << "Sample 1 DTS should be 0.1"; | ||
| EXPECT_NEAR(samples[1].mDuration, 0.1, 1e-6) << "Sample 1 duration should be 0.1"; |
There was a problem hiding this comment.
The test is checking floating-point equality with EXPECT_NEAR using a tolerance of 1e-6. While this is an improvement over exact equality checks (EXPECT_EQ), consider whether this tolerance is appropriate for the timescale being used (30000). For values like 0.1 seconds, the actual representation at this timescale would be 3000/30000, which is exactly representable. Document why EXPECT_NEAR is needed here or use EXPECT_DOUBLE_EQ if exact equality is expected after the division.
| if( size==1 ) | ||
| { // format: size(4)+type(4)+large_size(8)+payload | ||
| size = ReadU64(); | ||
| if ( size < 16) | ||
| { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_INVALID_BOX, "box: large_size < header"); | ||
| } | ||
| size -= 16; | ||
| } | ||
| else if( size>=8 ) | ||
| { // format: size(4)+type(4)+payload | ||
| size -= 8; | ||
| } | ||
| else | ||
| { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_INVALID_BOX, "box: size < 8"); | ||
| } | ||
| const uint64_t remaining = static_cast<uint64_t>(fin - ptr); | ||
| if( size > remaining ) | ||
| { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_DATA_BOUNDARY_MISMATCH, "box: payload > remaining"); | ||
| } | ||
| next = ptr + size; |
There was a problem hiding this comment.
In the extended box size handling (size==1), the code subtracts 16 from the 64-bit largesize to get the payload size. However, there's no check to ensure that size is castable to size_t before the addition 'ptr + size'. On 32-bit systems where size_t is 32 bits, a 64-bit box size larger than SIZE_MAX would cause the pointer arithmetic to overflow. Consider adding a check: if size > SIZE_MAX, throw an appropriate exception.
| // Map unknown std exceptions to a generic parse error | ||
| setParseError(MP4_PARSE_ERROR_INVALID_BOX); |
There was a problem hiding this comment.
The generic catch block for std::exception maps all unknown exceptions to MP4_PARSE_ERROR_INVALID_BOX, which may be misleading. For example, a std::bad_alloc would be mapped to INVALID_BOX rather than an out-of-memory error. Consider either adding specific handlers for common exceptions (bad_alloc, etc.) or using a more generic error code like MP4_PARSE_ERROR_INTERNAL for unexpected exceptions.
| // Map unknown std exceptions to a generic parse error | |
| setParseError(MP4_PARSE_ERROR_INVALID_BOX); | |
| // Map unknown std exceptions to a generic internal parse error | |
| setParseError(MP4_PARSE_ERROR_INTERNAL); |
| struct Box { | ||
| std::vector<uint8_t>& buf; size_t start{}; bool extended{}; | ||
| Box(std::vector<uint8_t>& b, const char type[4], bool forceExtended=false) | ||
| : buf(b) { | ||
| start = buf.size(); | ||
| write32be(buf, 0); write4cc(buf, type); | ||
| extended = forceExtended; | ||
| if (extended) { buf[start+3] = 1; write64be(buf, 0); } | ||
| } |
There was a problem hiding this comment.
The Box helper struct uses a boolean 'extended' flag to track whether 64-bit size is used, but initializes it in the constructor based on 'forceExtended' and then conditionally modifies buf[start+3] to set size to 1. This approach modifies already-written bytes. A cleaner approach would be to write the correct size value initially rather than writing 0 and then modifying. Consider refactoring to write the correct initial size field based on the extended flag.
Signed-off-by: Philip Stroffolino <[email protected]>
| */ | ||
| void Mp4Demux::ReadHeader() | ||
| { | ||
| if (!ptr || !endPtr || ptr + 1 > endPtr) |
There was a problem hiding this comment.
Replace hardcoded numbers with macro for better readability?
| */ | ||
| void Mp4Demux::SkipBytes(size_t len) | ||
| { | ||
| if (!ptr || !endPtr || ptr + len > endPtr) { |
There was a problem hiding this comment.
I think the boundary checks can be replaced with a macro, would look clean
This seems to be a common logic with len and the errorMsg being the variables
There was a problem hiding this comment.
fine idea, but can defer
Signed-off-by: Philip Stroffolino <[email protected]>
| static std::vector<uint8_t> encode_len(uint32_t value) | ||
| { | ||
| std::vector<uint8_t> out; | ||
| // Collect 7-bit groups in reverse | ||
| uint32_t v = value; | ||
| std::vector<uint8_t> groups; | ||
| do { | ||
| groups.push_back(static_cast<uint8_t>(v & 0x7F)); | ||
| v >>= 7; | ||
| } while (v != 0); | ||
| // Emit big-endian with continuation bits | ||
| for (size_t i = groups.size(); i-- > 0; ) | ||
| { | ||
| uint8_t b = groups[i]; | ||
| if (i != 0) b |= 0x80; // set continuation for all but last | ||
| out.push_back(b); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
The encode_len function could benefit from input validation. If value is 0, it will create a vector with a single element 0x00, which is correct. However, for large values that require more than 4 continuation bytes, this could create varint encodings longer than what Mp4Demux::ReadLen() can handle (which now enforces a 32-bit limit and throws on overflow). Consider adding a check and comment about the maximum value that can be encoded (2^28 - 1 for 4-byte encoding).
| MP4_PARSE_ERROR_INVALID_KID, /**< Invalid (huge) kidCount */ | ||
| MP4_PARSE_ERROR_INVALID_ENTRY_COUNT, /**< Entry count is zero */ | ||
| MP4_PARSE_ERROR_VARIABLE_LENGTH_OVERFLOW, /**< Value encoded using octets exceed 32 bits */ | ||
| MP4_PARSE_ERROR_UNEXPECTED_IS_ENCRYPTED_FIELD /**< is_encrypted not 0 or 1 */ |
There was a problem hiding this comment.
The error code MP4_PARSE_ERROR_MISSING_DATA_OFFSET has been removed from the enum but this appears to be intentional as the code no longer treats missing data_offset as an error (the check has been removed from ParseTrackRun). However, this is a breaking change to the public API enum that could affect external code checking for this specific error. Consider documenting this breaking change or adding a deprecation notice if backward compatibility is a concern.
| MP4_PARSE_ERROR_UNEXPECTED_IS_ENCRYPTED_FIELD /**< is_encrypted not 0 or 1 */ | |
| MP4_PARSE_ERROR_UNEXPECTED_IS_ENCRYPTED_FIELD, /**< is_encrypted not 0 or 1 */ | |
| MP4_PARSE_ERROR_MISSING_DATA_OFFSET /**< Deprecated: maintained for backward compatibility; no longer used */ |
| /** | ||
| * @brief Read length field with variable encoding | ||
| /** @brief Read length field with variable encoding | ||
| * @return Length value |
There was a problem hiding this comment.
The ReadLen() method return type has been changed from int to uint32_t. This is a good improvement for type safety since lengths should never be negative. However, this is a breaking API change for any code that may have assigned the result to an int variable. Ensure this change is documented in release notes.
| * @return Length value | |
| * | |
| * This method reads a length field that is encoded using a variable-length | |
| * scheme. The returned value is always non-negative and represented as | |
| * an unsigned 32-bit integer. | |
| * | |
| * @return Length value as uint32_t | |
| * | |
| * @note The return type of this method was changed from int to uint32_t | |
| * to improve type safety (lengths are never negative). Callers | |
| * should ensure they do not rely on negative values and avoid | |
| * unintended narrowing conversions to signed types. |
mp4demux/MP4Demux.cpp
Outdated
| { | ||
| constantIvSize = *ptr++; | ||
| { // Version 1 adds constant IV | ||
| constantIvSize = static_cast<uint8_t>(ReadBytes(1));; |
There was a problem hiding this comment.
The double semicolon is a syntax error. Remove one semicolon at the end of this line.
| { // rarely present? | ||
| sampleFlags = ReadU32(); | ||
| (void)sampleFlags; |
There was a problem hiding this comment.
The sampleFlags variable is read but marked with (void) to suppress unused variable warnings on lines 687 and 708. If sampleFlags is genuinely unused, consider removing the read entirely rather than reading and voiding it. If it will be used in future code, add a TODO comment explaining the intended usage. The current pattern of reading then voiding is unclear about the intent.
| { // rarely present? | |
| sampleFlags = ReadU32(); | |
| (void)sampleFlags; | |
| { // rarely present? currently ignored, but must advance stream | |
| ReadU32(); |
|
|
||
| 0x00,0x00,0x00,0xB2, // data_offset = 178 (from start of moof) | ||
|
|
There was a problem hiding this comment.
There are unnecessary blank lines added (lines 801 and 803) around the data_offset bytes. These should be removed for consistency with the surrounding test data formatting, where hex bytes are listed consecutively without blank line separators.
| 0x00,0x00,0x00,0xB2, // data_offset = 178 (from start of moof) | |
| 0x00,0x00,0x00,0xB2, // data_offset = 178 (from start of moof) |
| #if SIZE_MAX <= 0xffffffff | ||
| // if size_t is 32-bit or smaller, perform overflow check | ||
| if( kidCount > SIZE_MAX / 16 ) { | ||
| throw Mp4ParseException(MP4_PARSE_ERROR_INVALID_KID, "pssh[v1]: KID count overflow"); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The SIZE_MAX check for overflow protection is only needed on 32-bit platforms, but the conditional compilation guard checks if SIZE_MAX is 32-bit or smaller. However, SIZE_MAX is always defined as the maximum value for size_t, so this check would be more accurately expressed as checking the size of size_t directly: #if UINTPTR_MAX <= 0xffffffff or #if SIZE_MAX <= 0xffffffff. The current condition is correct but the comment should clarify that this protects against overflow when kidCount*16 exceeds SIZE_MAX on platforms where size_t is 32-bit.
Signed-off-by: Philip Stroffolino <[email protected]>
VPLAY-12333 mp4demux hardening
Reason for Change: bounds checks and support for 64 bit size
Test Guidance: general regression testing with mp4demux enabled and l1 tests
Risk: Low