Merge pull request #705 from TsudaKageyu/mpeg-invalid-frame

More robust checks for invalid MPEG frame headers. (again)

taglib/mpeg/mpegheader.cpp

@@ -25,6 +25,7 @@
 
 #include <tbytevector.h>
 #include <tstring.h>
+#include <tfile.h>
 #include <tdebug.h>
 #include <trefcounter.h>
 
@@ -70,7 +71,13 @@ public:
 MPEG::Header::Header(const ByteVector &data) :
   d(new HeaderPrivate())
 {
-  parse(data);
+  debug("MPEG::Header::Header() - This constructor is no longer used.");
+}
+
+MPEG::Header::Header(File *file, long offset, bool checkLength) :
+  d(new HeaderPrivate())
+{
+  parse(file, offset, checkLength);
 }
 
 MPEG::Header::Header(const Header &h) :
@@ -162,8 +169,11 @@ MPEG::Header &MPEG::Header::operator=(const Header &h)
 // private members
 ////////////////////////////////////////////////////////////////////////////////
 
-void MPEG::Header::parse(const ByteVector &data)
+void MPEG::Header::parse(File *file, long offset, bool checkLength)
 {
+  file->seek(offset);
+  const ByteVector data = file->readBlock(4);
+
   if(data.size() < 4) {
     debug("MPEG::Header::parse() -- data is too short for an MPEG frame header.");
     return;
@@ -288,6 +298,31 @@ void MPEG::Header::parse(const ByteVector &data)
   if(d->isPadded)
     d->frameLength += paddingSize[layerIndex];
 
+  // Check if the frame length has been calculated correctly, or the next frame
+  // synch bytes are right next to the end of this frame.
+
+  // We read some extra bytes to be a bit tolerant.
+
+  if(checkLength) {
+
+    bool nextFrameFound = false;
+
+    file->seek(offset + d->frameLength);
+    const ByteVector nextSynch = file->readBlock(4);
+
+    for(int i = 0; i < static_cast<int>(nextSynch.size()) - 1; ++i) {
+      if(firstSyncByte(nextSynch[0]) && secondSynchByte(nextSynch[1])) {
+        nextFrameFound = true;
+        break;
+      }
+    }
+
+    if(!nextFrameFound) {
+      debug("MPEG::Header::parse() -- Calculated frame length did not match the actual length.");
+      return;
+    }
+  }
+
   // Now that we're done parsing, set this to be a valid frame.
 
   d->isValid = true;

taglib/mpeg/mpegheader.h

@@ -31,6 +31,7 @@
 namespace TagLib {
 
   class ByteVector;
+  class File;
 
   namespace MPEG {
 
@@ -48,9 +49,20 @@ namespace TagLib {
     public:
       /*!
        * Parses an MPEG header based on \a data.
+       *
+       * \deprecated
        */
       Header(const ByteVector &data);
 
+      /*!
+       * Parses an MPEG header based on \a file and \a offset.
+       *
+       * \note If \a checkLength is true, this requires the next MPEG frame to
+       * check if the frame length is parsed and calculated correctly.  So it's
+       * suitable for seeking for the first valid frame.
+       */
+      Header(File *file, long offset, bool checkLength = true);
+
       /*!
        * Does a shallow copy of \a h.
        */
@@ -155,7 +167,7 @@ namespace TagLib {
       Header &operator=(const Header &h);
 
     private:
-      void parse(const ByteVector &data);
+      void parse(File *file, long offset, bool checkLength);
 
       class HeaderPrivate;
       HeaderPrivate *d;

taglib/mpeg/mpegproperties.cpp

@@ -163,8 +163,7 @@ void MPEG::Properties::read(File *file)
     return;
   }
 
-  file->seek(firstFrameOffset);
-  Header firstHeader(file->readBlock(4));
+  Header firstHeader(file, firstFrameOffset);
 
   while(!firstHeader.isValid()) {
     firstFrameOffset = file->nextFrameOffset(firstFrameOffset + 1);
@@ -173,8 +172,7 @@ void MPEG::Properties::read(File *file)
       return;
     }
 
-    file->seek(firstFrameOffset);
-    firstHeader = Header(file->readBlock(4));
+    firstHeader = Header(file, firstFrameOffset);
   }
 
   // Check for a VBR header that will help us in gathering information about a
@@ -207,14 +205,27 @@ void MPEG::Properties::read(File *file)
 
     d->bitrate = firstHeader.bitrate();
 
-    long streamLength = file->length() - firstFrameOffset;
+    // Look for the last MPEG audio frame to calculate the stream length.
 
-    if(file->hasID3v1Tag())
-      streamLength -= 128;
+    long lastFrameOffset = file->lastFrameOffset();
+    if(lastFrameOffset < 0) {
+      debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream.");
+      return;
+    }
 
-    if(file->hasAPETag())
-      streamLength -= file->APETag()->footer()->completeTagSize();
+    Header lastHeader(file, lastFrameOffset, false);
 
+    while(!lastHeader.isValid()) {
+      lastFrameOffset = file->previousFrameOffset(lastFrameOffset);
+      if(lastFrameOffset < 0) {
+        debug("MPEG::Properties::read() -- Could not find a valid last MPEG frame in the stream.");
+        return;
+      }
+
+      lastHeader = Header(file, lastFrameOffset, false);
+    }
+
+    const long streamLength = lastFrameOffset - firstFrameOffset + lastHeader.frameLength();
     if(streamLength > 0)
       d->length = static_cast<int>(streamLength * 8.0 / d->bitrate + 0.5);
   }

tests/test_mpeg.cpp

@@ -22,7 +22,8 @@ class TestMPEG : public CppUnit::TestFixture
   CPPUNIT_TEST(testAudioPropertiesXingHeaderVBR);
   CPPUNIT_TEST(testAudioPropertiesVBRIHeader);
   CPPUNIT_TEST(testAudioPropertiesNoVBRHeaders);
-  CPPUNIT_TEST(testSkipInvalidFrames);
+  CPPUNIT_TEST(testSkipInvalidFrames1);
+  CPPUNIT_TEST(testSkipInvalidFrames2);
   CPPUNIT_TEST(testVersion2DurationWithXingHeader);
   CPPUNIT_TEST(testSaveID3v24);
   CPPUNIT_TEST(testSaveID3v24WrongParam);
@@ -93,35 +94,43 @@ public:
     CPPUNIT_ASSERT(!f.audioProperties()->xingHeader());
 
     long last = f.lastFrameOffset();
+    MPEG::Header lastHeader(&f, last, false);
 
-    f.seek(last);
-    MPEG::Header lastHeader(f.readBlock(4));
-
-    while (!lastHeader.isValid()) {
-
+    while(!lastHeader.isValid()) {
       last = f.previousFrameOffset(last);
-
-      f.seek(last);
-      lastHeader = MPEG::Header(f.readBlock(4));
+      lastHeader = MPEG::Header(&f, last, false);
     }
 
     CPPUNIT_ASSERT_EQUAL(28213L, last);
     CPPUNIT_ASSERT_EQUAL(209, lastHeader.frameLength());
   }
 
-  void testSkipInvalidFrames()
+  void testSkipInvalidFrames1()
   {
     MPEG::File f(TEST_FILE_PATH_C("invalid-frames.mp3"));
     CPPUNIT_ASSERT(f.audioProperties());
     CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length());
     CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds());
-    CPPUNIT_ASSERT_EQUAL(393, f.audioProperties()->lengthInMilliseconds());
+    CPPUNIT_ASSERT_EQUAL(392, f.audioProperties()->lengthInMilliseconds());
     CPPUNIT_ASSERT_EQUAL(160, f.audioProperties()->bitrate());
     CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels());
     CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate());
     CPPUNIT_ASSERT(!f.audioProperties()->xingHeader());
   }
 
+  void testSkipInvalidFrames2()
+  {
+    MPEG::File f(TEST_FILE_PATH_C("invalid-frames2.mp3"));
+    CPPUNIT_ASSERT(f.audioProperties());
+    CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length());
+    CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds());
+    CPPUNIT_ASSERT_EQUAL(314, f.audioProperties()->lengthInMilliseconds());
+    CPPUNIT_ASSERT_EQUAL(192, f.audioProperties()->bitrate());
+    CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels());
+    CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate());
+    CPPUNIT_ASSERT(!f.audioProperties()->xingHeader());
+  }
+
   void testVersion2DurationWithXingHeader()
   {
     MPEG::File f(TEST_FILE_PATH_C("mpeg2.mp3"));