diff --git a/CMakeLists.txt b/CMakeLists.txt index ae30f7ad..6294f5fd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -89,9 +89,9 @@ endif() # 2. If any interfaces have been added, removed, or changed since the last update, increment current, and set revision to 0. # 3. If any interfaces have been added since the last public release, then increment age. # 4. If any interfaces have been removed since the last public release, then set age to 0. -set(TAGLIB_SOVERSION_CURRENT 16) -set(TAGLIB_SOVERSION_REVISION 1) -set(TAGLIB_SOVERSION_AGE 15) +set(TAGLIB_SOVERSION_CURRENT 17) +set(TAGLIB_SOVERSION_REVISION 0) +set(TAGLIB_SOVERSION_AGE 16) math(EXPR TAGLIB_SOVERSION_MAJOR "${TAGLIB_SOVERSION_CURRENT} - ${TAGLIB_SOVERSION_AGE}") math(EXPR TAGLIB_SOVERSION_MINOR "${TAGLIB_SOVERSION_AGE}") diff --git a/NEWS b/NEWS index a3e5c93a..787f2d3b 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,12 @@ +TagLib 1.11 (Jan 30, 2016) ========================== + * Better handling of PCM WAV files with a 'fact' chunk. + * Better handling of corrupted APE tags. + * Several smaller bug fixes and performance improvements. + +1.11 BETA: + * New API for creating FileRef from IOStream. * Added support for ID3v2 PCST and WFED frames. * Added support for pictures in XiphComment. @@ -8,6 +15,7 @@ * Added alternative functions to XiphComment::removeField(). * Added BUILD_BINDINGS build option. * Added ENABLE_CCACHE build option. + * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. * Better handling of duplicate ID3v2 tags in all kinds of files. * Better handling of duplicate tag chunks in WAV files. * Better handling of duplicate tag chunks in AIFF files. @@ -25,10 +33,10 @@ * Fixed possible file corruptions when saving WavPack 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 Ogg::Page::getCopyWithNewPageSequenceNumber() deprecated. It returns null. * Marked custom integer types deprecated. * Many smaller bug fixes and performance improvements. diff --git a/taglib/ape/apeitem.cpp b/taglib/ape/apeitem.cpp index 35236eca..661d2412 100644 --- a/taglib/ape/apeitem.cpp +++ b/taglib/ape/apeitem.cpp @@ -34,7 +34,9 @@ using namespace APE; class APE::Item::ItemPrivate { public: - ItemPrivate() : type(Text), readOnly(false) {} + ItemPrivate() : + type(Text), + readOnly(false) {} Item::ItemTypes type; String key; @@ -74,8 +76,9 @@ APE::Item::Item(const String &key, const ByteVector &value, bool binary) : d->type = Binary; d->value = value; } - else + else { d->text.append(value); + } } APE::Item::Item(const Item &item) : @@ -173,9 +176,8 @@ void APE::Item::appendValues(const StringList &values) int APE::Item::size() const { - // SFB: Why is d->key.size() used when size() returns the length in UniChars and not UTF-8? - size_t result = 8 + d->key.size() /* d->key.data(String::UTF8).size() */ + 1; - switch (d->type) { + size_t result = 8 + d->key.size() + 1; + switch(d->type) { case Text: if(!d->text.isEmpty()) { StringList::ConstIterator it = d->text.begin(); @@ -237,7 +239,10 @@ void APE::Item::parse(const ByteVector &data) const unsigned int valueLength = data.toUInt32LE(0); const unsigned int flags = data.toUInt32LE(4); - d->key = String(data.mid(8), String::UTF8); + // An item key can contain ASCII characters from 0x20 up to 0x7E, not UTF-8. + // We assume that the validity of the given key has been checked. + + d->key = String(&data[8], String::Latin1); const ByteVector value = data.mid(8 + d->key.size() + 1, valueLength); @@ -275,7 +280,7 @@ ByteVector APE::Item::render() const data.append(ByteVector::fromUInt32LE(value.size())); data.append(ByteVector::fromUInt32LE(flags)); - data.append(d->key.data(String::UTF8)); + data.append(d->key.data(String::Latin1)); data.append(ByteVector('\0')); data.append(value); diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index 4d592bb4..a8c181bf 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -36,6 +36,8 @@ #include #include #include +#include +#include #include "apetag.h" #include "apefooter.h" @@ -44,6 +46,32 @@ using namespace TagLib; using namespace APE; +namespace +{ + inline bool isKeyValid(const char *key, size_t length) + { + const char *invalidKeys[] = { "ID3", "TAG", "OGGS", "MP+", 0 }; + + if(length < 2 || length > 16) + return false; + + // only allow printable ASCII including space (32..126) + + for(const char *p = key; p < key + length; ++p) { + const int c = static_cast(*p); + if(c < 32 || c > 126) + return false; + } + + for(size_t i = 0; invalidKeys[i] != 0; ++i) { + if(Utils::equalsIgnoreCase(key, invalidKeys[i])) + return false; + } + + return true; + } +} + class APE::Tag::TagPrivate { public: @@ -362,16 +390,11 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) bool APE::Tag::checkKey(const String &key) { - if(key.size() < 2 || key.size() > 16) - return false; - for(String::ConstIterator it = key.begin(); it != key.end(); it++) - // only allow printable ASCII including space (32..127) - if (*it < 32 || *it >= 128) - return false; - String upperKey = key.upper(); - if (upperKey=="ID3" || upperKey=="TAG" || upperKey=="OGGS" || upperKey=="MP+") - return false; - return true; + if(!key.isLatin1()) + return false; + + const std::string data = key.to8Bit(false); + return isKeyValid(data.c_str(), data.size()); } APE::Footer *APE::Tag::footer() const @@ -393,31 +416,39 @@ void APE::Tag::addValue(const String &key, const String &value, bool replace) { if(replace) removeItem(key); - if(!key.isEmpty() && !value.isEmpty()) { - if(!replace && d->itemListMap.contains(key)) { - // Text items may contain more than one value - if(APE::Item::Text == d->itemListMap.begin()->second.type()) - d->itemListMap[key.upper()].appendValue(value); - // Binary or locator items may have only one value - else - setItem(key, Item(key, value)); - } - else - setItem(key, Item(key, value)); - } + + if(value.isEmpty()) + return; + + // Text items may contain more than one value. + // Binary or locator items may have only one value, hence always replaced. + + ItemListMap::Iterator it = d->itemListMap.find(key.upper()); + + if(it != d->itemListMap.end() && it->second.type() == Item::Text) + it->second.appendValue(value); + else + setItem(key, Item(key, value)); } void APE::Tag::setData(const String &key, const ByteVector &value) { removeItem(key); - if(!key.isEmpty() && !value.isEmpty()) - setItem(key, Item(key, value, true)); + + if(value.isEmpty()) + return; + + setItem(key, Item(key, value, true)); } void APE::Tag::setItem(const String &key, const Item &item) { - if(!key.isEmpty()) - d->itemListMap.insert(key.upper(), item); + if(!checkKey(key)) { + debug("APE::Tag::setItem() - Couldn't set an item due to an invalid key."); + return; + } + + d->itemListMap[key.upper()] = item; } bool APE::Tag::isEmpty() const @@ -469,14 +500,29 @@ void APE::Tag::parse(const ByteVector &data) if(data.size() < 11) return; - unsigned int pos = 0; + size_t pos = 0; for(unsigned int i = 0; i < d->footer.itemCount() && pos <= data.size() - 11; i++) { - APE::Item item; - item.parse(data.mid(pos)); - d->itemListMap.insert(item.key().upper(), item); + const size_t nullPos = data.find('\0', pos + 8); + if(nullPos == ByteVector::npos()) { + debug("APE::Tag::parse() - Couldn't find a key/value separator. Stopped parsing."); + return; + } - pos += item.size(); + const size_t keyLength = nullPos - pos - 8; + const size_t valLegnth = data.toUInt32LE(pos); + + if(isKeyValid(&data[pos + 8], keyLength)){ + APE::Item item; + item.parse(data.mid(pos)); + + d->itemListMap.insert(item.key().upper(), item); + } + else { + debug("APE::Tag::parse() - Skipped an item due to an invalid key."); + } + + pos += keyLength + valLegnth + 9; } } diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index e6c210ec..d2e528c7 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -299,27 +299,30 @@ void MPEG::Header::parse(File *file, long long offset, bool checkLength) if(d->data->isPadded) d->data->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; + // Check if the frame length has been calculated correctly, or the next frame + // header is right next to the end of this frame. + + // The MPEG versions, layers and sample rates of the two frames should be + // consistent. Otherwise, we assume that either or both of the frames are + // broken. file->seek(offset + d->data->frameLength); - const ByteVector nextSynch = file->readBlock(4); + const ByteVector nextData = file->readBlock(4); - for(int i = 0; i < static_cast(nextSynch.size()) - 1; ++i) { - if(firstSyncByte(nextSynch[i]) && secondSynchByte(nextSynch[i + 1])) { - nextFrameFound = true; - break; - } + if(nextData.size() < 4) { + debug("MPEG::Header::parse() -- Could not read the next frame header."); + return; } - if(!nextFrameFound) { - debug("MPEG::Header::parse() -- Calculated frame length did not match the actual length."); + const unsigned int HeaderMask = 0xfffe0c00; + + const unsigned int header = data.toUInt32BE(0) & HeaderMask; + const unsigned int nextHeader = nextData.toUInt32BE(0) & HeaderMask; + + if(header != nextHeader) { + debug("MPEG::Header::parse() -- The next frame was not consistent with this frame."); return; } } diff --git a/taglib/mpeg/mpegutils.h b/taglib/mpeg/mpegutils.h index 78cee280..5a7e0e6a 100644 --- a/taglib/mpeg/mpegutils.h +++ b/taglib/mpeg/mpegutils.h @@ -48,7 +48,9 @@ namespace TagLib inline bool secondSynchByte(unsigned char byte) { - return ((byte & 0xE0) == 0xE0); + // 0xFF is possible in theory, but it's very unlikely be a header. + + return (byte != 0xFF && ((byte & 0xE0) == 0xE0)); } } diff --git a/taglib/ogg/oggpage.cpp b/taglib/ogg/oggpage.cpp index 2bf7fa5e..911cc7ca 100644 --- a/taglib/ogg/oggpage.cpp +++ b/taglib/ogg/oggpage.cpp @@ -269,27 +269,10 @@ List Ogg::Page::paginate(const ByteVectorList &packets, return l; } -Ogg::Page* Ogg::Page::getCopyWithNewPageSequenceNumber(int sequenceNumber) +Ogg::Page* Ogg::Page::getCopyWithNewPageSequenceNumber(int /*sequenceNumber*/) { - Page *pResultPage = NULL; - - // TODO: a copy constructor would be helpful - - if(d->file == 0) { - pResultPage = new Page( - d->packets, - d->header.streamSerialNumber(), - sequenceNumber, - d->header.firstPacketContinued(), - d->header.lastPacketCompleted(), - d->header.lastPageOfStream()); - } - else - { - pResultPage = new Page(d->file, d->fileOffset); - pResultPage->d->header.setPageSequenceNumber(sequenceNumber); - } - return pResultPage; + debug("Ogg::Page::getCopyWithNewPageSequenceNumber() -- This function is obsolete. Returning null."); + return 0; } //////////////////////////////////////////////////////////////////////////////// diff --git a/taglib/ogg/oggpage.h b/taglib/ogg/oggpage.h index cc633180..a8c689b8 100644 --- a/taglib/ogg/oggpage.h +++ b/taglib/ogg/oggpage.h @@ -91,7 +91,7 @@ namespace TagLib { * \see header() * \see PageHeader::setPageSequenceNumber() * - * \deprecated + * \deprecated Always returns null. */ Page* getCopyWithNewPageSequenceNumber(int sequenceNumber); diff --git a/taglib/riff/wav/wavproperties.cpp b/taglib/riff/wav/wavproperties.cpp index 97d6aea7..4eb1c931 100644 --- a/taglib/riff/wav/wavproperties.cpp +++ b/taglib/riff/wav/wavproperties.cpp @@ -180,7 +180,7 @@ void RIFF::WAV::AudioProperties::read(File *file) d->sampleRate = data.toUInt32LE(4); d->bitsPerSample = data.toUInt16LE(14); - if(totalSamples > 0) + if(d->format != FORMAT_PCM) d->sampleFrames = totalSamples; else if(d->channels > 0 && d->bitsPerSample > 0) d->sampleFrames = streamLength / (d->channels * ((d->bitsPerSample + 7) / 8)); diff --git a/taglib/toolkit/taglib.h b/taglib/toolkit/taglib.h index da0d3e35..2f4c5655 100644 --- a/taglib/toolkit/taglib.h +++ b/taglib/toolkit/taglib.h @@ -27,7 +27,7 @@ #define TAGLIB_H #define TAGLIB_MAJOR_VERSION 1 -#define TAGLIB_MINOR_VERSION 10 +#define TAGLIB_MINOR_VERSION 11 #define TAGLIB_PATCH_VERSION 0 #if (defined(_MSC_VER) && _MSC_VER >= 1600) diff --git a/taglib/toolkit/tutils.h b/taglib/toolkit/tutils.h index 3d2c5488..d7adee29 100644 --- a/taglib/toolkit/tutils.h +++ b/taglib/toolkit/tutils.h @@ -219,6 +219,23 @@ namespace TagLib return String(); } + /*! + * Returns whether the two strings s1 and s2 are equal, ignoring the case of + * the characters. + * + * We took the trouble to define this one here, since there are some + * incompatible variations of case insensitive strcmp(). + */ + inline bool equalsIgnoreCase(const char *s1, const char *s2) + { + while(*s1 != '\0' && *s2 != '\0' && ::tolower(*s1) == ::tolower(*s2)) { + s1++; + s2++; + } + + return (*s1 == '\0' && *s2 == '\0'); + } + /*! * Returns the integer byte order of the system. */ diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames1.mp3 similarity index 100% rename from tests/data/invalid-frames.mp3 rename to tests/data/invalid-frames1.mp3 diff --git a/tests/data/invalid-frames3.mp3 b/tests/data/invalid-frames3.mp3 new file mode 100644 index 00000000..6bbd2d39 Binary files /dev/null and b/tests/data/invalid-frames3.mp3 differ diff --git a/tests/data/pcm_with_fact_chunk.wav b/tests/data/pcm_with_fact_chunk.wav new file mode 100644 index 00000000..a6dc1d6c Binary files /dev/null and b/tests/data/pcm_with_fact_chunk.wav differ diff --git a/tests/test_apetag.cpp b/tests/test_apetag.cpp index 81bfe8cf..e3391821 100644 --- a/tests/test_apetag.cpp +++ b/tests/test_apetag.cpp @@ -97,6 +97,11 @@ public: CPPUNIT_ASSERT_EQUAL((size_t)2, unsuccessful.size()); CPPUNIT_ASSERT(unsuccessful.contains("A")); CPPUNIT_ASSERT(unsuccessful.contains("MP+")); + + CPPUNIT_ASSERT_EQUAL((unsigned int)2, tag.itemListMap().size()); + tag.addValue("VALID KEY", "Test Value 1"); + tag.addValue("INVALID KEY \x7f", "Test Value 2"); + CPPUNIT_ASSERT_EQUAL((unsigned int)3, tag.itemListMap().size()); } void testTextBinary() diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index 5b58efcb..72398146 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -24,6 +24,7 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testAudioPropertiesNoVBRHeaders); CPPUNIT_TEST(testSkipInvalidFrames1); CPPUNIT_TEST(testSkipInvalidFrames2); + CPPUNIT_TEST(testSkipInvalidFrames3); CPPUNIT_TEST(testVersion2DurationWithXingHeader); CPPUNIT_TEST(testSaveID3v24); CPPUNIT_TEST(testSaveID3v24WrongParam); @@ -107,7 +108,7 @@ public: void testSkipInvalidFrames1() { - MPEG::File f(TEST_FILE_PATH_C("invalid-frames.mp3")); + MPEG::File f(TEST_FILE_PATH_C("invalid-frames1.mp3")); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); @@ -131,6 +132,19 @@ public: CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); } + void testSkipInvalidFrames3() + { + MPEG::File f(TEST_FILE_PATH_C("invalid-frames3.mp3")); + CPPUNIT_ASSERT(f.audioProperties()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(176, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(320, 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")); diff --git a/tests/test_wav.cpp b/tests/test_wav.cpp index 96891877..ade4f3f7 100644 --- a/tests/test_wav.cpp +++ b/tests/test_wav.cpp @@ -25,6 +25,7 @@ class TestWAV : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST(testFuzzedFile2); CPPUNIT_TEST(testStripAndProperties); + CPPUNIT_TEST(testPCMWithFactChunk); CPPUNIT_TEST_SUITE_END(); public: @@ -247,6 +248,22 @@ public: } } + void testPCMWithFactChunk() + { + RIFF::WAV::File f(TEST_FILE_PATH_C("pcm_with_fact_chunk.wav")); + CPPUNIT_ASSERT(f.audioProperties()); + CPPUNIT_ASSERT_EQUAL(3, f.audioProperties()->length()); + CPPUNIT_ASSERT_EQUAL(3, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(3675, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(32, f.audioProperties()->bitrate()); + CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels()); + CPPUNIT_ASSERT_EQUAL(1000, f.audioProperties()->sampleRate()); + CPPUNIT_ASSERT_EQUAL(16, f.audioProperties()->bitsPerSample()); + CPPUNIT_ASSERT_EQUAL(16, f.audioProperties()->sampleWidth()); + CPPUNIT_ASSERT_EQUAL(3675U, f.audioProperties()->sampleFrames()); + CPPUNIT_ASSERT_EQUAL(1, f.audioProperties()->format()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestWAV);