From edaad04f9a83c15d1348e19539c6bc91c3dd3c1f Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Sun, 1 Mar 2026 22:15:47 +0000 Subject: [PATCH 1/8] Avoid leaking memory if |new MP4BytesProperty()| throws --- src/mp4atom.cpp | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/mp4atom.cpp b/src/mp4atom.cpp index 0945748..04059fc 100644 --- a/src/mp4atom.cpp +++ b/src/mp4atom.cpp @@ -173,32 +173,32 @@ MP4Atom* MP4Atom::ReadAtom(MP4File& file, MP4Atom* pParentAtom) } MP4Atom* pAtom = CreateAtom(file, pParentAtom, type); - // pAtom->SetFile(pFile); - pAtom->SetStart(pos); - pAtom->SetEnd(pos + hdrSize + dataSize); - pAtom->SetLargesizeMode(largesizeMode); - pAtom->SetSize(dataSize); - if (ATOMID(type) == ATOMID("uuid")) { - pAtom->SetExtendedType(extendedType); - } - if (pAtom->IsUnknownType()) { - if (!IsReasonableType(pAtom->GetType())) { - log.warningf("%s: \"%s\": atom type %s is suspect", __FUNCTION__, file.GetFilename().c_str(), - pAtom->GetType()); - } else { - log.verbose1f("\"%s\": Info: atom type %s is unknown", file.GetFilename().c_str(), - pAtom->GetType()); + try { + // pAtom->SetFile(pFile); + pAtom->SetStart(pos); + pAtom->SetEnd(pos + hdrSize + dataSize); + pAtom->SetLargesizeMode(largesizeMode); + pAtom->SetSize(dataSize); + if (ATOMID(type) == ATOMID("uuid")) { + pAtom->SetExtendedType(extendedType); } + if (pAtom->IsUnknownType()) { + if (!IsReasonableType(pAtom->GetType())) { + log.warningf("%s: \"%s\": atom type %s is suspect", __FUNCTION__, file.GetFilename().c_str(), + pAtom->GetType()); + } else { + log.verbose1f("\"%s\": Info: atom type %s is unknown", file.GetFilename().c_str(), + pAtom->GetType()); + } - if (dataSize > 0) { - pAtom->AddProperty( - new MP4BytesProperty(*pAtom, "data", dataSize)); + if (dataSize > 0) { + pAtom->AddProperty( + new MP4BytesProperty(*pAtom, "data", dataSize)); + } } - } - pAtom->SetParentAtom(pParentAtom); + pAtom->SetParentAtom(pParentAtom); - try { pAtom->Read(); } catch (Exception* x) { From c5d62c94331ebef1a077549f94fff0fe30e02f63 Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Sun, 1 Mar 2026 22:35:18 +0000 Subject: [PATCH 2/8] Fix an OOM and stack overflow See comment for why this change is necessary. --- src/mp4atom.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/mp4atom.cpp b/src/mp4atom.cpp index 04059fc..542e6d0 100644 --- a/src/mp4atom.cpp +++ b/src/mp4atom.cpp @@ -155,7 +155,10 @@ MP4Atom* MP4Atom::ReadAtom(MP4File& file, MP4Atom* pParentAtom) log.verbose1f("\"%s\": type = \"%s\" data-size = %" PRIu64 " (0x%" PRIx64 ") hdr %u", file.GetFilename().c_str(), type, dataSize, dataSize, hdrSize); - if (pos + hdrSize + dataSize > pParentAtom->GetEnd()) { + // |dataSize + hdrSize| is what |dataSize| was when it was read. + // Rather than adding |pos| to it (which can overflow), compare it against how much is left in the file, + // otherwise the |SetEnd()| call below can jump backwards and result in an infinite loop of parsing the same atom. + if (dataSize + hdrSize > pParentAtom->GetEnd() - pos) { log.errorf("%s: \"%s\": invalid atom size, extends outside parent atom - skipping to end of \"%s\" \"%s\" %" PRIu64 " vs %" PRIu64, __FUNCTION__, file.GetFilename().c_str(), pParentAtom->GetType(), type, pos + hdrSize + dataSize, From 1accd5d7f78adc494543519a908bc67db0d7aa49 Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Sun, 1 Mar 2026 22:40:19 +0000 Subject: [PATCH 3/8] Fix crash on nullptr MP4File::GetTrackH264SeqPictHeaders() can return early if it fails to parse or allocate, which means that MP4GetTrackH264SeqPictHeaders() can return success and look like the pointers need freeing even though some may still be nullptrs. To fix this, guard the derefs. --- src/mp4.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/mp4.cpp b/src/mp4.cpp index 626a802..10a3d0f 100644 --- a/src/mp4.cpp +++ b/src/mp4.cpp @@ -2567,14 +2567,18 @@ MP4FileHandle MP4ReadProvider( const char* fileName, const MP4FileProvider* file { uint32_t ix; - for (ix = 0; pSeqHeaderSize[ix] != 0; ++ix) { - free(pSeqHeaders[ix]); + if (pSeqHeaderSize) { + for (ix = 0; pSeqHeaderSize[ix] != 0; ++ix) { + free(pSeqHeaders[ix]); + } } free(pSeqHeaders); free(pSeqHeaderSize); - for (ix = 0; pPictHeaderSize[ix] != 0; ++ix) { - free(pPictHeader[ix]); + if (pPictHeaderSize) { + for (ix = 0; pPictHeaderSize[ix] != 0; ++ix) { + free(pPictHeader[ix]); + } } free(pPictHeader); free(pPictHeaderSize); From 9aacbe74da2ea13067fa6f289d72f6a667e9b5c5 Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Sun, 1 Mar 2026 22:43:48 +0000 Subject: [PATCH 4/8] Fix read/write out of bounds due to off-by-one error We need an extra byte for the terminating NUL, otherwise the strncat() can write past the end or the caller can read past the end of the string. --- src/mp4info.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mp4info.cpp b/src/mp4info.cpp index 676a5be..ac1ed41 100644 --- a/src/mp4info.cpp +++ b/src/mp4info.cpp @@ -577,7 +577,7 @@ char* MP4Info( try { if (trackId == MP4_INVALID_TRACK_ID) { uint32_t buflen = 4 * 1024; - info = (char*)MP4Calloc(buflen); + info = (char*)MP4Calloc(buflen + 1); buflen -= snprintf(info, buflen, "Track\tType\tInfo\n"); From e93427b27336a7cff495592578d7e10311f8d7fb Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Mon, 2 Mar 2026 19:18:14 +0000 Subject: [PATCH 5/8] Bounds check tag sizes before attempting to read them ASAN flags up that these read out of bounds on bad data. --- src/itmf/Tags.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/itmf/Tags.cpp b/src/itmf/Tags.cpp index e8ca481..c2e90d8 100644 --- a/src/itmf/Tags.cpp +++ b/src/itmf/Tags.cpp @@ -420,7 +420,7 @@ Tags::fetchGenre( const CodeItemMap& cim, uint16_t& cpp, const uint16_t*& c ) return; MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 2 ) return; cpp = (uint16_t(data.value[0]) << 8) @@ -444,7 +444,7 @@ Tags::fetchDisk( const CodeItemMap& cim, MP4TagDisk& cpp, const MP4TagDisk*& c ) MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 6 ) return; cpp.index = (uint16_t(data.value[2]) << 8) @@ -471,7 +471,7 @@ Tags::fetchTrack( const CodeItemMap& cim, MP4TagTrack& cpp, const MP4TagTrack*& MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 6 ) return; cpp.index = (uint16_t(data.value[2]) << 8) @@ -517,7 +517,7 @@ Tags::fetchInteger( const CodeItemMap& cim, const string& code, uint16_t& cpp, c MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 2 ) return; cpp = (uint16_t(data.value[0]) << 8) @@ -540,7 +540,7 @@ Tags::fetchInteger( const CodeItemMap& cim, const string& code, uint32_t& cpp, c MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 4 ) return; cpp = (uint32_t(data.value[0]) << 24) @@ -565,7 +565,7 @@ Tags::fetchInteger( const CodeItemMap& cim, const string& code, uint64_t& cpp, c MP4ItmfData& data = f->second->dataList.elements[0]; - if( NULL == data.value ) + if( NULL == data.value || data.valueSize < 8 ) return; cpp = (uint64_t(data.value[0]) << 56) From 7f5d4ed0459937e1c20d5a888b5136f10f21446f Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Mon, 2 Mar 2026 19:26:44 +0000 Subject: [PATCH 6/8] Plug a leak if an exception is thrown MP4ConvertTime() can throw on bad data which ends up leaking |chapters| since it's not owned by anything. Add some RAII to plug that leak. --- src/mp4file.cpp | 8 ++++---- src/mp4util.h | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/mp4file.cpp b/src/mp4file.cpp index cd4ba83..6aef5be 100644 --- a/src/mp4file.cpp +++ b/src/mp4file.cpp @@ -2562,7 +2562,7 @@ MP4ChapterType MP4File::GetChapters(MP4Chapter_t ** chapterList, uint32_t * chap if (0 < counter) { uint32_t timescale = pChapterTrack->GetTimeScale(); - MP4Chapter_t * chapters = (MP4Chapter_t*)MP4Malloc(sizeof(MP4Chapter_t) * counter); + UniqueArray chapters(counter); // process all chapter sample for (uint32_t i = 0; i < counter; ++i) @@ -2588,7 +2588,7 @@ MP4ChapterType MP4File::GetChapters(MP4Chapter_t ** chapterList, uint32_t * chap sample = 0; } - *chapterList = chapters; + *chapterList = chapters.release(); *chapterCount = counter; // we got chapters so we are done @@ -2647,7 +2647,7 @@ MP4ChapterType MP4File::GetChapters(MP4Chapter_t ** chapterList, uint32_t * chap return MP4ChapterTypeNone; } - MP4Chapter_t * chapters = (MP4Chapter_t*)MP4Malloc(sizeof(MP4Chapter_t) * counter); + UniqueArray chapters(counter); // get the name of the first chapter name = pName->GetValue(); @@ -2685,7 +2685,7 @@ MP4ChapterType MP4File::GetChapters(MP4Chapter_t ** chapterList, uint32_t * chap chapters[i].duration = duration; } - *chapterList = chapters; + *chapterList = chapters.release(); *chapterCount = counter; return MP4ChapterTypeNero; diff --git a/src/mp4util.h b/src/mp4util.h index b33bb44..8c8b51e 100644 --- a/src/mp4util.h +++ b/src/mp4util.h @@ -84,6 +84,23 @@ inline void* MP4Realloc(void* p, uint32_t newSize) { return temp; } +// C++98 unique_ptr-like that allocates on construction. +template +class UniqueArray { + T *m_ptr; + UniqueArray(const UniqueArray&); + UniqueArray&operator=(const UniqueArray&); +public: + explicit UniqueArray(size_t count) : m_ptr((T*)MP4Malloc(sizeof(T) * count)) {} + ~UniqueArray() { CHECK_AND_FREE(m_ptr); } + T & operator[](size_t idx) { return m_ptr[idx]; } + T* release() { + T * ptr = m_ptr; + m_ptr = NULL; + return ptr; + } +}; + uint32_t STRTOINT32( const char* ); void INT32TOSTR( uint32_t, char* ); From ccc1d7e77c9719b7d524e7a9718ddd4287eb4560 Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Wed, 1 Jul 2026 19:57:31 +0100 Subject: [PATCH 7/8] Assign array size after reallocation If the reallocation or overflow check fails then an exception will be thrown, leaving the container thinking that it has the new size when it's still using the old data. This lead to cases where we'd try and free memory that we didn't allocate. --- src/mp4array.h | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/mp4array.h b/src/mp4array.h index 69d470a..a3396bc 100644 --- a/src/mp4array.h +++ b/src/mp4array.h @@ -77,9 +77,8 @@ class MP4Array { throw new PlatformException("illegal array index", ERANGE, __FILE__, __LINE__, __FUNCTION__); \ } \ if (m_numElements == m_maxNumElements) { \ - m_maxNumElements = max(m_maxNumElements, (MP4ArrayIndex)1) * 2; \ - m_elements = (type*)MP4Realloc(m_elements, \ - m_maxNumElements * sizeof(type)); \ + MP4ArrayIndex newCapacity = max(m_maxNumElements, (MP4ArrayIndex)1) * 2; \ + Reserve(newCapacity); \ } \ memmove(&m_elements[newIndex + 1], &m_elements[newIndex], \ (m_numElements - newIndex) * sizeof(type)); \ @@ -99,13 +98,19 @@ class MP4Array { (m_numElements - index) * sizeof(type)); \ } \ } \ + \ + void Reserve(MP4ArrayIndex newSize) { \ + if (newSize > m_maxNumElements) { \ + if ( (uint64_t) newSize * sizeof(type) > 0xFFFFFFFF ) \ + throw new PlatformException("requested array size exceeds 4GB", ERANGE, __FILE__, __LINE__, __FUNCTION__); /* prevent overflow */ \ + m_elements = (type*)MP4Realloc(m_elements, newSize * sizeof(type)); \ + m_maxNumElements = newSize; \ + } \ + } \ + \ void Resize(MP4ArrayIndex newSize) { \ + Reserve(newSize); \ m_numElements = newSize; \ - m_maxNumElements = newSize; \ - if ( (uint64_t) m_maxNumElements * sizeof(type) > 0xFFFFFFFF ) \ - throw new PlatformException("requested array size exceeds 4GB", ERANGE, __FILE__, __LINE__, __FUNCTION__); /* prevent overflow */ \ - m_elements = (type*)MP4Realloc(m_elements, \ - m_maxNumElements * sizeof(type)); \ } \ \ type& operator[](MP4ArrayIndex index) { \ From ab04a28546c30f7b7d9a7580a036fd6d13d63981 Mon Sep 17 00:00:00 2001 From: bobsayshilol Date: Wed, 1 Jul 2026 22:30:01 +0100 Subject: [PATCH 8/8] Cast to the correct property type This is created as a 2 bit MP4BitfieldProperty, so read it as that. --- src/descriptors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/descriptors.cpp b/src/descriptors.cpp index e83fc66..062620a 100644 --- a/src/descriptors.cpp +++ b/src/descriptors.cpp @@ -505,7 +505,7 @@ void MP4ContentIdDescriptor::Read(MP4File& file) ReadProperties(file, 0, 1); /* if compatiblity != 0 */ - if (((MP4Integer8Property*)m_pProperties[0])->GetValue() != 0) { + if (((MP4BitfieldProperty*)m_pProperties[0])->GetValue() != 0) { /* we don't understand it */ log.verbose1f("incompatible content id descriptor"); return;