From: Tsuda Kageyu <tsuda.kageyu@gmail.com>
Date: Tue, 22 Dec 2015 05:54:07 +0000 (+0900)
Subject: More robust checks for invalid MPEG frame headers. (again)
X-Git-Tag: v1.11beta~15^2~3
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=081d6eaf7656680e477244ba863afc930c09d6c0;p=taglib
More robust checks for invalid MPEG frame headers. (again)
---
diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp
index 01b37705..73d2196b 100644
--- 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 a55cac09..cd77d37a 100644
--- 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 989c9065..8507f38d 100644
--- 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 a6dc1117..c076712c 100644
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
index 00000000..01976fc5
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 92a7fc3a..9b972382 100644
--- 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"));