From ce3b45f1863c1022b1a2a11bd6032491f3cfd5bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20P=C3=A9ron?= Date: Fri, 7 Aug 2026 14:51:22 +0200 Subject: [PATCH] fix(matroska): find Segments past the fast scan limit (#1398) Element::factory() rejects any element whose declared size runs past the bound it is given, and read() passes the Fast scan limit as that bound. A Segment spans practically the whole file, so under ReadStyle::Fast every Matroska over 512 KiB is rejected and no tags are read: EBML: datasize too great: 1003369 > (524288 - 52) Failed to find Matroska segment The limit is readLimited()'s, which already applies it to skip Cues and to bound the walk over a segment with no usable SeekHead. The lookup only needs the file length. Correctly handle offsets and scan limits: - maxOffset: Maximum offset from the beginning of the file; the end of the element must be before this offset. - scanLimit: Offset from the current file position until which scanning for elements is allowed. Normally, elements are scanned up to the end of the enclosing master element or the end of the file, but in Fast reading mode, it is limited to FAST_SCAN_LIMIT, which is 512 kB. - maxScanOffset: scanLimit from the current file position --------- Co-authored-by: Claude Opus 5 Co-authored-by: Urs Fleisch --- taglib/matroska/ebml/ebmlmksegment.cpp | 4 +-- taglib/matroska/ebml/ebmlutils.cpp | 14 +++++++--- taglib/matroska/ebml/ebmlutils.h | 18 ++++++++++-- taglib/matroska/matroskafile.cpp | 18 ++++++------ tests/test_matroska.cpp | 38 ++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 17 deletions(-) diff --git a/taglib/matroska/ebml/ebmlmksegment.cpp b/taglib/matroska/ebml/ebmlmksegment.cpp index 88fcdd4c..79de5ca5 100644 --- a/taglib/matroska/ebml/ebmlmksegment.cpp +++ b/taglib/matroska/ebml/ebmlmksegment.cpp @@ -95,13 +95,13 @@ bool EBML::MkSegment::readLimited(File &file, offset_t scanLimit) MasterElement *pendingPaddingTarget = nullptr; offset_t accumulatedPadding = 0; std::unique_ptr element; - while((element = findNextElement(file, maxScanOffset))) { + while((element = findNextElement(file, maxOffset, maxScanOffset))) { if(const Id id = element->getId(); id == Id::MkSeekHead) { seekHead = element_cast(std::move(element)); if(!seekHead->read(file)) return false; // We have a seek head, let's use it for faster access to the other elements - if(const auto elementAfterSeekHead = findNextElement(file, maxScanOffset); + if(const auto elementAfterSeekHead = findNextElement(file, maxOffset, maxScanOffset); elementAfterSeekHead && elementAfterSeekHead->getId() == Id::VoidElement) seekHead->setPadding(elementAfterSeekHead->getSize()); const offset_t segDataOffset = segmentDataOffset(); diff --git a/taglib/matroska/ebml/ebmlutils.cpp b/taglib/matroska/ebml/ebmlutils.cpp index b76cdf2c..a150c78a 100644 --- a/taglib/matroska/ebml/ebmlutils.cpp +++ b/taglib/matroska/ebml/ebmlutils.cpp @@ -30,10 +30,13 @@ using namespace TagLib; -std::unique_ptr EBML::findElement(File &file, Element::Id id, offset_t maxOffset) +std::unique_ptr EBML::findElement( + File &file, Element::Id id, offset_t maxOffset, offset_t maxScanOffset) { + if(maxScanOffset < 0) + maxScanOffset = maxOffset; std::unique_ptr element; - while(file.tell() < maxOffset) { + while(file.tell() < maxScanOffset) { element = Element::factory(file, maxOffset); if(!element || element->getId() == id) return element; @@ -43,9 +46,12 @@ std::unique_ptr EBML::findElement(File &file, Element::Id id, off return element; } -std::unique_ptr EBML::findNextElement(File &file, offset_t maxOffset) +std::unique_ptr EBML::findNextElement( + File &file, offset_t maxOffset, offset_t maxScanOffset) { - return file.tell() < maxOffset ? Element::factory(file, maxOffset) : nullptr; + if(maxScanOffset < 0) + maxScanOffset = maxOffset; + return file.tell() < maxScanOffset ? Element::factory(file, maxOffset) : nullptr; } template diff --git a/taglib/matroska/ebml/ebmlutils.h b/taglib/matroska/ebml/ebmlutils.h index f35228dd..abbec1a5 100644 --- a/taglib/matroska/ebml/ebmlutils.h +++ b/taglib/matroska/ebml/ebmlutils.h @@ -31,8 +31,22 @@ namespace TagLib { class ByteVector; namespace EBML { - std::unique_ptr findElement(File &file, Element::Id id, offset_t maxOffset); - std::unique_ptr findNextElement(File &file, offset_t maxOffset); + /*! + * Find element with \a id in \a file starting at current file offset. + * The end of the element must be before \a maxOffset. + * The begin of the element must not be after \a maxScanOffset + * (\a maxOffset if not given). + */ + std::unique_ptr findElement( + File &file, Element::Id id, offset_t maxOffset, offset_t maxScanOffset = -1); + /*! + * Find element in \a file at current file offset. + * The end of the element must be before \a maxOffset. + * The begin of the element must not be after \a maxScanOffset + * (\a maxOffset if not given). + */ + std::unique_ptr findNextElement( + File &file, offset_t maxOffset, offset_t maxScanOffset = -1); template unsigned int VINTSizeLength(uint8_t firstByte); diff --git a/taglib/matroska/matroskafile.cpp b/taglib/matroska/matroskafile.cpp index 7ff001bd..81742a94 100644 --- a/taglib/matroska/matroskafile.cpp +++ b/taglib/matroska/matroskafile.cpp @@ -381,15 +381,15 @@ void Matroska::File::read(bool readProperties, Properties::ReadStyle readStyle) head->skipData(*this); } - offset_t maxOffset = fileLength; - if (readStyle == Properties::ReadStyle::Fast && maxOffset > FAST_SCAN_LIMIT) { - maxOffset = FAST_SCAN_LIMIT; + offset_t maxScanOffset = fileLength; + if(readStyle == Properties::ReadStyle::Fast && maxScanOffset > FAST_SCAN_LIMIT) { + maxScanOffset = FAST_SCAN_LIMIT; } - // Find the Matroska segment in the file + // Find the Matroska segment in the file. const std::unique_ptr segment( EBML::element_cast( - EBML::findElement(*this, EBML::Element::Id::MkSegment, maxOffset) + EBML::findElement(*this, EBML::Element::Id::MkSegment, fileLength, maxScanOffset) ) ); if(!segment) { @@ -400,11 +400,11 @@ void Matroska::File::read(bool readProperties, Properties::ReadStyle readStyle) // Read the segment into memory from file d->segment = segment->parseSegment(); - maxOffset = segment->getDataSize(); - if (readStyle == Properties::ReadStyle::Fast && maxOffset > FAST_SCAN_LIMIT) { - maxOffset = FAST_SCAN_LIMIT; + offset_t scanLimit = segment->getDataSize(); + if(readStyle == Properties::ReadStyle::Fast && scanLimit > FAST_SCAN_LIMIT) { + scanLimit = FAST_SCAN_LIMIT; } - if(!segment->readLimited(*this, maxOffset)) { + if(!segment->readLimited(*this, scanLimit)) { debug("Failed to read segment"); setValid(false); return; diff --git a/tests/test_matroska.cpp b/tests/test_matroska.cpp index 5ec0e79e..730d4a10 100644 --- a/tests/test_matroska.cpp +++ b/tests/test_matroska.cpp @@ -161,6 +161,7 @@ class TestMatroska : public CppUnit::TestFixture CPPUNIT_TEST(testSaveTypesNoTrailingVoid); CPPUNIT_TEST(testSaveTypesReclaimVoid); CPPUNIT_TEST(testUnknownSizeSegment); + CPPUNIT_TEST(testFastReadStyleLargeSegment); CPPUNIT_TEST_SUITE_END(); public: @@ -1778,6 +1779,43 @@ public: } } + void testFastReadStyleLargeSegment() + { + ScopedFileCopy copy("tags-before-cues", ".mkv"); + string newname = copy.fileName(); + + // Grow the segment past FAST_SCAN_LIMIT, as every real world file is. + { + PlainFile file(newname.c_str()); + ByteVector fileData = file.readAll(); + CPPUNIT_ASSERT_EQUAL(3412U, fileData.size()); + + // Void element, appended to the segment so the existing seek positions still hold. + const unsigned int voidDataSize = 1024 * 1024; + ByteVector voidElement("\xec", 1); + voidElement.append(ByteVector::fromULongLong(0x0100000000000000ULL | voidDataSize)); + voidElement.append(ByteVector(voidDataSize, '\0')); + + // Segment: 4 byte ID at 0x28, 8 byte size VINT at 0x2c, data from 0x34 to EOF. + ByteVector newData = fileData.mid(0, 0x2c); + newData.append(ByteVector::fromULongLong( + 0x0100000000000000ULL | (fileData.size() - 0x34 + voidElement.size()))); + newData.append(fileData.mid(0x34)); + newData.append(voidElement); + + file.seek(0); + file.writeBlock(newData); + } + + for(auto readStyle : {AudioProperties::Fast, AudioProperties::Average, + AudioProperties::Accurate}) { + Matroska::File f(newname.c_str(), true, readStyle); + CPPUNIT_ASSERT(f.isValid()); + CPPUNIT_ASSERT(f.tag(false)); + CPPUNIT_ASSERT_EQUAL(String("handbrake"), f.tag()->title()); + } + } + void testUnknownSizeSegment() { ScopedFileCopy copy("no-tags", ".mka");