From 2651372291df9106ff2c63c1e356969239fe31b4 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 9 Nov 2016 15:51:33 +0900 Subject: [PATCH 01/24] Separate some tests to make them more specific. --- tests/test_list.cpp | 28 ++++++++++++++++++++-------- tests/test_map.cpp | 18 ++++++++++++++---- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/tests/test_list.cpp b/tests/test_list.cpp index e314836a..1c6d8c4c 100644 --- a/tests/test_list.cpp +++ b/tests/test_list.cpp @@ -32,12 +32,13 @@ using namespace TagLib; class TestList : public CppUnit::TestFixture { CPPUNIT_TEST_SUITE(TestList); - CPPUNIT_TEST(testList); + CPPUNIT_TEST(testAppend); + CPPUNIT_TEST(testDetach); CPPUNIT_TEST_SUITE_END(); public: - void testList() + void testAppend() { List l1; List l2; @@ -51,14 +52,25 @@ public: l3.append(2); l3.append(3); l3.append(4); + CPPUNIT_ASSERT_EQUAL(4U, l1.size()); CPPUNIT_ASSERT(l1 == l3); - - List l4 = l1; - List::Iterator it = l4.find(3); - *it = 33; - CPPUNIT_ASSERT_EQUAL(l1[2], 3); - CPPUNIT_ASSERT_EQUAL(l4[2], 33); } + + void testDetach() + { + List l1; + l1.append(1); + l1.append(2); + l1.append(3); + l1.append(4); + + List l2 = l1; + List::Iterator it = l2.find(3); + *it = 33; + CPPUNIT_ASSERT_EQUAL(3, l1[2]); + CPPUNIT_ASSERT_EQUAL(33, l2[2]); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestList); diff --git a/tests/test_map.cpp b/tests/test_map.cpp index 3096ff69..b5e493b6 100644 --- a/tests/test_map.cpp +++ b/tests/test_map.cpp @@ -34,6 +34,7 @@ class TestMap : public CppUnit::TestFixture { CPPUNIT_TEST_SUITE(TestMap); CPPUNIT_TEST(testInsert); + CPPUNIT_TEST(testDetach); CPPUNIT_TEST_SUITE_END(); public: @@ -42,19 +43,28 @@ public: { Map m1; m1.insert("foo", 3); + m1.insert("bar", 5); + CPPUNIT_ASSERT_EQUAL(2U, m1.size()); CPPUNIT_ASSERT_EQUAL(3, m1["foo"]); + CPPUNIT_ASSERT_EQUAL(5, m1["bar"]); m1.insert("foo", 7); + CPPUNIT_ASSERT_EQUAL(2U, m1.size()); CPPUNIT_ASSERT_EQUAL(7, m1["foo"]); + CPPUNIT_ASSERT_EQUAL(5, m1["bar"]); + } - m1.insert("alice", 5); - m1.insert("bob", 9); + void testDetach() + { + Map m1; + m1.insert("alice", 5); + m1.insert("bob", 9); m1.insert("carol", 11); Map m2 = m1; Map::Iterator it = m2.find("bob"); (*it).second = 99; - CPPUNIT_ASSERT_EQUAL(m1["bob"], 9); - CPPUNIT_ASSERT_EQUAL(m2["bob"], 99); + CPPUNIT_ASSERT_EQUAL(9, m1["bob"]); + CPPUNIT_ASSERT_EQUAL(99, m2["bob"]); } }; From 7b8d576bdee3b7304806b3008bfde30baa3d03a6 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 10 Nov 2016 17:09:40 +0900 Subject: [PATCH 02/24] Don't decode redundant UTF-8 sequences in Win32. Linux and OS X are working well and won't be affected. --- taglib/toolkit/tstring.cpp | 2 +- tests/test_string.cpp | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index a1891a45..44788249 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -88,7 +88,7 @@ namespace #ifdef _WIN32 len = ::MultiByteToWideChar( - CP_UTF8, 0, src, static_cast(srcLength), dst, static_cast(dstLength)); + CP_UTF8, MB_ERR_INVALID_CHARS, src, static_cast(srcLength), dst, static_cast(dstLength)); #else diff --git a/tests/test_string.cpp b/tests/test_string.cpp index f9f1a2dd..599e002d 100644 --- a/tests/test_string.cpp +++ b/tests/test_string.cpp @@ -50,6 +50,7 @@ class TestString : public CppUnit::TestFixture CPPUNIT_TEST(testEncodeNonLatin1); CPPUNIT_TEST(testEncodeEmpty); CPPUNIT_TEST(testIterator); + CPPUNIT_TEST(testRedundantUTF8); CPPUNIT_TEST_SUITE_END(); public: @@ -321,6 +322,14 @@ public: CPPUNIT_ASSERT_EQUAL(L'I', *it2); } + void testRedundantUTF8() + { + CPPUNIT_ASSERT_EQUAL(String("/"), String(ByteVector("\x2F"), String::UTF8)); + CPPUNIT_ASSERT(String(ByteVector("\xC0\xAF"), String::UTF8).isEmpty()); + CPPUNIT_ASSERT(String(ByteVector("\xE0\x80\xAF"), String::UTF8).isEmpty()); + CPPUNIT_ASSERT(String(ByteVector("\xF0\x80\x80\xAF"), String::UTF8).isEmpty()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestString); From f9a747dceb694e7adcf65705f2b784ba806e7301 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 10 Nov 2016 20:02:30 +0900 Subject: [PATCH 03/24] Avoid adding fields with invalid keys to Vorbis Comments. According to the spec, '\x7F' is not allowed. --- taglib/ogg/xiphcomment.cpp | 21 ++++++++++++++++----- tests/test_xiphcomment.cpp | 20 +++++++++++++++++--- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index db46aecb..d6b8e11c 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -261,10 +261,14 @@ bool Ogg::XiphComment::checkKey(const String &key) { if(key.size() < 1) return false; - for(String::ConstIterator it = key.begin(); it != key.end(); it++) - // forbid non-printable, non-ascii, '=' (#61) and '~' (#126) - if (*it < 32 || *it >= 128 || *it == 61 || *it == 126) + + // A key may consist of ASCII 0x20 through 0x7D, 0x3D ('=') excluded. + + for(String::ConstIterator it = key.begin(); it != key.end(); it++) { + if(*it < 0x20 || *it > 0x7D || *it == 0x3D) return false; + } + return true; } @@ -275,11 +279,18 @@ String Ogg::XiphComment::vendorID() const void Ogg::XiphComment::addField(const String &key, const String &value, bool replace) { + if(!checkKey(key)) { + debug("Ogg::XiphComment::addField() - Invalid key. Field not added."); + return; + } + + const String upperKey = key.upper(); + if(replace) - removeFields(key.upper()); + removeFields(upperKey); if(!key.isEmpty() && !value.isEmpty()) - d->fieldListMap[key.upper()].append(value); + d->fieldListMap[upperKey].append(value); } void Ogg::XiphComment::removeField(const String &key, const String &value) diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index 699f60a5..d2a28a1f 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -42,7 +42,8 @@ class TestXiphComment : public CppUnit::TestFixture CPPUNIT_TEST(testSetYear); CPPUNIT_TEST(testTrack); CPPUNIT_TEST(testSetTrack); - CPPUNIT_TEST(testInvalidKeys); + CPPUNIT_TEST(testInvalidKeys1); + CPPUNIT_TEST(testInvalidKeys2); CPPUNIT_TEST(testClearComment); CPPUNIT_TEST(testRemoveFields); CPPUNIT_TEST(testPicture); @@ -90,19 +91,32 @@ public: CPPUNIT_ASSERT_EQUAL(String("3"), cmt.fieldListMap()["TRACKNUMBER"].front()); } - void testInvalidKeys() + void testInvalidKeys1() { PropertyMap map; map[""] = String("invalid key: empty string"); map["A=B"] = String("invalid key: contains '='"); map["A~B"] = String("invalid key: contains '~'"); + map["A\x7F\B"] = String("invalid key: contains '\x7F'"); + map[L"A\x3456\B"] = String("invalid key: Unicode"); Ogg::XiphComment cmt; PropertyMap unsuccessful = cmt.setProperties(map); - CPPUNIT_ASSERT_EQUAL((unsigned int)3, unsuccessful.size()); + CPPUNIT_ASSERT_EQUAL((unsigned int)5, unsuccessful.size()); CPPUNIT_ASSERT(cmt.properties().isEmpty()); } + void testInvalidKeys2() + { + Ogg::XiphComment cmt; + cmt.addField("", "invalid key: empty string"); + cmt.addField("A=B", "invalid key: contains '='"); + cmt.addField("A~B", "invalid key: contains '~'"); + cmt.addField("A\x7F\B", "invalid key: contains '\x7F'"); + cmt.addField(L"A\x3456\B", "invalid key: Unicode"); + CPPUNIT_ASSERT_EQUAL(0U, cmt.fieldCount()); + } + void testClearComment() { ScopedFileCopy copy("empty", ".ogg"); From b98a984b66f69cacfcbf259ac3b2d77ea3c9415f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 11 Nov 2016 00:07:32 +0900 Subject: [PATCH 04/24] Fix handling of lowercase 'metadata_block_picture' fields in Vorbis comments. Also refactored some redundant code for parsing pictures. --- taglib/ogg/xiphcomment.cpp | 122 ++++++++++++++------------------ tests/data/lowercase-fields.ogg | Bin 0 -> 4477 bytes tests/test_xiphcomment.cpp | 18 +++++ 3 files changed, 71 insertions(+), 69 deletions(-) create mode 100644 tests/data/lowercase-fields.ogg diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index d6b8e11c..f56bf810 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -447,85 +447,69 @@ void Ogg::XiphComment::parse(const ByteVector &data) const unsigned int commentLength = data.toUInt(pos, false); pos += 4; - ByteVector entry = data.mid(pos, commentLength); - + const ByteVector entry = data.mid(pos, commentLength); pos += commentLength; // Don't go past data end + if(pos > data.size()) break; - // Handle Pictures separately - if(entry.startsWith("METADATA_BLOCK_PICTURE=")) { - - // We need base64 encoded data including padding - if((entry.size() - 23) > 3 && ((entry.size() - 23) % 4) == 0) { - - // Decode base64 picture data - ByteVector picturedata = ByteVector::fromBase64(entry.mid(23)); - if(picturedata.size()) { - - // Decode Flac Picture - FLAC::Picture * picture = new FLAC::Picture(); - if(picture->parse(picturedata)) { - - d->pictureList.append(picture); - - // continue to next field - continue; - } - else { - delete picture; - debug("Failed to decode FlacPicture block"); - } - } - else { - debug("Failed to decode base64 encoded data"); - } - } - else { - debug("Invalid base64 encoded data"); - } - } - - // Handle old picture standard - if(entry.startsWith("COVERART=")) { - - if((entry.size() - 9) > 3 && ((entry.size() - 9) % 4) == 0) { - - // Decode base64 picture data - ByteVector picturedata = ByteVector::fromBase64(entry.mid(9)); - if (picturedata.size()) { - - // Assume it's some type of image file - FLAC::Picture * picture = new FLAC::Picture(); - picture->setData(picturedata); - picture->setMimeType("image/"); - picture->setType(FLAC::Picture::Other); - d->pictureList.append(picture); - - // continue to next field - continue; - } - else { - debug("Failed to decode base64 encoded data"); - } - } - else { - debug("Invalid base64 encoded data"); - } - } - // Check for field separator - int sep = entry.find('='); + + const int sep = entry.find('='); if(sep < 1) { - debug("Discarding invalid comment field."); + debug("Ogg::XiphComment::parse() - Discarding a field. Separator not found."); continue; } - // Parse key and value - String key = String(entry.mid(0, sep), String::UTF8); - String value = String(entry.mid(sep + 1), String::UTF8); - addField(key, value, false); + // Parse the key + + const String key = String(entry.mid(0, sep), String::UTF8).upper(); + if(!checkKey(key)) { + debug("Ogg::XiphComment::parse() - Discarding a field. Invalid key."); + continue; + } + + if(key == "METADATA_BLOCK_PICTURE" || key == "COVERART") { + + // Handle Pictures separately + + const ByteVector picturedata = ByteVector::fromBase64(entry.mid(sep + 1)); + if(picturedata.isEmpty()) { + debug("Ogg::XiphComment::parse() - Discarding a field. Invalid base64 data"); + continue; + } + + if(key[0] == L'M') { + + // Decode FLAC Picture + + FLAC::Picture * picture = new FLAC::Picture(); + if(picture->parse(picturedata)) { + d->pictureList.append(picture); + } + else { + delete picture; + debug("Ogg::XiphComment::parse() - Failed to decode FLAC Picture block"); + } + } + else { + + // Assume it's some type of image file + + FLAC::Picture * picture = new FLAC::Picture(); + picture->setData(picturedata); + picture->setMimeType("image/"); + picture->setType(FLAC::Picture::Other); + d->pictureList.append(picture); + } + } + else { + + // Parse the text + + addField(key, String(entry.mid(sep + 1), String::UTF8), false); + } } } diff --git a/tests/data/lowercase-fields.ogg b/tests/data/lowercase-fields.ogg new file mode 100644 index 0000000000000000000000000000000000000000..0ddd49357a906c70d6f3e5f222f0fa5404e4e2cb GIT binary patch literal 4477 zcmeHLeNatHl+?=l0qu3&|>lZIIRhca`BP{0# z{ezHojK%z(U=`3a9~62fJ@Qrimjm3>|NMSg5gyXC76eMnjddFenrej&=Bi>wzfdF0 zkjXM-8)fov5Qu}PvZ>i@Y2Iv5>kUE`WzgsiwZPqDYObuQY_5Evs^O*T`WG6_)y*wU zrp+oSB;BYQ8>_bGzF4ML?5)l#ev#&E*`C=@(?*8)MKs4qH+giUrW;i?-DvdZ4 z)|Zx0br_gxHa9nzLiPV0XbQJJCWSR zZ5L#0E1wWsdPKfH(ovqef2zlb=n1c=T6uP-iX*a%H(lf!TAxvO3td%2!vRO19JIJUw-$u`7XTF zC1&cvt3m0+C{r_->G=p17R9Ld@>VeOY7pY|WjXyNPJf+q(b3JZA2+eu^IEOp-6`;JTa2-11!VJb;{0I?}KdRYhwqZh817+ z&i9qZdbJ~pwY~XcZLYVSA1M}lEtBULdebL?8t5&J@q*l9@0q^R=n$1M(l+Il(7e5q zH+J@N$AMZu+}7iLZ49W0VRN^4>j+T!AGJ|$Z>4z_S{2jWJbOdX8&7u!fB%u^?Wwas z6^!5bvKMx8-7s-%n?s_&L$QAxI5^5*nXozK0ABR;s7 z(x!v}4?{|hsDnr4-*2iMq67S<3&gv9?OMsu;6PW#g!KK3Otb0sk@sVti%CC&ZFy$+ z45ih{ z>?wNIG@DtWf40I{yl1;-PxYyXUi9xW*+myK37!gKv3nymd$tWJC}~i_paxXmt*?ItN?3mmIFTw>*PG z-e)cZZl64Q>xb$}=78f6P4K)XJdeWF6c7x$))*RH!N=*ix=A`QBM3)zN;B%yE>r5udgBij-K<;}55PvD9v14s^l541-l$HzYG(I) zebq74ZU0EVq;~AX3+WyFv7aZs;x58|J%m>~RTtBIQ^fk=IRn|j^SxOuNuXjWa9#(( zysYbu0_CU_DuWC{J@Qpk*{hGqS1+tLp@sdW9 zks}^8=EvbtF7~J4`JM=!E$~M<%Vko{_L*h6hgje+pqdprkp>2)Tn9g*td)h?geo8rVZv)Khavv;xWkool8Y#1}yZT=5l5`%Mwy#{R>=!)~W=%!t6hFYzUy!XA`LLvM+|DD{i~TcHQ0Sv5BEfFZ zO2miV)NaCvQN)ltnv{qSQV3n$9}Z7PVcj_`60Sy8SuXw2em>d9tDaP(a4Bc{V(;tP z^aV(RG(;kN%@=)cCgq{DG*3bJnY_nMX(3a3UQ3AWld2TIdj|5Oc!UfL+K10Au-B;= zxG5Hj>8_^X2BbVVP0V7(^ffZ+N2cpd~d($EJbR}vsE&>4*tqf!RTnqX^w$uRwiaPZsB85lJhh@)|Myy;SmT zJ&=-{Ffv+XcT*gAD;_!iaVQ^-eAmbvolwmwYoP(kdXecqY$$s4VfR0x(jYlwGPFmU z2=otQ7P|MdtH(B-)=Zb|1dp<;aNJMv{d`LdN%##M@IbSntaF5Pq%Y#W&&nFkS|(Mg zqJwi)(nFS5k|53&R~Hd>iqfVXFX`0wA9hjN;majm=|jG`vd-c0sxA%j!DnTi>1)hp0*60`767+ug4lG8TAb40%(~ zvW8A^8eDt0K}U~O3X=4iNxE#j!8v3*vypttek_?KEwuX=upkhBOvFG8!( z9a!KF#N|$3tgNYPXl!nM*=l!m^+14xl89c0o2yuvlOwnA6O*14tbOWfp(uGB4q-g> sCxFn3*z8b7MMrQpo4Mt)PyH$s&*@t}P3`*azVd$7kG-Ji*dKTK7v71uJOBUy literal 0 HcmV?d00001 diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index d2a28a1f..64ae0b21 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -47,6 +47,7 @@ class TestXiphComment : public CppUnit::TestFixture CPPUNIT_TEST(testClearComment); CPPUNIT_TEST(testRemoveFields); CPPUNIT_TEST(testPicture); + CPPUNIT_TEST(testLowercaseFields); CPPUNIT_TEST_SUITE_END(); public: @@ -192,6 +193,23 @@ public: } } + void testLowercaseFields() + { + const ScopedFileCopy copy("lowercase-fields", ".ogg"); + { + Vorbis::File f(copy.fileName().c_str()); + List lst = f.tag()->pictureList(); + CPPUNIT_ASSERT_EQUAL(String("TEST TITLE"), f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(String("TEST ARTIST"), f.tag()->artist()); + CPPUNIT_ASSERT_EQUAL((unsigned int)1, lst.size()); + f.save(); + } + { + Vorbis::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.find("METADATA_BLOCK_PICTURE") > 0); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestXiphComment); From d5b9d7b8a79ba77e7b803ffa2a04781b8c06de86 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 18 Nov 2016 13:55:43 +0900 Subject: [PATCH 05/24] Update NEWS. --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 3c74bc45..1a185342 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,8 @@ * Added support for classical music tags of iTunes 12.5. * Dropped support for Windows 9x and NT 4.0 or older. * Fixed reading MP4 atoms with zero length. + * Fixed handling of redundant UTF-8 sequences in Win32. + * Fixed handling of lowercase field names in Vorbis Comments. * Several smaller bug fixes and performance improvements. TagLib 1.11.1 (Oct 24, 2016) From eff28c55bf6cc3437dcfbc96449e3d2904370a74 Mon Sep 17 00:00:00 2001 From: mathbunnyru Date: Tue, 22 Nov 2016 01:10:28 +0300 Subject: [PATCH 06/24] Increment fixes --- taglib/toolkit/tstring.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index 44788249..b81b084e 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -508,7 +508,7 @@ ByteVector String::data(Type t) const ByteVector v(size(), 0); char *p = v.data(); - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) *p++ = static_cast(*it); return v; @@ -537,7 +537,7 @@ ByteVector String::data(Type t) const *p++ = '\xff'; *p++ = '\xfe'; - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) { + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) { *p++ = static_cast(*it & 0xff); *p++ = static_cast(*it >> 8); } @@ -549,7 +549,7 @@ ByteVector String::data(Type t) const ByteVector v(size() * 2, 0); char *p = v.data(); - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) { + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) { *p++ = static_cast(*it >> 8); *p++ = static_cast(*it & 0xff); } @@ -561,7 +561,7 @@ ByteVector String::data(Type t) const ByteVector v(size() * 2, 0); char *p = v.data(); - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) { + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) { *p++ = static_cast(*it & 0xff); *p++ = static_cast(*it >> 8); } @@ -611,7 +611,7 @@ String String::stripWhiteSpace() const bool String::isLatin1() const { - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) { + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) { if(*it >= 256) return false; } @@ -620,7 +620,7 @@ bool String::isLatin1() const bool String::isAscii() const { - for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); it++) { + for(wstring::const_iterator it = d->data.begin(); it != d->data.end(); ++it) { if(*it >= 128) return false; } From 5e1d9fad31b65bd798381b41be7268dea919a920 Mon Sep 17 00:00:00 2001 From: mathbunnyru Date: Thu, 24 Nov 2016 02:02:38 +0300 Subject: [PATCH 07/24] Small fixes --- taglib/ape/apetag.cpp | 4 ++-- taglib/asf/asfattribute.h | 2 +- taglib/mp4/mp4tag.cpp | 4 ++-- taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp | 4 +++- taglib/mpeg/id3v2/id3v2tag.cpp | 5 +++-- taglib/xm/xmfile.cpp | 2 +- 6 files changed, 12 insertions(+), 9 deletions(-) diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index c31237eb..92ad536f 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -191,7 +191,7 @@ void APE::Tag::setGenre(const String &s) void APE::Tag::setYear(unsigned int i) { - if(i <= 0) + if(i == 0) removeItem("YEAR"); else addValue("YEAR", String::number(i), true); @@ -199,7 +199,7 @@ void APE::Tag::setYear(unsigned int i) void APE::Tag::setTrack(unsigned int i) { - if(i <= 0) + if(i == 0) removeItem("TRACK"); else addValue("TRACK", String::number(i), true); diff --git a/taglib/asf/asfattribute.h b/taglib/asf/asfattribute.h index 64979216..1738eb45 100644 --- a/taglib/asf/asfattribute.h +++ b/taglib/asf/asfattribute.h @@ -113,7 +113,7 @@ namespace TagLib /*! * Copies the contents of \a other into this item. */ - ASF::Attribute &operator=(const Attribute &other); + Attribute &operator=(const Attribute &other); /*! * Exchanges the content of the Attribute by the content of \a other. diff --git a/taglib/mp4/mp4tag.cpp b/taglib/mp4/mp4tag.cpp index 0c2e5cbc..a7a473a2 100644 --- a/taglib/mp4/mp4tag.cpp +++ b/taglib/mp4/mp4tag.cpp @@ -948,10 +948,10 @@ PropertyMap MP4::Tag::setProperties(const PropertyMap &props) if(reverseKeyMap.contains(it->first)) { String name = reverseKeyMap[it->first]; if((it->first == "TRACKNUMBER" || it->first == "DISCNUMBER") && !it->second.isEmpty()) { - int first = 0, second = 0; StringList parts = StringList::split(it->second.front(), "/"); if(!parts.isEmpty()) { - first = parts[0].toInt(); + int first = parts[0].toInt(); + int second = 0; if(parts.size() > 1) { second = parts[1].toInt(); } diff --git a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp index f7971250..b554d2cd 100644 --- a/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp +++ b/taglib/mpeg/id3v2/frames/tableofcontentsframe.cpp @@ -36,7 +36,9 @@ class TableOfContentsFrame::TableOfContentsFramePrivate { public: TableOfContentsFramePrivate() : - tagHeader(0) + tagHeader(0), + isTopLevel(false), + isOrdered(false) { embeddedFrameList.setAutoDelete(true); } diff --git a/taglib/mpeg/id3v2/id3v2tag.cpp b/taglib/mpeg/id3v2/id3v2tag.cpp index 4c00ab6f..810b2aef 100644 --- a/taglib/mpeg/id3v2/id3v2tag.cpp +++ b/taglib/mpeg/id3v2/id3v2tag.cpp @@ -60,6 +60,7 @@ class ID3v2::Tag::TagPrivate { public: TagPrivate() : + factory(0), file(0), tagOffset(0), extendedHeader(0), @@ -286,7 +287,7 @@ void ID3v2::Tag::setGenre(const String &s) void ID3v2::Tag::setYear(unsigned int i) { - if(i <= 0) { + if(i == 0) { removeFrames("TDRC"); return; } @@ -295,7 +296,7 @@ void ID3v2::Tag::setYear(unsigned int i) void ID3v2::Tag::setTrack(unsigned int i) { - if(i <= 0) { + if(i == 0) { removeFrames("TRCK"); return; } diff --git a/taglib/xm/xmfile.cpp b/taglib/xm/xmfile.cpp index 9192e9bf..216dfd8c 100644 --- a/taglib/xm/xmfile.cpp +++ b/taglib/xm/xmfile.cpp @@ -586,9 +586,9 @@ void XM::File::read(bool) unsigned int count = 4 + instrument.read(*this, instrumentHeaderSize - 4U); READ_ASSERT(count == std::min(instrumentHeaderSize, (unsigned long)instrument.size() + 4)); - unsigned long sampleHeaderSize = 0; long offset = 0; if(sampleCount > 0) { + unsigned long sampleHeaderSize = 0; sumSampleCount += sampleCount; // wouldn't know which header size to assume otherwise: READ_ASSERT(instrumentHeaderSize >= count + 4 && readU32L(sampleHeaderSize)); From ad075a56f924b9b40c09ddff68a940655dc97c33 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 24 Nov 2016 14:45:22 +0900 Subject: [PATCH 08/24] Suppress MSVC warnings in test. --- tests/test_xiphcomment.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index 64ae0b21..386a3e67 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -98,8 +98,8 @@ public: map[""] = String("invalid key: empty string"); map["A=B"] = String("invalid key: contains '='"); map["A~B"] = String("invalid key: contains '~'"); - map["A\x7F\B"] = String("invalid key: contains '\x7F'"); - map[L"A\x3456\B"] = String("invalid key: Unicode"); + map["A\x7F" "B"] = String("invalid key: contains '\x7F'"); + map[L"A\x3456" "B"] = String("invalid key: Unicode"); Ogg::XiphComment cmt; PropertyMap unsuccessful = cmt.setProperties(map); @@ -113,8 +113,8 @@ public: cmt.addField("", "invalid key: empty string"); cmt.addField("A=B", "invalid key: contains '='"); cmt.addField("A~B", "invalid key: contains '~'"); - cmt.addField("A\x7F\B", "invalid key: contains '\x7F'"); - cmt.addField(L"A\x3456\B", "invalid key: Unicode"); + cmt.addField("A\x7F" "B", "invalid key: contains '\x7F'"); + cmt.addField(L"A\x3456" "B", "invalid key: Unicode"); CPPUNIT_ASSERT_EQUAL(0U, cmt.fieldCount()); } From 6cfb11bb12bc2a83d5c36151538ad080ba6b7bde Mon Sep 17 00:00:00 2001 From: Martin Flaska Date: Fri, 25 Nov 2016 12:58:25 +0100 Subject: [PATCH 09/24] test_string: Make 'stripWhiteSpace' test more complex No string without leading/trailing spaces was used in the test. Signed-off-by: Martin Flaska --- tests/test_string.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_string.cpp b/tests/test_string.cpp index 599e002d..b99c3f30 100644 --- a/tests/test_string.cpp +++ b/tests/test_string.cpp @@ -112,6 +112,9 @@ public: CPPUNIT_ASSERT(String(" foo ").stripWhiteSpace() == String("foo")); CPPUNIT_ASSERT(String("foo ").stripWhiteSpace() == String("foo")); CPPUNIT_ASSERT(String(" foo").stripWhiteSpace() == String("foo")); + CPPUNIT_ASSERT(String("foo").stripWhiteSpace() == String("foo")); + CPPUNIT_ASSERT(String("f o o").stripWhiteSpace() == String("f o o")); + CPPUNIT_ASSERT(String(" f o o ").stripWhiteSpace() == String("f o o")); CPPUNIT_ASSERT(memcmp(String("foo").data(String::Latin1).data(), "foo", 3) == 0); CPPUNIT_ASSERT(memcmp(String("f").data(String::Latin1).data(), "f", 1) == 0); From c9a0754e3b4eff05491ac82ecd934aa86bf665d5 Mon Sep 17 00:00:00 2001 From: Martin Flaska Date: Fri, 25 Nov 2016 15:32:26 +0100 Subject: [PATCH 10/24] tstring: String::substr optimization when returning itself as a substring Use copy ctor to return in a case whole string is being returned. The intention was to optimize String::stripWhiteSpace for no-strip case (without any leading or trailing white space removal). copyFromUTF16 was used in any case previously and allocated duplicate buffer for the same string - no implicit sharing. Signed-off-by: Martin Flaska --- taglib/toolkit/tstring.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index b81b084e..ee8d04d1 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -447,7 +447,10 @@ bool String::startsWith(const String &s) const String String::substr(unsigned int position, unsigned int n) const { - return String(d->data.substr(position, n)); + if(position == 0 && n == size()) + return *this; + else + return String(d->data.substr(position, n)); } String &String::append(const String &s) From d3062f3af4c4c0274ee7e886ece333a71814021b Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 26 Nov 2016 13:05:14 +0900 Subject: [PATCH 11/24] A bit more tolerant check to return itself in String::substr(). --- taglib/toolkit/tstring.cpp | 2 +- tests/test_string.cpp | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index ee8d04d1..1639d7c5 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -447,7 +447,7 @@ bool String::startsWith(const String &s) const String String::substr(unsigned int position, unsigned int n) const { - if(position == 0 && n == size()) + if(position == 0 && n >= size()) return *this; else return String(d->data.substr(position, n)); diff --git a/tests/test_string.cpp b/tests/test_string.cpp index b99c3f30..4e3a1ac5 100644 --- a/tests/test_string.cpp +++ b/tests/test_string.cpp @@ -257,6 +257,8 @@ public: CPPUNIT_ASSERT_EQUAL(String("01"), String("0123456").substr(0, 2)); CPPUNIT_ASSERT_EQUAL(String("12"), String("0123456").substr(1, 2)); CPPUNIT_ASSERT_EQUAL(String("123456"), String("0123456").substr(1, 200)); + CPPUNIT_ASSERT_EQUAL(String("0123456"), String("0123456").substr(0, 7)); + CPPUNIT_ASSERT_EQUAL(String("0123456"), String("0123456").substr(0, 200)); } void testNewline() From 2786aa7463700aeefa358cbbfeccb9ff4839eadd Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Sun, 27 Nov 2016 19:17:13 +0100 Subject: [PATCH 12/24] Fixes #743 by not overwriting existing Xiph comment in FLAC::File::save --- taglib/flac/flacfile.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index b31cc65e..dba54d58 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -168,8 +168,9 @@ bool FLAC::File::save() } // Create new vorbis comments - - Tag::duplicate(&d->tag, xiphComment(true), false); + if (!hasXiphComment()) + Tag::duplicate(&d->tag, xiphComment(true), false); + d->xiphCommentData = xiphComment()->render(false); From 6df61cf2af02189d37f783b63745efca00d1bd34 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 29 Nov 2016 10:38:11 +0900 Subject: [PATCH 13/24] Small fix in style. --- taglib/flac/flacfile.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index dba54d58..93a0e952 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -168,9 +168,8 @@ bool FLAC::File::save() } // Create new vorbis comments - if (!hasXiphComment()) + if(!hasXiphComment()) Tag::duplicate(&d->tag, xiphComment(true), false); - d->xiphCommentData = xiphComment()->render(false); From 4381bd75f3a41f1507b13922dade6efce0da7ec2 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 29 Nov 2016 10:53:33 +0900 Subject: [PATCH 14/24] Add a test for #743/#779. --- tests/test_flac.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index 6a306981..f2065e09 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -62,6 +62,7 @@ class TestFLAC : public CppUnit::TestFixture CPPUNIT_TEST(testUpdateID3v2); CPPUNIT_TEST(testEmptyID3v2); CPPUNIT_TEST(testStripTags); + CPPUNIT_TEST(testRemoveXiphField); CPPUNIT_TEST_SUITE_END(); public: @@ -493,6 +494,28 @@ public: } } + void testRemoveXiphField() + { + ScopedFileCopy copy("silence-44-s", ".flac"); + + { + FLAC::File f(copy.fileName().c_str()); + f.xiphComment(true)->setTitle("XiphComment Title"); + f.ID3v2Tag(true)->setTitle("ID3v2 Title"); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT_EQUAL(String("XiphComment Title"), f.xiphComment()->title()); + f.xiphComment()->removeFields("TITLE"); + f.save(); + } + { + FLAC::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT_EQUAL(String(), f.xiphComment()->title()); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestFLAC); From 046c98230f5f458932fe24bc3e936ed4a47c3ce7 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 29 Nov 2016 14:58:39 +0900 Subject: [PATCH 15/24] Remove Utils::floatByteOrder() and use systemByteOrder() instead. We can safely assume that the integer and float byte orders are the same on IEEE754 compliant systems. --- taglib/toolkit/tbytevector.cpp | 4 ++-- taglib/toolkit/tutils.h | 22 +--------------------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index a2258bc5..3d6daed0 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -176,7 +176,7 @@ TFloat toFloat(const ByteVector &v, size_t offset) } tmp; ::memcpy(&tmp, v.data() + offset, sizeof(TInt)); - if(ENDIAN != Utils::floatByteOrder()) + if(ENDIAN != Utils::systemByteOrder()) tmp.i = Utils::byteSwap(tmp.i); return tmp.f; @@ -191,7 +191,7 @@ ByteVector fromFloat(TFloat value) } tmp; tmp.f = value; - if(ENDIAN != Utils::floatByteOrder()) + if(ENDIAN != Utils::systemByteOrder()) tmp.i = Utils::byteSwap(tmp.i); return ByteVector(reinterpret_cast(&tmp), sizeof(TInt)); diff --git a/taglib/toolkit/tutils.h b/taglib/toolkit/tutils.h index d4051f64..b114619e 100644 --- a/taglib/toolkit/tutils.h +++ b/taglib/toolkit/tutils.h @@ -233,7 +233,7 @@ namespace TagLib }; /*! - * Returns the integer byte order of the system. + * Returns the byte order of the system. */ inline ByteOrder systemByteOrder() { @@ -248,26 +248,6 @@ namespace TagLib else return BigEndian; } - - /*! - * Returns the IEEE754 byte order of the system. - */ - inline ByteOrder floatByteOrder() - { - union { - double d; - char c; - } u; - - // 1.0 is stored in memory like 0x3FF0000000000000 in canonical form. - // So the first byte is zero if little endian. - - u.d = 1.0; - if(u.c == 0) - return LittleEndian; - else - return BigEndian; - } } } } From cfbaf3459776e45cdbf2954de57bde8b689264fa Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 1 Dec 2016 10:50:30 +0900 Subject: [PATCH 16/24] Prevent the segment table of Ogg pages from exceeding the size limit. --- taglib/ogg/oggpage.cpp | 8 ++++---- tests/test_ogg.cpp | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/taglib/ogg/oggpage.cpp b/taglib/ogg/oggpage.cpp index 75aea22a..414d3d53 100644 --- a/taglib/ogg/oggpage.cpp +++ b/taglib/ogg/oggpage.cpp @@ -208,15 +208,15 @@ List Ogg::Page::paginate(const ByteVectorList &packets, static const unsigned int SplitSize = 32 * 255; - // Force repagination if the packets are too large for a page. + // Force repagination if the segment table will exceed the size limit. if(strategy != Repaginate) { - size_t totalSize = packets.size(); + size_t tableSize = 0; for(ByteVectorList::ConstIterator it = packets.begin(); it != packets.end(); ++it) - totalSize += it->size(); + tableSize += it->size() / 255 + 1; - if(totalSize > 255 * 255) + if(tableSize > 255) strategy = Repaginate; } diff --git a/tests/test_ogg.cpp b/tests/test_ogg.cpp index af2d5b94..5569e59c 100644 --- a/tests/test_ogg.cpp +++ b/tests/test_ogg.cpp @@ -42,7 +42,8 @@ class TestOGG : public CppUnit::TestFixture { CPPUNIT_TEST_SUITE(TestOGG); CPPUNIT_TEST(testSimple); - CPPUNIT_TEST(testSplitPackets); + CPPUNIT_TEST(testSplitPackets1); + CPPUNIT_TEST(testSplitPackets2); CPPUNIT_TEST(testDictInterface1); CPPUNIT_TEST(testDictInterface2); CPPUNIT_TEST(testAudioProperties); @@ -67,7 +68,7 @@ public: } } - void testSplitPackets() + void testSplitPackets1() { ScopedFileCopy copy("empty", ".ogg"); string newname = copy.fileName(); @@ -110,6 +111,33 @@ public: } } + void testSplitPackets2() + { + ScopedFileCopy copy("empty", ".ogg"); + string newname = copy.fileName(); + + const String text = longText(60890, true); + + { + Vorbis::File f(newname.c_str()); + f.tag()->setTitle(text); + f.save(); + } + { + Vorbis::File f(newname.c_str()); + CPPUNIT_ASSERT(f.isValid()); + CPPUNIT_ASSERT_EQUAL(text, f.tag()->title()); + + f.tag()->setTitle("ABCDE"); + f.save(); + } + { + Vorbis::File f(newname.c_str()); + CPPUNIT_ASSERT(f.isValid()); + CPPUNIT_ASSERT_EQUAL(String("ABCDE"), f.tag()->title()); + } + } + void testDictInterface1() { ScopedFileCopy copy("empty", ".ogg"); From 9336c82da3a04552168f208cd7a5fa4646701ea4 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 1 Dec 2016 11:32:01 +0900 Subject: [PATCH 17/24] Fix possible Ogg packet losses. --- taglib/ogg/oggfile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taglib/ogg/oggfile.cpp b/taglib/ogg/oggfile.cpp index 86b0b076..c36e4d46 100644 --- a/taglib/ogg/oggfile.cpp +++ b/taglib/ogg/oggfile.cpp @@ -253,7 +253,7 @@ void Ogg::File::writePacket(unsigned int i, const ByteVector &packet) ByteVectorList packets = firstPage->packets(); packets[i - firstPage->firstPacketIndex()] = packet; - if(firstPage != lastPage && lastPage->packetCount() > 2) { + if(firstPage != lastPage && lastPage->packetCount() > 1) { ByteVectorList lastPagePackets = lastPage->packets(); lastPagePackets.erase(lastPagePackets.begin()); packets.append(lastPagePackets); From 489e2e6cbbce49712ce149208b057e1e4788fb9f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 1 Dec 2016 15:25:30 +0900 Subject: [PATCH 18/24] Update NEWS. --- NEWS | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS b/NEWS index 1a185342..774e6532 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,7 @@ * Fixed reading MP4 atoms with zero length. * Fixed handling of redundant UTF-8 sequences in Win32. * Fixed handling of lowercase field names in Vorbis Comments. + * Fixed possible file corruptions when saving Ogg files. * Several smaller bug fixes and performance improvements. TagLib 1.11.1 (Oct 24, 2016) From f6a604f54b27b5f8c21f0d7ccda756029ed35ac6 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 2 Dec 2016 17:26:43 +0900 Subject: [PATCH 19/24] #include guards in CMake generated headers. --- config.h.cmake | 5 +++++ taglib/taglib_config.h.cmake | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/config.h.cmake b/config.h.cmake index 7eb5993f..90ae2eba 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -1,5 +1,8 @@ /* config.h. Generated by cmake from config.h.cmake */ +#ifndef TAGLIB_CONFIG_H +#define TAGLIB_CONFIG_H + /* Defined if your compiler supports some byte swap functions */ #cmakedefine HAVE_BOOST_BYTESWAP 1 #cmakedefine HAVE_GCC_BYTESWAP 1 @@ -31,3 +34,5 @@ #cmakedefine TRACE_IN_RELEASE 1 #cmakedefine TESTS_DIR "@TESTS_DIR@" + +#endif diff --git a/taglib/taglib_config.h.cmake b/taglib/taglib_config.h.cmake index 5f0ee6cf..915f130a 100644 --- a/taglib/taglib_config.h.cmake +++ b/taglib/taglib_config.h.cmake @@ -1,6 +1,11 @@ /* taglib_config.h. Generated by cmake from taglib_config.h.cmake */ +#ifndef TAGLIB_TAGLIB_CONFIG_H +#define TAGLIB_TAGLIB_CONFIG_H + /* These values are no longer used. This file is present only for compatibility reasons. */ #define TAGLIB_WITH_ASF 1 #define TAGLIB_WITH_MP4 1 + +#endif From b00a5c1aab4603819cce2a2d4946c978c60d8fd0 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 5 Dec 2016 10:15:26 +0900 Subject: [PATCH 20/24] Add a test to check if ByteVector is detached correctly when being replaced. --- tests/test_bytevector.cpp | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/test_bytevector.cpp b/tests/test_bytevector.cpp index 1e063b71..428e795c 100644 --- a/tests/test_bytevector.cpp +++ b/tests/test_bytevector.cpp @@ -46,6 +46,7 @@ class TestByteVector : public CppUnit::TestFixture CPPUNIT_TEST(testIntegerConversion); CPPUNIT_TEST(testFloatingPointConversion); CPPUNIT_TEST(testReplace); + CPPUNIT_TEST(testReplaceAndDetach); CPPUNIT_TEST(testIterator); CPPUNIT_TEST(testResize); CPPUNIT_TEST(testAppend1); @@ -312,6 +313,44 @@ public: CPPUNIT_ASSERT_EQUAL(ByteVector("abcdaba"), a); } } + void testReplaceAndDetach() + { + { + ByteVector a("abcdabf"); + ByteVector b = a; + a.replace(ByteVector("a"), ByteVector("x")); + CPPUNIT_ASSERT_EQUAL(ByteVector("xbcdxbf"), a); + CPPUNIT_ASSERT_EQUAL(ByteVector("abcdabf"), b); + } + { + ByteVector a("abcdabf"); + ByteVector b = a; + a.replace('a', 'x'); + CPPUNIT_ASSERT_EQUAL(ByteVector("xbcdxbf"), a); + CPPUNIT_ASSERT_EQUAL(ByteVector("abcdabf"), b); + } + { + ByteVector a("abcdabf"); + ByteVector b = a; + a.replace(ByteVector("ab"), ByteVector("xy")); + CPPUNIT_ASSERT_EQUAL(ByteVector("xycdxyf"), a); + CPPUNIT_ASSERT_EQUAL(ByteVector("abcdabf"), b); + } + { + ByteVector a("abcdabf"); + ByteVector b = a; + a.replace(ByteVector("a"), ByteVector("")); + CPPUNIT_ASSERT_EQUAL(ByteVector("bcdbf"), a); + CPPUNIT_ASSERT_EQUAL(ByteVector("abcdabf"), b); + } + { + ByteVector a("abdab"); + ByteVector b = a; + a.replace(ByteVector(""), ByteVector("c")); + CPPUNIT_ASSERT_EQUAL(ByteVector("abcdabc"), a); + CPPUNIT_ASSERT_EQUAL(ByteVector("abdab"), b); + } + } void testIterator() { From 36ccad2bd4b5b8aec1e547faef3bfe0269316ae9 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 2 Dec 2016 16:31:27 +0900 Subject: [PATCH 21/24] Rewrite ByteVector::replace() to run in O(n) time. --- taglib/toolkit/tbytevector.cpp | 77 ++++++++++++++++++++-------------- tests/test_bytevector.cpp | 1 + 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/taglib/toolkit/tbytevector.cpp b/taglib/toolkit/tbytevector.cpp index 3d6daed0..d272057f 100644 --- a/taglib/toolkit/tbytevector.cpp +++ b/taglib/toolkit/tbytevector.cpp @@ -29,7 +29,6 @@ #include #include #include -#include #include #include @@ -485,46 +484,60 @@ ByteVector &ByteVector::replace(char oldByte, char newByte) ByteVector &ByteVector::replace(const ByteVector &pattern, const ByteVector &with) { - // TODO: This takes O(n!) time in the worst case. Rewrite it to run in O(n) time. - - if(pattern.size() == 0 || pattern.size() > size()) - return *this; - if(pattern.size() == 1 && with.size() == 1) return replace(pattern[0], with[0]); - const unsigned int withSize = with.size(); - const unsigned int patternSize = pattern.size(); - const int diff = withSize - patternSize; + // Check if there is at least one occurrence of the pattern. - unsigned int offset = 0; - while (true) { - offset = find(pattern, offset); - if(offset == static_cast(-1)) - break; + int offset = find(pattern, 0); + if(offset == -1) + return *this; + + if(pattern.size() == with.size()) { + + // We think this case might be common enough to optimize it. detach(); + do + { + ::memcpy(data() + offset, with.data(), with.size()); + offset = find(pattern, offset + pattern.size()); + } while(offset != -1); + } + else { - if(diff < 0) { - ::memmove( - data() + offset + withSize, - data() + offset + patternSize, - size() - offset - patternSize); - resize(size() + diff); - } - else if(diff > 0) { - resize(size() + diff); - ::memmove( - data() + offset + withSize, - data() + offset + patternSize, - size() - diff - offset - patternSize); + // Loop once to calculate the result size. + + unsigned int dstSize = size(); + do + { + dstSize += with.size() - pattern.size(); + offset = find(pattern, offset + pattern.size()); + } while(offset != -1); + + // Loop again to copy modified data to the new vector. + + ByteVector dst(dstSize); + int dstOffset = 0; + + offset = 0; + while(true) { + const int next = find(pattern, offset); + if(next == -1) { + ::memcpy(dst.data() + dstOffset, data() + offset, size() - offset); + break; + } + + ::memcpy(dst.data() + dstOffset, data() + offset, next - offset); + dstOffset += next - offset; + + ::memcpy(dst.data() + dstOffset, with.data(), with.size()); + dstOffset += with.size(); + + offset = next + pattern.size(); } - ::memcpy(data() + offset, with.data(), with.size()); - - offset += withSize; - if(offset > size() - patternSize) - break; + swap(dst); } return *this; diff --git a/tests/test_bytevector.cpp b/tests/test_bytevector.cpp index 428e795c..f03dda1a 100644 --- a/tests/test_bytevector.cpp +++ b/tests/test_bytevector.cpp @@ -313,6 +313,7 @@ public: CPPUNIT_ASSERT_EQUAL(ByteVector("abcdaba"), a); } } + void testReplaceAndDetach() { { From b5115e3497328d80a9accf8c1c303374ec36e884 Mon Sep 17 00:00:00 2001 From: Hao Xi Date: Thu, 8 Dec 2016 12:53:40 +0800 Subject: [PATCH 22/24] Fix #667: Compiled TagLib framework for OS X fails at codesign. --- CMakeLists.txt | 13 ++++++++----- ConfigureChecks.cmake | 15 ++++++++++----- taglib/CMakeLists.txt | 8 +++++++- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a59efc9d..bbd7d7cc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,6 +13,14 @@ if(DEFINED ENABLE_STATIC) endif() option(BUILD_SHARED_LIBS "Build shared libraries" OFF) +if(APPLE) + option(BUILD_FRAMEWORK "Build an OS X framework" OFF) + if(BUILD_FRAMEWORK) + set(BUILD_SHARED_LIBS ON) + #set(CMAKE_MACOSX_RPATH 1) + set(FRAMEWORK_INSTALL_DIR "/Library/Frameworks" CACHE STRING "Directory to install frameworks to.") + endif() +endif() if(NOT BUILD_SHARED_LIBS) add_definitions(-DTAGLIB_STATIC) endif() @@ -48,11 +56,6 @@ set(BIN_INSTALL_DIR "${EXEC_INSTALL_PREFIX}/bin" CACHE PATH "The subdirectory to set(LIB_INSTALL_DIR "${EXEC_INSTALL_PREFIX}/lib${LIB_SUFFIX}" CACHE PATH "The subdirectory relative to the install prefix where libraries will be installed (default is /lib${LIB_SUFFIX})") set(INCLUDE_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/include" CACHE PATH "The subdirectory to the header prefix") -if(APPLE) - option(BUILD_FRAMEWORK "Build an OS X framework" OFF) - set(FRAMEWORK_INSTALL_DIR "/Library/Frameworks" CACHE STRING "Directory to install frameworks to.") -endif() - if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX) set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall") diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index ee4fdc2e..fe365711 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -54,11 +54,16 @@ check_cxx_source_compiles(" " HAVE_STD_ATOMIC) if(NOT HAVE_STD_ATOMIC) - find_package(Boost COMPONENTS atomic) - if(Boost_ATOMIC_FOUND) - set(HAVE_BOOST_ATOMIC 1) - else() - set(HAVE_BOOST_ATOMIC 0) + + # We will not find BOOST_ATOMIC on macOS when BUILD_FRAMEWORK is set, since we don't want to link + # to `libboost_atomic-mt.dylib` within `tag.framework`. + if(NOT BUILD_FRAMEWORK) + find_package(Boost COMPONENTS atomic) + if(Boost_ATOMIC_FOUND) + set(HAVE_BOOST_ATOMIC 1) + else() + set(HAVE_BOOST_ATOMIC 0) + endif() endif() if(NOT HAVE_BOOST_ATOMIC) diff --git a/taglib/CMakeLists.txt b/taglib/CMakeLists.txt index 000f7937..70ff8d1b 100644 --- a/taglib/CMakeLists.txt +++ b/taglib/CMakeLists.txt @@ -365,7 +365,13 @@ set_target_properties(tag PROPERTIES PUBLIC_HEADER "${tag_HDRS}" ) if(BUILD_FRAMEWORK) - set_target_properties(tag PROPERTIES FRAMEWORK TRUE) + unset(INSTALL_NAME_DIR) + set_target_properties(tag PROPERTIES + FRAMEWORK TRUE + MACOSX_RPATH 1 + VERSION "A" + SOVERSION "A" + ) endif() install(TARGETS tag From 250c59f783f11e5b23eea4cb73063b6ecb28453e Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 9 Dec 2016 09:42:29 +0900 Subject: [PATCH 23/24] Remove optional dependencies on Boost's dynamic libraries. Using precompiled Boost libraries can lead to depending on external dynamic libraries. --- ConfigureChecks.cmake | 75 ++++++++++++---------------------- config.h.cmake | 2 - taglib/CMakeLists.txt | 12 +----- taglib/toolkit/trefcounter.cpp | 5 --- taglib/toolkit/tzlib.cpp | 46 +++------------------ 5 files changed, 33 insertions(+), 107 deletions(-) diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index fe365711..253af335 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -54,61 +54,47 @@ check_cxx_source_compiles(" " HAVE_STD_ATOMIC) if(NOT HAVE_STD_ATOMIC) - - # We will not find BOOST_ATOMIC on macOS when BUILD_FRAMEWORK is set, since we don't want to link - # to `libboost_atomic-mt.dylib` within `tag.framework`. - if(NOT BUILD_FRAMEWORK) - find_package(Boost COMPONENTS atomic) - if(Boost_ATOMIC_FOUND) - set(HAVE_BOOST_ATOMIC 1) - else() - set(HAVE_BOOST_ATOMIC 0) - endif() - endif() + check_cxx_source_compiles(" + int main() { + volatile int x; + __sync_add_and_fetch(&x, 1); + int y = __sync_sub_and_fetch(&x, 1); + return 0; + } + " HAVE_GCC_ATOMIC) - if(NOT HAVE_BOOST_ATOMIC) + if(NOT HAVE_GCC_ATOMIC) check_cxx_source_compiles(" + #include int main() { - volatile int x; - __sync_add_and_fetch(&x, 1); - int y = __sync_sub_and_fetch(&x, 1); + volatile int32_t x; + OSAtomicIncrement32Barrier(&x); + int32_t y = OSAtomicDecrement32Barrier(&x); return 0; } - " HAVE_GCC_ATOMIC) + " HAVE_MAC_ATOMIC) - if(NOT HAVE_GCC_ATOMIC) + if(NOT HAVE_MAC_ATOMIC) check_cxx_source_compiles(" - #include + #include int main() { - volatile int32_t x; - OSAtomicIncrement32Barrier(&x); - int32_t y = OSAtomicDecrement32Barrier(&x); + volatile LONG x; + InterlockedIncrement(&x); + LONG y = InterlockedDecrement(&x); return 0; } - " HAVE_MAC_ATOMIC) + " HAVE_WIN_ATOMIC) - if(NOT HAVE_MAC_ATOMIC) + if(NOT HAVE_WIN_ATOMIC) check_cxx_source_compiles(" - #include + #include int main() { - volatile LONG x; - InterlockedIncrement(&x); - LONG y = InterlockedDecrement(&x); + volatile int x; + __sync_add_and_fetch(&x, 1); + int y = __sync_sub_and_fetch(&x, 1); return 0; } - " HAVE_WIN_ATOMIC) - - if(NOT HAVE_WIN_ATOMIC) - check_cxx_source_compiles(" - #include - int main() { - volatile int x; - __sync_add_and_fetch(&x, 1); - int y = __sync_sub_and_fetch(&x, 1); - return 0; - } - " HAVE_IA64_ATOMIC) - endif() + " HAVE_IA64_ATOMIC) endif() endif() endif() @@ -230,15 +216,6 @@ if(NOT ZLIB_SOURCE) else() set(HAVE_ZLIB 0) endif() - - if(NOT HAVE_ZLIB) - find_package(Boost COMPONENTS iostreams zlib) - if(Boost_IOSTREAMS_FOUND AND Boost_ZLIB_FOUND) - set(HAVE_BOOST_ZLIB 1) - else() - set(HAVE_BOOST_ZLIB 0) - endif() - endif() endif() # Determine whether CppUnit is installed. diff --git a/config.h.cmake b/config.h.cmake index 90ae2eba..fd716134 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -13,7 +13,6 @@ /* Defined if your compiler supports some atomic operations */ #cmakedefine HAVE_STD_ATOMIC 1 -#cmakedefine HAVE_BOOST_ATOMIC 1 #cmakedefine HAVE_GCC_ATOMIC 1 #cmakedefine HAVE_MAC_ATOMIC 1 #cmakedefine HAVE_WIN_ATOMIC 1 @@ -28,7 +27,6 @@ /* Defined if zlib is installed */ #cmakedefine HAVE_ZLIB 1 -#cmakedefine HAVE_BOOST_ZLIB 1 /* Indicates whether debug messages are shown even in release mode */ #cmakedefine TRACE_IN_RELEASE 1 diff --git a/taglib/CMakeLists.txt b/taglib/CMakeLists.txt index 70ff8d1b..4ea54304 100644 --- a/taglib/CMakeLists.txt +++ b/taglib/CMakeLists.txt @@ -32,7 +32,7 @@ elseif(HAVE_ZLIB_SOURCE) include_directories(${ZLIB_SOURCE}) endif() -if(HAVE_BOOST_BYTESWAP OR HAVE_BOOST_ATOMIC OR HAVE_BOOST_ZLIB) +if(Boost_FOUND) include_directories(${Boost_INCLUDE_DIR}) endif() @@ -344,18 +344,10 @@ set(tag_LIB_SRCS add_library(tag ${tag_LIB_SRCS} ${tag_HDRS}) -if(ZLIB_FOUND) +if(HAVE_ZLIB AND NOT HAVE_ZLIB_SOURCE) target_link_libraries(tag ${ZLIB_LIBRARIES}) endif() -if(HAVE_BOOST_ATOMIC) - target_link_libraries(tag ${Boost_ATOMIC_LIBRARY}) -endif() - -if(HAVE_BOOST_ZLIB) - target_link_libraries(tag ${Boost_IOSTREAMS_LIBRARY} ${Boost_ZLIB_LIBRARY}) -endif() - set_target_properties(tag PROPERTIES VERSION ${TAGLIB_SOVERSION_MAJOR}.${TAGLIB_SOVERSION_MINOR}.${TAGLIB_SOVERSION_PATCH} SOVERSION ${TAGLIB_SOVERSION_MAJOR} diff --git a/taglib/toolkit/trefcounter.cpp b/taglib/toolkit/trefcounter.cpp index 27d17b83..eb2aa69f 100644 --- a/taglib/toolkit/trefcounter.cpp +++ b/taglib/toolkit/trefcounter.cpp @@ -34,11 +34,6 @@ # define ATOMIC_INT std::atomic # define ATOMIC_INC(x) x.fetch_add(1) # define ATOMIC_DEC(x) (x.fetch_sub(1) - 1) -#elif defined(HAVE_BOOST_ATOMIC) -# include -# define ATOMIC_INT boost::atomic -# define ATOMIC_INC(x) x.fetch_add(1) -# define ATOMIC_DEC(x) (x.fetch_sub(1) - 1) #elif defined(HAVE_GCC_ATOMIC) # define ATOMIC_INT int # define ATOMIC_INC(x) __sync_add_and_fetch(&x, 1) diff --git a/taglib/toolkit/tzlib.cpp b/taglib/toolkit/tzlib.cpp index 40158fd2..6d07ba3d 100644 --- a/taglib/toolkit/tzlib.cpp +++ b/taglib/toolkit/tzlib.cpp @@ -27,23 +27,19 @@ # include #endif -#if defined(HAVE_ZLIB) +#ifdef HAVE_ZLIB # include -#elif defined(HAVE_BOOST_ZLIB) -# include -# include +# include +# include #endif -#include -#include - #include "tzlib.h" using namespace TagLib; bool zlib::isAvailable() { -#if defined(HAVE_ZLIB) || defined(HAVE_BOOST_ZLIB) +#ifdef HAVE_ZLIB return true; @@ -56,7 +52,7 @@ bool zlib::isAvailable() ByteVector zlib::decompress(const ByteVector &data) { -#if defined(HAVE_ZLIB) +#ifdef HAVE_ZLIB z_stream stream = {}; @@ -102,38 +98,6 @@ ByteVector zlib::decompress(const ByteVector &data) return outData; -#elif defined(HAVE_BOOST_ZLIB) - - using namespace boost::iostreams; - - struct : public sink - { - ByteVector data; - - typedef char char_type; - typedef sink_tag category; - - std::streamsize write(char const* s, std::streamsize n) - { - const unsigned int originalSize = data.size(); - - data.resize(static_cast(originalSize + n)); - ::memcpy(data.data() + originalSize, s, static_cast(n)); - - return n; - } - } sink; - - try { - zlib_decompressor().write(sink, data.data(), data.size()); - } - catch(const zlib_error &) { - debug("zlib::decompress() - Error reading compressed stream."); - return ByteVector(); - } - - return sink.data; - #else return ByteVector(); From a19a623d4bfaa7d26ece05ac73c4716e5710d34f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 9 Dec 2016 09:56:37 +0900 Subject: [PATCH 24/24] Make use of increment/decrement operators of std::atomic. --- ConfigureChecks.cmake | 6 +++--- taglib/toolkit/trefcounter.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index 253af335..4b55f273 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -46,9 +46,9 @@ endif() check_cxx_source_compiles(" #include int main() { - std::atomic x; - x.fetch_add(1); - x.fetch_sub(1); + std::atomic_int x; + ++x; + --x; return 0; } " HAVE_STD_ATOMIC) diff --git a/taglib/toolkit/trefcounter.cpp b/taglib/toolkit/trefcounter.cpp index eb2aa69f..6638fcaa 100644 --- a/taglib/toolkit/trefcounter.cpp +++ b/taglib/toolkit/trefcounter.cpp @@ -31,9 +31,9 @@ #if defined(HAVE_STD_ATOMIC) # include -# define ATOMIC_INT std::atomic -# define ATOMIC_INC(x) x.fetch_add(1) -# define ATOMIC_DEC(x) (x.fetch_sub(1) - 1) +# define ATOMIC_INT std::atomic_int +# define ATOMIC_INC(x) (++x) +# define ATOMIC_DEC(x) (--x) #elif defined(HAVE_GCC_ATOMIC) # define ATOMIC_INT int # define ATOMIC_INC(x) __sync_add_and_fetch(&x, 1)