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 <[email protected]>
Co-authored-by: Urs Fleisch <[email protected]>

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> element;
-  while((element = findNextElement(file, maxScanOffset))) {
+  while((element = findNextElement(file, maxOffset, maxScanOffset))) {
     if(const Id id = element->getId(); id == Id::MkSeekHead) {
       seekHead = element_cast<Id::MkSeekHead>(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();

taglib/matroska/ebml/ebmlutils.cpp

@@ -30,10 +30,13 @@
 
 using namespace TagLib;
 
-std::unique_ptr<EBML::Element> EBML::findElement(File &file, Element::Id id, offset_t maxOffset)
+std::unique_ptr<EBML::Element> EBML::findElement(
+  File &file, Element::Id id, offset_t maxOffset, offset_t maxScanOffset)
 {
+  if(maxScanOffset < 0)
+    maxScanOffset = maxOffset;
   std::unique_ptr<Element> 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::Element> EBML::findElement(File &file, Element::Id id, off
   return element;
 }
 
-std::unique_ptr<EBML::Element> EBML::findNextElement(File &file, offset_t maxOffset)
+std::unique_ptr<EBML::Element> 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 <int maxSizeLength>

taglib/matroska/ebml/ebmlutils.h

@@ -31,8 +31,22 @@ namespace TagLib {
   class ByteVector;
 
   namespace EBML {
-    std::unique_ptr<Element> findElement(File &file, Element::Id id, offset_t maxOffset);
-    std::unique_ptr<Element> 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<Element> 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<Element> findNextElement(
+      File &file, offset_t maxOffset, offset_t maxScanOffset = -1);
 
     template <int maxSizeLength>
     unsigned int VINTSizeLength(uint8_t firstByte);

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<EBML::MkSegment> segment(
     EBML::element_cast<EBML::Element::Id::MkSegment>(
-      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;

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");