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

diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp
index 01b37705cf0ef2a838f6e402fee0a68635669043..73d2196b946626f410e5bf74dd6e22ad495cbb7e 100644 (file)
--- a/taglib/mpeg/mpegheader.cpp
+++ b/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) :
+  d(new HeaderPrivate())
+{
+  parse(file, offset);
 }
 
 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)
 {
+  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,17 @@ 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
+  // header is near the end of this frame.
+
+  file->seek(offset + d->frameLength);
+  const ByteVector nextSynch = file->readBlock(2);
+
+  if(nextSynch.size() < 2 || !firstSyncByte(nextSynch[0]) || !secondSynchByte(nextSynch[1])) {
+    debug("MPEG::Header::parse() -- Frame length seems to be wrong.");
+    return;
+  }
+
   // Now that we're done parsing, set this to be a valid frame.
 
   d->isValid = true;

diff --git a/taglib/mpeg/mpegheader.h b/taglib/mpeg/mpegheader.h
index a55cac0996e1f60eb0dc7680353ab64664e5e93d..cd77d37a203f0bfc5ee691462eef066420cc5898 100644 (file)
--- a/taglib/mpeg/mpegheader.h
+++ b/taglib/mpeg/mpegheader.h
@@ -31,6 +31,7 @@
 namespace TagLib {
 
   class ByteVector;
+  class File;
 
   namespace MPEG {
 
@@ -48,9 +49,16 @@ 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.
+       */
+      Header(File *file, long offset);
+
       /*!
        * Does a shallow copy of \a h.
        */
@@ -155,7 +163,7 @@ namespace TagLib {
       Header &operator=(const Header &h);
 
     private:
-      void parse(const ByteVector &data);
+      void parse(File *file, long offset);
 
       class HeaderPrivate;
       HeaderPrivate *d;

diff --git a/taglib/mpeg/mpegproperties.cpp b/taglib/mpeg/mpegproperties.cpp
index 989c9065e9a6cc41c61a5abf5f66808a472004bf..8507f38d6e899f1e2cb69bf66f68786b7051a214 100644 (file)
--- a/taglib/mpeg/mpegproperties.cpp
+++ b/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,25 @@ void MPEG::Properties::read(File *file)
 
     d->bitrate = firstHeader.bitrate();
 
-    long streamLength = file->length() - firstFrameOffset;
+    long lastFrameOffset = file->lastFrameOffset();
+    if(lastFrameOffset < 0) {
+      debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream.");
+      return;
+    }
 
-    if(file->hasID3v1Tag())
-      streamLength -= 128;
+    Header lastHeader(file, lastFrameOffset);
 
-    if(file->hasAPETag())
-      streamLength -= file->APETag()->footer()->completeTagSize();
+    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);
+    }
 
+    const long streamLength = lastFrameOffset - firstFrameOffset + lastHeader.frameLength() * 2;
     if(streamLength > 0)
       d->length = static_cast<int>(streamLength * 8.0 / d->bitrate + 0.5);
   }

diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames.mp3
index a6dc1117c4bd168901f24e866c3595f798d8224a..c076712c029bb734f1627817660a4b2a4fdf8925 100644 (file)
Binary files a/tests/data/invalid-frames.mp3 and b/tests/data/invalid-frames.mp3 differ

diff --git a/tests/data/invalid-frames2.mp3 b/tests/data/invalid-frames2.mp3
new file mode 100644 (file)
index 0000000..01976fc
Binary files /dev/null and b/tests/data/invalid-frames2.mp3 differ

diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp
index 92a7fc3af44a8000497063601dd24e17a38294ba..9b972382daf904e7df07d0309e1c4022cf8a74ab 100644 (file)
--- a/tests/test_mpeg.cpp
+++ b/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();
-
-    f.seek(last);
-    MPEG::Header lastHeader(f.readBlock(4));
+    MPEG::Header lastHeader(&f, last);
 
     while (!lastHeader.isValid()) {
-
       last = f.previousFrameOffset(last);
-
-      f.seek(last);
-      lastHeader = MPEG::Header(f.readBlock(4));
+      lastHeader = MPEG::Header(&f, last);
     }
 
-    CPPUNIT_ASSERT_EQUAL(28213L, last);
+    CPPUNIT_ASSERT_EQUAL(28004L, 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"));