From 94b78289900337e57dc1c090cd2485bc80da8100 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 24 Nov 2015 23:41:10 +0900 Subject: [PATCH 01/12] Fix saving MP4 files. This fixes the issue reported at #619. --- taglib/mp4/mp4file.cpp | 18 +++--------------- taglib/mp4/mp4file.h | 3 --- taglib/mp4/mp4tag.cpp | 5 +++++ tests/test_mp4.cpp | 12 ++++++++++++ 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/taglib/mp4/mp4file.cpp b/taglib/mp4/mp4file.cpp index 566a5e32..d0a6c4c6 100644 --- a/taglib/mp4/mp4file.cpp +++ b/taglib/mp4/mp4file.cpp @@ -55,8 +55,7 @@ public: FilePrivate() : tag(0), atoms(0), - properties(0), - hasMP4Tag(false) {} + properties(0) {} ~FilePrivate() { @@ -68,8 +67,6 @@ public: MP4::Tag *tag; MP4::Atoms *atoms; MP4::Properties *properties; - - bool hasMP4Tag; }; MP4::File::File(FileName file, bool readProperties, AudioProperties::ReadStyle) : @@ -138,10 +135,6 @@ MP4::File::read(bool readProperties) return; } - if(d->atoms->find("moov", "udta", "meta", "ilst")) { - d->hasMP4Tag = true; - } - d->tag = new Tag(this, d->atoms); if(readProperties) { d->properties = new Properties(this, d->atoms); @@ -161,16 +154,11 @@ MP4::File::save() return false; } - const bool success = d->tag->save(); - if(success) { - d->hasMP4Tag = true; - } - - return success; + return d->tag->save(); } bool MP4::File::hasMP4Tag() const { - return d->hasMP4Tag; + return (d->atoms->find("moov", "udta", "meta", "ilst") != 0); } diff --git a/taglib/mp4/mp4file.h b/taglib/mp4/mp4file.h index 40fcde78..3840bd02 100644 --- a/taglib/mp4/mp4file.h +++ b/taglib/mp4/mp4file.h @@ -111,9 +111,6 @@ namespace TagLib { * Save the file. * * This returns true if the save was successful. - * - * \warning In the current implementation, it's dangerous to call save() - * repeatedly. At worst it will corrupt the file. */ bool save(); diff --git a/taglib/mp4/mp4tag.cpp b/taglib/mp4/mp4tag.cpp index 84219196..384a4548 100644 --- a/taglib/mp4/mp4tag.cpp +++ b/taglib/mp4/mp4tag.cpp @@ -629,6 +629,11 @@ MP4::Tag::saveNew(ByteVector data) updateParents(path, data.size()); updateOffsets(data.size(), offset); + + // Insert the newly created atoms into the tree to keep it up-to-date. + + d->file->seek(offset); + path.back()->children.prepend(new Atom(d->file)); } void diff --git a/tests/test_mp4.cpp b/tests/test_mp4.cpp index 58dbae42..10b1e1fc 100644 --- a/tests/test_mp4.cpp +++ b/tests/test_mp4.cpp @@ -30,6 +30,7 @@ class TestMP4 : public CppUnit::TestFixture CPPUNIT_TEST(testCovrRead2); CPPUNIT_TEST(testProperties); CPPUNIT_TEST(testFuzzedFile); + CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST_SUITE_END(); public: @@ -344,6 +345,17 @@ public: CPPUNIT_ASSERT(f.isValid()); } + void testRepeatedSave() + { + ScopedFileCopy copy("no-tags", ".m4a"); + + MP4::File f(copy.fileName().c_str()); + f.tag()->setTitle("0123456789"); + f.save(); + f.save(); + CPPUNIT_ASSERT_EQUAL(2862L, f.find("0123456789")); + CPPUNIT_ASSERT_EQUAL(-1L, f.find("0123456789", 2863)); + } }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMP4); From e6e9ba60947019b19b93b830e4f05442570404c3 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 24 Nov 2015 12:27:39 +0900 Subject: [PATCH 02/12] Fix saving FLAC files. This fixes all the issues reported at #622. --- taglib/flac/flacfile.cpp | 148 ++++++++++++++++++++++++--------------- taglib/flac/flacfile.h | 3 - tests/test_flac.cpp | 113 ++++++++++++++++++++++++++++-- 3 files changed, 200 insertions(+), 64 deletions(-) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 252907d0..90df397a 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -46,27 +46,25 @@ using namespace TagLib; namespace { enum { FlacXiphIndex = 0, FlacID3v2Index = 1, FlacID3v1Index = 2 }; - enum { MinPaddingLength = 4096 }; - enum { LastBlockFlag = 0x80 }; + + const long MinPaddingLength = 4096; + const long MaxPaddingLegnth = 1024 * 1024; + + const char LastBlockFlag = '\x80'; } class FLAC::File::FilePrivate { public: - FilePrivate() : - ID3v2FrameFactory(ID3v2::FrameFactory::instance()), + FilePrivate(const ID3v2::FrameFactory *frameFactory = ID3v2::FrameFactory::instance()) : + ID3v2FrameFactory(frameFactory), ID3v2Location(-1), ID3v2OriginalSize(0), ID3v1Location(-1), properties(0), flacStart(0), streamStart(0), - scanned(false), - hasXiphComment(false), - hasID3v2(false), - hasID3v1(false) - { - } + scanned(false) {} ~FilePrivate() { @@ -92,10 +90,6 @@ public: long flacStart; long streamStart; bool scanned; - - bool hasXiphComment; - bool hasID3v2; - bool hasID3v1; }; //////////////////////////////////////////////////////////////////////////////// @@ -113,9 +107,8 @@ FLAC::File::File(FileName file, bool readProperties, Properties::ReadStyle) : FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory, bool readProperties, Properties::ReadStyle) : TagLib::File(file), - d(new FilePrivate()) + d(new FilePrivate(frameFactory)) { - d->ID3v2FrameFactory = frameFactory; if(isOpen()) read(readProperties); } @@ -123,9 +116,8 @@ FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory, FLAC::File::File(IOStream *stream, ID3v2::FrameFactory *frameFactory, bool readProperties, Properties::ReadStyle) : TagLib::File(stream), - d(new FilePrivate()) + d(new FilePrivate(frameFactory)) { - d->ID3v2FrameFactory = frameFactory; if(isOpen()) read(readProperties); } @@ -214,39 +206,83 @@ bool FLAC::File::save() data.append(blockData); } - // Adjust the padding block(s) + // Compute the amount of padding, and append that to data. + // TODO: Should be calculated in offset_t in taglib2. long originalLength = d->streamStart - d->flacStart; - int paddingLength = originalLength - data.size() - 4; + long paddingLength = originalLength - data.size() - 4; + if(paddingLength <= 0) { paddingLength = MinPaddingLength; } - ByteVector padding = ByteVector::fromUInt(paddingLength); - padding.resize(paddingLength + 4); - padding[0] = (char)(FLAC::MetadataBlock::Padding | LastBlockFlag); - data.append(padding); + else { + // Padding won't increase beyond 1% of the file size or 1MB. + + long threshold = length() / 100; + threshold = std::max(threshold, MinPaddingLength); + threshold = std::min(threshold, MaxPaddingLegnth); + + if(paddingLength > threshold) + paddingLength = MinPaddingLength; + } + + ByteVector paddingHeader = ByteVector::fromUInt(paddingLength); + paddingHeader[0] = static_cast(MetadataBlock::Padding | LastBlockFlag); + data.append(paddingHeader); + data.resize(static_cast(data.size() + paddingLength)); // Write the data to the file insert(data, d->flacStart, originalLength); - d->hasXiphComment = true; + + d->streamStart += (data.size() - originalLength); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (data.size() - originalLength); // Update ID3 tags - if(ID3v2Tag()) { - if(d->hasID3v2) { - if(d->ID3v2Location < d->flacStart) - debug("FLAC::File::save() -- This can't be right -- an ID3v2 tag after the " - "start of the FLAC bytestream? Not writing the ID3v2 tag."); - else - insert(ID3v2Tag()->render(), d->ID3v2Location, d->ID3v2OriginalSize); + if(ID3v2Tag() && !ID3v2Tag()->isEmpty()) { + + // ID3v2 tag is not empty. Update the old one or create a new one. + + if(d->ID3v2Location < 0) + d->ID3v2Location = 0; + + data = ID3v2Tag()->render(); + insert(data, d->ID3v2Location, d->ID3v2OriginalSize); + + d->flacStart += (data.size() - d->ID3v2OriginalSize); + d->streamStart += (data.size() - d->ID3v2OriginalSize); + + if(d->ID3v1Location >= 0) + d->ID3v1Location += (data.size() - d->ID3v2OriginalSize); + + d->ID3v2OriginalSize = data.size(); + } + else { + + // ID3v2 tag is empty. Remove the old one. + + if(d->ID3v2Location >= 0) { + removeBlock(d->ID3v2Location, d->ID3v2OriginalSize); + + d->flacStart -= d->ID3v2OriginalSize; + d->streamStart -= d->ID3v2OriginalSize; + + if(d->ID3v1Location >= 0) + d->ID3v1Location -= d->ID3v2OriginalSize; + + d->ID3v2Location = -1; + d->ID3v2OriginalSize = 0; } - else - insert(ID3v2Tag()->render(), 0, 0); } - if(ID3v1Tag()) { - if(d->hasID3v1) { + if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) { + + // ID3v1 tag is not empty. Update the old one or create a new one. + + if(d->ID3v1Location >= 0) { seek(d->ID3v1Location); } else { @@ -255,7 +291,15 @@ bool FLAC::File::save() } writeBlock(ID3v1Tag()->render()); - d->hasID3v1 = true; + } + else { + + // ID3v1 tag is empty. Remove the old one. + + if(d->ID3v1Location >= 0) { + truncate(d->ID3v1Location); + d->ID3v1Location = -1; + } } return true; @@ -342,17 +386,17 @@ void FLAC::File::removePictures() bool FLAC::File::hasXiphComment() const { - return d->hasXiphComment; + return !d->xiphCommentData.isEmpty(); } bool FLAC::File::hasID3v1Tag() const { - return d->hasID3v1; + return (d->ID3v1Location >= 0); } bool FLAC::File::hasID3v2Tag() const { - return d->hasID3v2; + return (d->ID3v2Location >= 0); } //////////////////////////////////////////////////////////////////////////////// @@ -366,25 +410,16 @@ void FLAC::File::read(bool readProperties) d->ID3v2Location = Utils::findID3v2(this); if(d->ID3v2Location >= 0) { - d->tag.set(FlacID3v2Index, new ID3v2::Tag(this, d->ID3v2Location, d->ID3v2FrameFactory)); - d->ID3v2OriginalSize = ID3v2Tag()->header()->completeTagSize(); - - if(ID3v2Tag()->header()->tagSize() <= 0) - d->tag.set(FlacID3v2Index, 0); - else - d->hasID3v2 = true; } // Look for an ID3v1 tag d->ID3v1Location = Utils::findID3v1(this); - if(d->ID3v1Location >= 0) { + if(d->ID3v1Location >= 0) d->tag.set(FlacID3v1Index, new ID3v1::Tag(this, d->ID3v1Location)); - d->hasID3v1 = true; - } // Look for FLAC metadata, including vorbis comments @@ -393,10 +428,10 @@ void FLAC::File::read(bool readProperties) if(!isValid()) return; - if(d->hasXiphComment) + if(!d->xiphCommentData.isEmpty()) d->tag.set(FlacXiphIndex, new Ogg::XiphComment(d->xiphCommentData)); else - d->tag.set(FlacXiphIndex, new Ogg::XiphComment); + d->tag.set(FlacXiphIndex, new Ogg::XiphComment()); if(readProperties) { @@ -406,10 +441,10 @@ void FLAC::File::read(bool readProperties) long streamLength; - if(d->hasID3v1) + if(d->ID3v1Location >= 0) streamLength = d->ID3v1Location - d->streamStart; else - streamLength = File::length() - d->streamStart; + streamLength = length() - d->streamStart; d->properties = new Properties(infoData, streamLength); } @@ -427,7 +462,7 @@ void FLAC::File::scan() long nextBlockOffset; - if(d->hasID3v2) + if(d->ID3v2Location >= 0) nextBlockOffset = find("fLaC", d->ID3v2Location + d->ID3v2OriginalSize); else nextBlockOffset = find("fLaC"); @@ -495,9 +530,8 @@ void FLAC::File::scan() // Found the vorbis-comment if(blockType == MetadataBlock::VorbisComment) { - if(!d->hasXiphComment) { + if(d->xiphCommentData.isEmpty()) { d->xiphCommentData = data; - d->hasXiphComment = true; } else { debug("FLAC::File::scan() -- multiple Vorbis Comment blocks found, using the first one"); diff --git a/taglib/flac/flacfile.h b/taglib/flac/flacfile.h index 6cbcb5a0..cf8c15c0 100644 --- a/taglib/flac/flacfile.h +++ b/taglib/flac/flacfile.h @@ -155,9 +155,6 @@ namespace TagLib { * has no XiphComment, one will be constructed from the ID3-tags. * * This returns true if the save was successful. - * - * \warning In the current implementation, it's dangerous to call save() - * repeatedly. At worst it will corrupt the file. */ virtual bool save(); diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index 9fdd51e1..8b2e1f5b 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "utils.h" @@ -22,13 +23,19 @@ class TestFLAC : public CppUnit::TestFixture CPPUNIT_TEST(testAddPicture); CPPUNIT_TEST(testReplacePicture); CPPUNIT_TEST(testRemoveAllPictures); - CPPUNIT_TEST(testRepeatedSave); + CPPUNIT_TEST(testRepeatedSave1); + CPPUNIT_TEST(testRepeatedSave2); + CPPUNIT_TEST(testRepeatedSave3); CPPUNIT_TEST(testSaveMultipleValues); CPPUNIT_TEST(testDict); CPPUNIT_TEST(testInvalid); CPPUNIT_TEST(testAudioProperties); - CPPUNIT_TEST(testZeroSizedPadding); + CPPUNIT_TEST(testZeroSizedPadding1); + CPPUNIT_TEST(testZeroSizedPadding2); + CPPUNIT_TEST(testShrinkPadding); CPPUNIT_TEST(testSaveID3v1); + CPPUNIT_TEST(testUpdateID3v2); + CPPUNIT_TEST(testEmptyID3v2); CPPUNIT_TEST_SUITE_END(); public: @@ -185,7 +192,7 @@ public: } } - void testRepeatedSave() + void testRepeatedSave1() { ScopedFileCopy copy("silence-44-s", ".flac"); string newname = copy.fileName(); @@ -206,6 +213,31 @@ public: } } + void testRepeatedSave2() + { + ScopedFileCopy copy("no-tags", ".flac"); + + FLAC::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle("0123456789"); + f.save(); + CPPUNIT_ASSERT_EQUAL(5735L, f.length()); + f.save(); + CPPUNIT_ASSERT_EQUAL(5735L, f.length()); + CPPUNIT_ASSERT(f.find("fLaC") >= 0); + } + + void testRepeatedSave3() + { + ScopedFileCopy copy("no-tags", ".flac"); + + FLAC::File f(copy.fileName().c_str()); + f.xiphComment()->setTitle(std::string(8 * 1024, 'X').c_str()); + f.save(); + CPPUNIT_ASSERT_EQUAL(12862L, f.length()); + f.save(); + CPPUNIT_ASSERT_EQUAL(12862L, f.length()); + } + void testSaveMultipleValues() { ScopedFileCopy copy("silence-44-s", ".flac"); @@ -278,7 +310,7 @@ public: f.audioProperties()->signature()); } - void testZeroSizedPadding() + void testZeroSizedPadding1() { ScopedFileCopy copy("zero-sized-padding", ".flac"); @@ -286,6 +318,44 @@ public: CPPUNIT_ASSERT(f.isValid()); } + void testZeroSizedPadding2() + { + ScopedFileCopy copy("silence-44-s", ".flac"); + + { + FLAC::File f(copy.fileName().c_str()); + f.xiphComment()->setTitle("ABC"); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + f.xiphComment()->setTitle(std::string(3067, 'X').c_str()); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.isValid()); + } + } + + void testShrinkPadding() + { + ScopedFileCopy copy("no-tags", ".flac"); + + { + FLAC::File f(copy.fileName().c_str()); + f.xiphComment()->setTitle(std::wstring(128 * 1024, L'X').c_str()); + f.save(); + CPPUNIT_ASSERT(f.length() > 128 * 1024); + } + { + FLAC::File f(copy.fileName().c_str()); + f.xiphComment()->setTitle("0123456789"); + f.save(); + CPPUNIT_ASSERT(f.length() < 8 * 1024); + } + } + void testSaveID3v1() { ScopedFileCopy copy("no-tags", ".flac"); @@ -309,6 +379,41 @@ public: } } + void testUpdateID3v2() + { + ScopedFileCopy copy("no-tags", ".flac"); + + { + FLAC::File f(copy.fileName().c_str()); + f.ID3v2Tag(true)->setTitle("0123456789"); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + f.ID3v2Tag()->setTitle("ABCDEFGHIJ"); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT_EQUAL(String("ABCDEFGHIJ"), f.ID3v2Tag()->title()); + } + } + + void testEmptyID3v2() + { + ScopedFileCopy copy("no-tags", ".flac"); + + { + FLAC::File f(copy.fileName().c_str()); + f.ID3v2Tag(true); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(!f.hasID3v2Tag()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestFLAC); From 52d3122e9e603419799a03dfee1df7a2dd4788ef Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 3 Dec 2015 16:55:05 +0900 Subject: [PATCH 03/12] Remove some unused private data members. --- taglib/riff/rifffile.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/taglib/riff/rifffile.cpp b/taglib/riff/rifffile.cpp index b96d3db5..b1064cc1 100644 --- a/taglib/riff/rifffile.cpp +++ b/taglib/riff/rifffile.cpp @@ -51,10 +51,8 @@ public: size(0) {} const Endianness endianness; - ByteVector type; - unsigned int size; - ByteVector format; + unsigned int size; std::vector chunks; }; @@ -177,8 +175,7 @@ void RIFF::File::setChunkData(const ByteVector &name, const ByteVector &data, bo // Couldn't find an existing chunk, so let's create a new one. - unsigned int i = d->chunks.size() - 1; - unsigned long offset = d->chunks[i].offset + d->chunks[i].size; + unsigned long offset = d->chunks.back().offset + d->chunks.back().size; // First we update the global size @@ -192,7 +189,7 @@ void RIFF::File::setChunkData(const ByteVector &name, const ByteVector &data, bo // And update our internal structure if(offset & 1) { - d->chunks[i].padding = 1; + d->chunks.back().padding = 1; offset++; } @@ -235,11 +232,13 @@ void RIFF::File::removeChunk(const ByteVector &name) void RIFF::File::read() { - bool bigEndian = (d->endianness == BigEndian); + const bool bigEndian = (d->endianness == BigEndian); + const long baseOffset = tell(); - d->type = readBlock(4); + seek(baseOffset + 4); d->size = readBlock(4).toUInt(bigEndian); - d->format = readBlock(4); + + seek(baseOffset + 12); // + 8: chunk header at least, fix for additional junk bytes while(tell() + 8 <= length()) { From 62ec63b0391d15ae727a90ea14752cbf4c13190a Mon Sep 17 00:00:00 2001 From: Festus Hagen Date: Thu, 3 Dec 2015 14:36:42 -0500 Subject: [PATCH 04/12] Disable tests with a shared library. --- CMakeLists.txt | 4 ++-- ConfigureChecks.cmake | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b01022ce..0be18e27 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -129,7 +129,7 @@ if(WITH_MP4) set(TAGLIB_WITH_MP4 TRUE) endif() -option(TRACE_IN_RELEASE "Output debug messages even in release mode" OFF) +option(TRACE_IN_RELEASE "Output debug messages even in release mode" OFF) if(TRACE_IN_RELEASE) set(TRACE_IN_RELEASE TRUE) endif() @@ -142,7 +142,7 @@ if(BUILD_BINDINGS) add_subdirectory(bindings) endif() -if(BUILD_TESTS) +if(BUILD_TESTS AND NOT BUILD_SHARED_LIBS) enable_testing() add_subdirectory(tests) endif() diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index 3fa01bb1..d0ad1eaa 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -223,7 +223,7 @@ if(NOT ZLIB_SOURCE) endif() endif() -if(BUILD_TESTS) +if(BUILD_TESTS AND NOT BUILD_SHARED_LIBS) find_package(CppUnit) if(NOT CppUnit_FOUND) message(STATUS "CppUnit not found, disabling tests.") From 9dfb3fe45244fc479444e012324e4285a35db482 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 4 Dec 2015 14:15:51 +0900 Subject: [PATCH 05/12] Update NEWS. --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index bec7d290..9a7a2933 100644 --- a/NEWS +++ b/NEWS @@ -13,10 +13,12 @@ * Fixed crash when calling File::properties() after strip(). * Fixed possible file corruptions when saving ASF files. * Fixed updating the comment field of Vorbis comments. + * Fixed reading date and time in ID3v2.3 tags. * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. * Marked ByteVector::null and ByteVector::isNull() deprecated. * Marked String::null and ByteVector::isNull() deprecated. * Marked XiphComment::removeField() deprecated. + * Marked custom integer types deprecated. * Many smaller bug fixes and performance improvements. TagLib 1.10 (Nov 11, 2015) From 847eec23cf60fb32df8193b2c2678de98433376c Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 4 Dec 2015 14:28:56 +0900 Subject: [PATCH 06/12] Fix careless copy-and-paste code. --- taglib/riff/riffutils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taglib/riff/riffutils.h b/taglib/riff/riffutils.h index 42977efe..14b8508e 100644 --- a/taglib/riff/riffutils.h +++ b/taglib/riff/riffutils.h @@ -34,7 +34,7 @@ namespace TagLib { namespace RIFF { - static bool isValidChunkName(const ByteVector &name) + inline bool isValidChunkName(const ByteVector &name) { if(name.size() != 4) return false; From be9b5cc93a3535023fc2bacc1f3dda3364c83bf0 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 8 Dec 2015 11:11:50 +0900 Subject: [PATCH 07/12] More robust checks for invalid MPEG frame headers. --- taglib/mpeg/mpegheader.cpp | 79 ++++++++++++++++++++------------- taglib/mpeg/mpegproperties.cpp | 30 ++++++++----- tests/data/invalid-frames.mp3 | Bin 0 -> 8192 bytes tests/test_mpeg.cpp | 14 ++++++ 4 files changed, 81 insertions(+), 42 deletions(-) create mode 100644 tests/data/invalid-frames.mp3 diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 01cc6c57..05177f3e 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -23,8 +23,6 @@ * http://www.mozilla.org/MPL/ * ***************************************************************************/ -#include - #include #include #include @@ -68,20 +66,21 @@ public: // public members //////////////////////////////////////////////////////////////////////////////// -MPEG::Header::Header(const ByteVector &data) +MPEG::Header::Header(const ByteVector &data) : + d(new HeaderPrivate()) { - d = new HeaderPrivate; parse(data); } -MPEG::Header::Header(const Header &h) : d(h.d) +MPEG::Header::Header(const Header &h) : + d(h.d) { d->ref(); } MPEG::Header::~Header() { - if (d->deref()) + if(d->deref()) delete d; } @@ -164,39 +163,54 @@ MPEG::Header &MPEG::Header::operator=(const Header &h) void MPEG::Header::parse(const ByteVector &data) { - if(data.size() < 4 || static_cast(data[0]) != 0xff) { + if(data.size() < 4) { + debug("MPEG::Header::parse() -- data is too short for an MPEG frame header."); + return; + } + + // Check for the MPEG synch bytes. + + if(static_cast(data[0]) != 0xFF) { debug("MPEG::Header::parse() -- First byte did not match MPEG synch."); return; } - std::bitset<32> flags(TAGLIB_CONSTRUCT_BITSET(data.toUInt())); - - // Check for the second byte's part of the MPEG synch - - if(!flags[23] || !flags[22] || !flags[21]) { + if((static_cast(data[1]) & 0xE0) != 0xE0) { debug("MPEG::Header::parse() -- Second byte did not match MPEG synch."); return; } // Set the MPEG version - if(!flags[20] && !flags[19]) + const int versionBits = (static_cast(data[1]) >> 3) & 0x03; + + if(versionBits == 0) d->version = Version2_5; - else if(flags[20] && !flags[19]) + else if(versionBits == 2) d->version = Version2; - else if(flags[20] && flags[19]) + else if(versionBits == 3) d->version = Version1; + else { + debug("MPEG::Header::parse() -- Invalid MPEG version bits."); + return; + } // Set the MPEG layer - if(!flags[18] && flags[17]) - d->layer = 3; - else if(flags[18] && !flags[17]) - d->layer = 2; - else if(flags[18] && flags[17]) - d->layer = 1; + const int layerBits = (static_cast(data[1]) >> 1) & 0x03; - d->protectionEnabled = !flags[16]; + if(layerBits == 1) + d->layer = 3; + else if(layerBits == 2) + d->layer = 2; + else if(layerBits == 3) + d->layer = 1; + else { + debug("MPEG::Header::parse() -- Invalid MPEG layer bits."); + return; + } + + d->protectionEnabled = (static_cast(data[1] & 0x01) == 0); // Set the bitrate @@ -219,9 +233,14 @@ void MPEG::Header::parse(const ByteVector &data) // The bitrate index is encoded as the first 4 bits of the 3rd byte, // i.e. 1111xxxx - int i = static_cast(data[2]) >> 4; + const int bitrateIndex = (static_cast(data[2]) >> 4) & 0x0F; - d->bitrate = bitrates[versionIndex][layerIndex][i]; + d->bitrate = bitrates[versionIndex][layerIndex][bitrateIndex]; + + if(d->bitrate == 0) { + debug("MPEG::Header::parse() -- Invalid bit rate."); + return; + } // Set the sample rate @@ -233,9 +252,9 @@ void MPEG::Header::parse(const ByteVector &data) // The sample rate index is encoded as two bits in the 3nd byte, i.e. xxxx11xx - i = static_cast(data[2]) >> 2 & 0x03; + const int samplerateIndex = (static_cast(data[2]) >> 2) & 0x03; - d->sampleRate = sampleRates[d->version][i]; + d->sampleRate = sampleRates[d->version][samplerateIndex]; if(d->sampleRate == 0) { debug("MPEG::Header::parse() -- Invalid sample rate."); @@ -245,13 +264,13 @@ void MPEG::Header::parse(const ByteVector &data) // The channel mode is encoded as a 2 bit value at the end of the 3nd byte, // i.e. xxxxxx11 - d->channelMode = static_cast((static_cast(data[3]) & 0xC0) >> 6); + d->channelMode = static_cast((static_cast(data[3]) >> 6) & 0x03); // TODO: Add mode extension for completeness - d->isOriginal = flags[2]; - d->isCopyrighted = flags[3]; - d->isPadded = flags[9]; + d->isOriginal = ((static_cast(data[3]) & 0x04) != 0); + d->isCopyrighted = ((static_cast(data[3]) & 0x08) != 0); + d->isPadded = ((static_cast(data[2]) & 0x02) != 0); // Samples per frame diff --git a/taglib/mpeg/mpegproperties.cpp b/taglib/mpeg/mpegproperties.cpp index 9fece404..989c9065 100644 --- a/taglib/mpeg/mpegproperties.cpp +++ b/taglib/mpeg/mpegproperties.cpp @@ -155,27 +155,33 @@ bool MPEG::Properties::isOriginal() const void MPEG::Properties::read(File *file) { - // Only the first frame is required if we have a VBR header. + // Only the first valid frame is required if we have a VBR header. - const long first = file->firstFrameOffset(); - if(first < 0) { - debug("MPEG::Properties::read() -- Could not find a valid first MPEG frame in the stream."); + long firstFrameOffset = file->firstFrameOffset(); + if(firstFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream."); return; } - file->seek(first); - const Header firstHeader(file->readBlock(4)); + file->seek(firstFrameOffset); + Header firstHeader(file->readBlock(4)); - if(!firstHeader.isValid()) { - debug("MPEG::Properties::read() -- The first page header is invalid."); - return; + while(!firstHeader.isValid()) { + firstFrameOffset = file->nextFrameOffset(firstFrameOffset + 1); + if(firstFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find a valid first MPEG frame in the stream."); + return; + } + + file->seek(firstFrameOffset); + firstHeader = Header(file->readBlock(4)); } // Check for a VBR header that will help us in gathering information about a // VBR stream. - file->seek(first + 4); - d->xingHeader = new XingHeader(file->readBlock(firstHeader.frameLength() - 4)); + file->seek(firstFrameOffset); + d->xingHeader = new XingHeader(file->readBlock(firstHeader.frameLength())); if(!d->xingHeader->isValid()) { delete d->xingHeader; d->xingHeader = 0; @@ -201,7 +207,7 @@ void MPEG::Properties::read(File *file) d->bitrate = firstHeader.bitrate(); - long streamLength = file->length() - first; + long streamLength = file->length() - firstFrameOffset; if(file->hasID3v1Tag()) streamLength -= 128; diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames.mp3 new file mode 100644 index 0000000000000000000000000000000000000000..a6dc1117c4bd168901f24e866c3595f798d8224a GIT binary patch literal 8192 zcmV+bAphTMF_~i((dIeU^}(GhRtTnpIXSke2mFzbN2s8I;S44ZXXojlZ$#uRbwV@Q zejxE(U`jLyEhMCJ+}TciZy0#>Qn+>*LX54`VgEaeQltv|d7*e@Fd{ zMhVL|qyOynBZrht5#%GlniTHH4iW`_#FJd{#JA!P9>=TseHYq{O8G4g>-PrfY3&d! zrddA6oHELXoKCzS7M8gs+uWt{N196=#5*Yf#9jtxeXHAO?aE=nd+V#j@fV zQK2nxk{pVc4N>%;x^&XdQIum03TprRqHKzQ1h8h?>kmn0Mnl_rVP!m%xovAqu_;ka zmaXZ89Wu~$)nOK?hZbwlJp7YRayBBnYF4)FjMcnTEb>i#Vo+4lCn!ZFT3Yc!lU+7E5>K_&Nr=8Qi_;EvMKfUHV4;x! zdi_kU1!*SSa^k`RAVf+=;`K^c#{B6BpwM_fy62)38x~DNm*X=D7BT2#7oTFRbi^jb zTPV4!l`Zw-MtVg~8VLcqze>Ftov5#atpMaA5F)|UXT5+)#0e#)+>+cfiN7&K&XCip z2%{y4T}XSfLE_w$Umi{Sj<;wzZS1ohP58^$UX@%f>RxAECt+a05H`nz2a(uPOmbz| z(YdFFPcy7ElGM3FYuU&VLCsk-NPE>)*nTS$Zm7%ke&mvi6N<3H_K5`*8&~+E=*r5_ zn^?+eFD9B}Uw20lt8$cJuejoQj^`FM`e5_zj9OJF#*;*%?IIj|1;fEkDZ~K@HrtY1 zqE<$e3^o-q8dw3+_z(PzT4Xo=jwiXF*gTwj%WTC_zE7OB#u65!NTS`rZ`YN-P?aHA z<_TmYY6w0KL^hm3RlBACmoH?uXIb*%&p)l*lx64Peo46{w5oK>&~n-EAR1yz5JIyY zN;s4*W*=OyK^WoaGNV3Y5UF8h63DZsTL1f?Y?lB9C}CS`FG}WGMmv3BVqB9!S#3={ zEs<3rE$H-#a@`rW@$|UxBf_JJ@SyH1q=Cc5Y#eoSjk!0R&X_pc85uR6B(c7GCn zELgerP;f_!p=H}jR*`;9Hs!cGh2pjn;Y>V}j&ptm`#RS=un@@tV6-!&=6WTn?rmWs z^JBsky%Z?qJ&w$lAZgx#wFxfv_3hD#sLW!I7n)(_D$y=XN@4yX^X#Hd)q?Rr zi(JLO5JzO5u&g2J6AZ~Mxk;6gx=rr?D)s(K;o!ZieOW68r5CTVx~ub@%5!E+?bb=c z0c|xTw%!ilmmUsh5%>m?=NZe$m>RjlzzZfwJ2sUUhyR7&7~JA#6a|pXq@a^RA{EG6 zMtj^~2vHoEkd;HiW87VY-A`?Mo?LGrySy9n@Ggh^TmP17Q7Dv}a?->lW}Kjv1WWg5 zZ-NkFmn(xPx7`-nTNI2JXzH|x64V_+LoiYQ`=V@;fCT_$+tUw9=2-`8ojnCilnt3@ zOuZ$UUnK3dgp}$q^0^~0bt5r}vh6e?g>~8aPjwNuP0FmDg^(zCbqy&Mnw_$*>i;!A zTl<^oIbNi)iFrv|v~5+r7{`Qqw88|_gH)U zwQ$@d+O@uCGHdp|!yxNdJo+yQLGF?0#tA_rnr=%eZ{QU3luGwBc_C@SN<<(ZD6!g83cB)Zj=?%ev z2~anlEJD`+UIk>N+j7!a%rJppsex*cf&?lGyVG81zf$%=C?-c7SK1?DaxzeiEwDWK zsw)ZM$`d#Rb6jAK3ZQ(L?6%2eg*P!!EQ$_L!f)fK)D?0>X*n5Gm0c&o;GzugwPcp~ zwA?&9{JcMWrQE?vJv$|eQ2s;}JlSgLZu8{j&@i!72?deNC&(nzXE1 zhhNTKzE_H9i>~WOn?Y$L+j8Qlh+TePB5uraM?!(_$22KP2~_RA1Vu_Ql{Ujpa0&(|Nr$Os@vQ4)8(y@%81NQe%!W>BnXfIxHQZ=i`IU* z5u}@vTZ*`vrwNY34A2r2*TYk{`yUA?N4##ZU5fwvplpo*1ps2%(>O_HTFVP+VFEgn z4Uuh3u_)PBA?@h&l9!4 zPrp%IAcDHoEHsmDOKGrTLSt?t2hyZ;Zg+~hBvmgcdM-K@RM(``4VqGu#^ z#uNev9x8O7D)Nmf*a<*nqLXq`rOUQF_+VLRAb+K4H0{!QUNW=Nh+>r)=RbiJB{Gxn z8q6zK5nLG~7Z~%oGGT67@_%|UTZwF|r@?cZ;Qm+C`woJu88&kQ%PR^|Ol4~*nckGP zXLu<*(S(8F7qE$X`3MoE(J^sYk2JDWyFTQPtcX9Qs`={pF*4UP*UU`vx9+hfV!EQ) z7=8XyN?{S85|E`GlJz@!i9KHRi2c~RzYzCvjf>nc{%W` z>IkE-QTJ!-ys!bs80<5HfkcB0oDGSOlSW*)C~t{PC=vPZ{xYuK$LH3^#SGHYf~6+gl7$fTn(33IDFs<_H#?1EvpQ>&i~6`X zk`_@pTXC*n;L`_)B!d8Y3OlHfaA@-Y!v>8QjVs#E%RO`@NK)2*b@uoF%IhnZkEKTy zC8OoH`SQ%1Inu5Gc_!qN(xoKCiLBEQeFK5!O57sFl*h9aIWqm@pH{*MJay#}?*HHA_C;4+ zfBTYY3tOzDE3?AZ#<~NSErYpF>G(cTD98aO(r!y}Qm~+5fiY8DhAamlCr~G6UopJ8 zapw~XBD1%P?m#Fc6PFMHB1DL(8cq)UPu8SWsK(v3&!TDn`=D%&fCQje+fy$|W=Y5k zYGC3_ls%tqti2_fB@(Umg_P>K_`QeJjY{Bd6YzFqn_Y2;F$6C!l?4zeveAUNy+vqu zDsnX?t|O@#Z0gO)-Rv@JQ81kPL|2Kl){&}JMvgf@tdl-FnXi)Yd88u|nGo$9gjkv? zL2Z7t=WTgMJhydsu(DB*0d1t)lG4BvjF|H*xnLLjj&S!b=}YQ-%M8^RWw=lRDtIMs z!ey>>ckuZ!5ncO`k%RK-^4uyRL@fR<_O0ImWdQJQX}mg0G65~5o0i-`X_bmaJ*W;+ zZt(ze$L`e(AIU=k&7($H!svE|W*pVp5poej^4p;H%Cx z;Ee@8fo=zj!Tl1$T(^lY;!F*64tFKsZTY8bvNSlBl$xlg9J@R zSy)@csfu?Xh)A^+RI%yH#`KLUsLe)-^bm$9fj~0CgTOFIO8@(yY@Gll1!G%lFEe&f zB}|#m z?;SX*dwzc#x{|53%%*Wz=1G52QGJZik}}J$JSTO(VK4LVNf{wARv1#3Q?SXD+-Itk z{O5=wfYOqhA5F0|*M!!RC?b#W+v!Sv8q-XL5FjR!ZMiLjE+W*Geg#?qfqKzREh?!p zQE1x4>H$~=II`M@JnxLehC}a^5Dpt`leOOizrAPio;G#^JrsyVNSBKxFnQ3)Lida{ zZ6SnPjQj!AvM91iwwb+OuZwSV5hC9_6af;My%O~{&Exw&@j+A$zhZVz^WqzBFH3p>b8X& zEvSVKYo2*Bu5PF95#}Ix!)D48nn;2vTCK7*-8oH@Y9Cb3QK+iB3GZ|Nm+E_(P`^rk zzt&Nh+9=yirsbqyagQX)bOv3K^}FpLLpk}A-O!0ziG`!B9|V@swf8}o_8=VLL;?uz z2pE|NRCxc}`4PI`^poj-m--d(v9*qlr9KJR?X{RnYn)ig-)4Gl?!R;TmY-crvQRd1df>jhG@)G=6 z@gDMXkZ|xZfrI&D)T5L$^$emaQcbxl2|5%{tjJp`p+ua{>+Nm~NtnY;Om3WBu@1B^ zbX|AlZ~OBU3hc?DqO#&x^>F1zSiRzKa*%hvl}{vPBq)vplz8%W0-OK)plqE01p8>) zQwd9SO~osUJt9n!y`yJLFwK!~9_?uM2{H?A?0yrJ6%cWMNYQ*fL9dJpz3QDgH39d7 zWDf|+7;sWf6dT7zzrLbaMI7_;*tPE0$Fhh0*E0>`{L}u$CV6+!RQ>(&e_9F(L}?`3 za#F~>$;}mLw4o=7@hC-_2H3M(rMFiH@ypTF>4WUO;}$9c!I+JPG#fQa1i%3X7>FR# z?513ZflXZ~N_3-sh}FEkb%KWu?vzvvBV_!ko%vIUU~X6$07Rsla#-k2Lt3H0CLlq{ zQR|PqUGQ<~pEwbDDLF^5q53~jEj~01{WZ`y)nFJw>6QcvlYsWRptvtgBzLmLU?5Wf zD1xe`X1Q7OG^$VX>yTG0o~ZLP>gVHJQU5qv%}5BQARA!R@Z!V_OW7yG^69txVvuNA zTJVqYh#aO1Da2TLDN}&wWAD4x?EH{KK zB6&fkgBfs;Jfe*NWnm)uQ2yQi`}O~-e1H9=?~C`GJ}iOn@?V{Txn5bi`a6Cl8boCz z+mhLVYL)4qF$AP2YI-A*lh{C`tuK45Rs>9`Msq#DmySqAVAnK^+=|g9dvwJNca;?FO`$R-3prmS%pS_;Ja4vG{98SK=ZW3JP!&lw+?29B-eAQEV=QI-wg zH`{Ed>AUUxUyLXGnOGt?I3Mtl8aB?TAci1$3{jqR6X$W=(Is^sIU>(jWaO|Il#9zB zo;lK~S8~VvP$r~EHMZMQ?$rVLDMHFjasw?|WTSCnl2OtU(43+ilN!Pkhn8jG{e(}1 zf?}!|Eg;CMR&*BV1%^Dr_^CY(#u=!iP7Y*~EV=n*YJ`E*-_6OtIsfypK@+T-!A}`I zL?Q==fTCD|V@MW!qS`;fB-N*%E?B7|e4yM?9O4*^(R=5bgc1nIFB>Wh8_S2>XZibh zZ=dw24isf1+HzK?$K^DV5kwXX&$g+P$VO2pI|NcDP0KV)MQy-g&PufzAI7$8SlOv$2aXQvU+a;N~b!glL}PdMwU&*EeCs>?oxizOypps9y)H@o_wMI`=D&E z00g*Z+3O54=uAi(T49AIl9hvHth~#qD-W$Hh9siP5f!HAX6+F&C9Cb_Atg&A>T&4J zI~wkF-js~dNi^K4v=F#f4_Z$rMBr5ucVt)>2xv$dMdi)bDE3QStI4cp!1r&i^kw~Y z|J-V_m^+;-#T702V%xA`AEWsu>SX`<&HcYX8j)!flX6s~1z1;7#ttnrl$eM%>{-|> zXB;aQMTvHYt!|k_y`A*R))>qt3a*wreN6-P58WIkJqe&@lQ0ki&@DI5?dv4Jd4r4Qn=?+YphooWj?(S zXZ!v}U0aXwJcXoCmo{tOZtZ^xm-25f-&7j_qEb!CUj?s6a#qotc@!wrTxdl2tC0&g zixv%^QU-^3uUW9$zT1r&5}8RqFsa=+b2o0i2y&{eQ?fc!rHZIWoj)wvRHgN*%iI2B zp!<1C8Oi@B#-s+4Ny#lFKw6OeS5iSF(0UxBRFz#ahgvLVPGcx5rZzx+L_&*P!$2bF z^otutHT9*JlpfyN#=T_rd-^zJY{N)}aaBd`n3RhmP`xNXD-f=kUg~2Lp0L6}_Yk>Q zn2LUF+6ra;8xcTmyqec$xjTw}dASFZLd-94ii2hvIhR&Og$|+NU5XsoeG{vFyA|7L9Gbnu)s~m3QeTkmfS(Z)Ph2W zDbChLNHYWYL_Nj(fH44J-xyVKxe1j~6qJ326oNBj}RP?!H zESboY-o_JI5&$F!FaJf{5A zdcqxb`;P%zW09{>l(e(+wfk@kWxk`l`c*JJP||Pf}P`p2D@)Ia;?k zz>NRkl#JQ$#CjVdgB9#iMIXy(^I;4sE5Rp(;|qsTH<5Mn`B9K#ZxL zg(cQMsekc~t#XrJx&^ALG;`Uw@B;!+`q4$wx&KqNP!hPRw)BI(saS3XLaHoc4x=y7 zKsCCr8}G~iJb3=C9Psbo6IVOSW-qlB`b(WDw!}g5GvMXU2IcCgfbG=dnnj#3YS5 zyv5FP?Z}j(MVanG{sa-V;+Yg)fp-j%rI1E}S!zm?ol4VPo03RB%U|{VQnQCjNl^P! zU0qU+5=4dV5Mo}dwO{R>IO3#E6!YkA)hxbND5U0+78sOs_?~=ZlXVaXO(L2~W{z>H zJba;8n;R_7W%Kv`D)R2G^Ap3zxMu^70EbsV7tFo`5hiW-&wrey{8RfkfBJcGU)26} zdIGYdh%uA8Clw4sRe?_=V)feEY(grcPA$`VjoEE^99N>$mdF zajC!4b4ozhKBGP|hbdy7NQh3&U5J+sdm`b{QkygB+w!|DKT>Zp=~)xuZ7X;F!nBLP zw?C9_!kx_6b_psJ+miB17Vvt_qFgGJI$RdG(pHlu<0*HMM%1imS=JuYTif>+6kz2R z3}!16O&KCdLBs?=nW@1Et(d@{DDD1a%L%RY$f(MDkS`t>x?wq_{&Dox;;|Pi$dMb? z3EL0uC-`b{B$AV;OzA?0CXc6?4TAGPY3jXnid6dd#6;+A4 zj({(*f(U>t7#taET-2WB)jNvn8avgc&!Gv>;e@53!i1{X;WaHY!Fb$|FX-7{-jcLy zcHn>VCS1EHjIN3&<7i&qZ=?{oWCLq8y;e{pW`{i?N39UH#2(u=!a7b(L#ffAVR;f) zfr_RQZdJbLE1ubPP_5Zjv}$YRs>|gi;*a%okE`+(33>P0UveG$iBIp}`fj_{(Ozvg mB$cp(mq{A``=V^mfCU|ASkn(mYEJ|!I$?+#6cw3kti3A?nyM`T literal 0 HcmV?d00001 diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index a4ceced4..c04eecd9 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -22,6 +22,7 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testAudioPropertiesXingHeaderVBR); CPPUNIT_TEST(testAudioPropertiesVBRIHeader); CPPUNIT_TEST(testAudioPropertiesNoVBRHeaders); + CPPUNIT_TEST(testSkipInvalidFrames); CPPUNIT_TEST(testVersion2DurationWithXingHeader); CPPUNIT_TEST(testSaveID3v24); CPPUNIT_TEST(testSaveID3v24WrongParam); @@ -102,6 +103,19 @@ public: CPPUNIT_ASSERT_EQUAL(209, lastHeader.frameLength()); } + void testSkipInvalidFrames() + { + 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(160, 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")); From 94f28b4fa1cddc6303caf08477c9b489dedb1c4c Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 15 Dec 2015 15:18:07 +0900 Subject: [PATCH 08/12] Update NEWS. --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 9a7a2933..8dc40132 100644 --- a/NEWS +++ b/NEWS @@ -11,7 +11,9 @@ * Better handling of duplicate tag chunks in WAV files. * Better handling of duplicate tag chunks in AIFF files. * Fixed crash when calling File::properties() after strip(). + * Fixed crash when parsing certain MPEG files. * Fixed possible file corruptions when saving ASF files. + * Fixed possible file corruptions when saving FLAC files. * Fixed updating the comment field of Vorbis comments. * Fixed reading date and time in ID3v2.3 tags. * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. From 63e838f831eb713901191732b7359bb21149acd6 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 15 Dec 2015 15:31:33 +0900 Subject: [PATCH 09/12] Make use of List iterators and setAutoDelete() in FLAC::File. --- taglib/flac/flacfile.cpp | 61 ++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 34 deletions(-) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index a41613e5..bdb77d52 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -45,6 +45,10 @@ using namespace TagLib; namespace { + typedef List BlockList; + typedef BlockList::Iterator BlockIterator; + typedef BlockList::Iterator BlockConstIterator; + enum { FlacXiphIndex = 0, FlacID3v2Index = 1, FlacID3v1Index = 2 }; const long MinPaddingLength = 4096; @@ -64,14 +68,13 @@ public: properties(0), flacStart(0), streamStart(0), - scanned(false) {} + scanned(false) + { + blocks.setAutoDelete(true); + } ~FilePrivate() { - unsigned int size = blocks.size(); - for(unsigned int i = 0; i < size; i++) { - delete blocks[i]; - } delete properties; } @@ -85,7 +88,7 @@ public: Properties *properties; ByteVector xiphCommentData; - List blocks; + BlockList blocks; long flacStart; long streamStart; @@ -173,35 +176,28 @@ bool FLAC::File::save() // Replace metadata blocks bool foundVorbisCommentBlock = false; - List newBlocks; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - MetadataBlock *block = d->blocks[i]; - if(block->code() == MetadataBlock::VorbisComment) { + + for(BlockIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + if((*it)->code() == MetadataBlock::VorbisComment) { // Set the new Vorbis Comment block - delete block; - block = new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData); + delete *it; + *it = new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData); foundVorbisCommentBlock = true; } - if(block->code() == MetadataBlock::Padding) { - delete block; - continue; - } - newBlocks.append(block); } + if(!foundVorbisCommentBlock) { - newBlocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); + d->blocks.append(new UnknownMetadataBlock(MetadataBlock::VorbisComment, d->xiphCommentData)); foundVorbisCommentBlock = true; } - d->blocks = newBlocks; // Render data for the metadata blocks ByteVector data; - for(unsigned int i = 0; i < newBlocks.size(); i++) { - FLAC::MetadataBlock *block = newBlocks[i]; - ByteVector blockData = block->render(); + for(BlockConstIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + ByteVector blockData = (*it)->render(); ByteVector blockHeader = ByteVector::fromUInt(blockData.size()); - blockHeader[0] = block->code(); + blockHeader[0] = (*it)->code(); data.append(blockHeader); data.append(blockData); } @@ -344,8 +340,8 @@ long FLAC::File::streamLength() List FLAC::File::pictureList() { List pictures; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - Picture *picture = dynamic_cast(d->blocks[i]); + for(BlockConstIterator it = d->blocks.begin(); it != d->blocks.end(); ++it) { + Picture *picture = dynamic_cast(*it); if(picture) { pictures.append(picture); } @@ -360,8 +356,7 @@ void FLAC::File::addPicture(Picture *picture) void FLAC::File::removePicture(Picture *picture, bool del) { - MetadataBlock *block = picture; - List::Iterator it = d->blocks.find(block); + BlockIterator it = d->blocks.find(picture); if(it != d->blocks.end()) d->blocks.erase(it); @@ -371,17 +366,15 @@ void FLAC::File::removePicture(Picture *picture, bool del) void FLAC::File::removePictures() { - List newBlocks; - for(unsigned int i = 0; i < d->blocks.size(); i++) { - Picture *picture = dynamic_cast(d->blocks[i]); - if(picture) { - delete picture; + for(BlockIterator it = d->blocks.begin(); it != d->blocks.end(); ) { + if(dynamic_cast(*it)) { + delete *it; + it = d->blocks.erase(it); } else { - newBlocks.append(d->blocks[i]); + ++it; } } - d->blocks = newBlocks; } bool FLAC::File::hasXiphComment() const From 5480458dfcc0747cb10532fb301e4b40afd20a12 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 16 Dec 2015 11:00:13 +0900 Subject: [PATCH 10/12] Change a method name of XiphComment to make it consistent with other method. --- taglib/ogg/xiphcomment.cpp | 2 +- taglib/ogg/xiphcomment.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index bb62069e..db46aecb 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -326,7 +326,7 @@ void Ogg::XiphComment::removePicture(FLAC::Picture *picture, bool del) delete picture; } -void Ogg::XiphComment::removePictures() +void Ogg::XiphComment::removeAllPictures() { d->pictureList.clear(); } diff --git a/taglib/ogg/xiphcomment.h b/taglib/ogg/xiphcomment.h index 055477da..14fc5407 100644 --- a/taglib/ogg/xiphcomment.h +++ b/taglib/ogg/xiphcomment.h @@ -245,7 +245,7 @@ namespace TagLib { /*! * Remove all pictures. */ - void removePictures(); + void removeAllPictures(); /*! * Add a new picture to the comment block. The comment block takes ownership of the From 42459fe45735f04b57df8cf3b53fc6ef8a50984a Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 16 Dec 2015 11:49:55 +0900 Subject: [PATCH 11/12] Update NEWS. --- NEWS | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS b/NEWS index 8dc40132..6d7c485c 100644 --- a/NEWS +++ b/NEWS @@ -14,6 +14,7 @@ * Fixed crash when parsing certain MPEG files. * Fixed possible file corruptions when saving ASF files. * Fixed possible file corruptions when saving FLAC files. + * Fixed possible file corruptions when saving MP4 files. * Fixed updating the comment field of Vorbis comments. * Fixed reading date and time in ID3v2.3 tags. * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. From 36b905670c127a4527d87cfea7ed2218bdaf2125 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 16 Dec 2015 12:59:43 +0900 Subject: [PATCH 12/12] Replace TagLib::uint with unsinged int. --- taglib/flac/flacfile.cpp | 2 +- taglib/toolkit/trefcounter.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index bdb77d52..2d65846b 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -225,7 +225,7 @@ bool FLAC::File::save() ByteVector paddingHeader = ByteVector::fromUInt(paddingLength); paddingHeader[0] = static_cast(MetadataBlock::Padding | LastBlockFlag); data.append(paddingHeader); - data.resize(static_cast(data.size() + paddingLength)); + data.resize(static_cast(data.size() + paddingLength)); // Write the data to the file diff --git a/taglib/toolkit/trefcounter.h b/taglib/toolkit/trefcounter.h index c231779d..bc3cdd9b 100644 --- a/taglib/toolkit/trefcounter.h +++ b/taglib/toolkit/trefcounter.h @@ -102,7 +102,7 @@ namespace TagLib bool deref() { return ! --refCount; } int count() { return refCount; } private: - uint refCount; + unsigned int refCount; #endif };