From fc3fc10f60488e97423ff7ef077c940a5acc0449 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Mon, 23 Jul 2012 20:53:25 +0200 Subject: [PATCH 1/2] add id3v2 frame delete test --- tests/test_id3v2.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 27533655..0dd033d5 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -71,6 +71,7 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testCompressedFrameWithBrokenLength); CPPUNIT_TEST(testPropertyInterface); CPPUNIT_TEST(testPropertyInterface2); + CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST_SUITE_END(); @@ -629,6 +630,21 @@ public: CPPUNIT_ASSERT(tag.frameList("TIPL").isEmpty()); } + void testDeleteFrame() + { + ScopedFileCopy copy("rare_frames", ".mp3"); + string newname = copy.fileName(); + MPEG::File f(newname.c_str()); + ID3v2::Tag *t = f.ID3v2Tag(); + ID3v2::Frame *frame = t->frameList("TCON")[0]; + CPPUNIT_ASSERT_EQUAL(1u, t->frameList("TCON").size()); + t->removeFrame(frame, true); + f.save(MPEG::File::ID3v2); + + MPEG::File f2(newname.c_str()); + t = f2.ID3v2Tag(); + CPPUNIT_ASSERT(t->frameList("TCON").isEmpty()); + } void testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2() { ScopedFileCopy copy("xing", ".mp3"); From 4140c5f2eb3d0ea3a07ec79fdac63d6806f10bc6 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Mon, 30 Jul 2012 20:52:30 +0200 Subject: [PATCH 2/2] Check PropertyMap keys format-specifically instead of globally. Instead of statically forbidding certain keys in PropertyMap, now the setProperties() implementations of the different formats check if the keys are valid for that particular specification and include them in the returned PropertyMap otherwise. This should remove an unneccessary complification for programmers since now there's only one step, namely calling setProperties(), where problems might occur. Also the previous implementation leads to problems with invalid keys: because taglib doesn't use exceptions, something like map.insert("FORBIDDEN KEY", "some value"); would lead to the value being inserted under String::null, which smells like the source of strange bugs. --- taglib/ape/apetag.cpp | 25 ++++++++-- taglib/ape/apetag.h | 9 +++- taglib/mpeg/id3v2/frames/commentsframe.cpp | 2 +- .../id3v2/frames/textidentificationframe.cpp | 4 +- .../frames/unsynchronizedlyricsframe.cpp | 2 +- taglib/mpeg/id3v2/frames/urllinkframe.cpp | 2 +- taglib/ogg/xiphcomment.cpp | 20 ++++++-- taglib/ogg/xiphcomment.h | 10 +++- taglib/toolkit/tpropertymap.cpp | 49 ++++--------------- taglib/toolkit/tpropertymap.h | 26 +++------- tests/CMakeLists.txt | 1 + tests/test_apetag.cpp | 18 ++++++- tests/test_propertymap.cpp | 32 ++++++++++++ tests/test_xiphcomment.cpp | 15 ++++++ 14 files changed, 143 insertions(+), 72 deletions(-) create mode 100644 tests/test_propertymap.cpp diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index 7a16f3f2..f1af0028 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -189,7 +189,7 @@ PropertyMap APE::Tag::properties() const PropertyMap properties; ItemListMap::ConstIterator it = itemListMap().begin(); for(; it != itemListMap().end(); ++it) { - String tagName = PropertyMap::prepareKey(it->first); + String tagName = it->first.upper(); // if the item is Binary or Locator, or if the key is an invalid string, // add to unsupportedData if(it->second.type() != Item::Text || tagName.isNull()) @@ -227,7 +227,7 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) StringList toRemove; ItemListMap::ConstIterator remIt = itemListMap().begin(); for(; remIt != itemListMap().end(); ++remIt) { - String key = PropertyMap::prepareKey(remIt->first); + String key = remIt->first.upper(); // only remove if a) key is valid, b) type is text, c) key not contained in new properties if(!key.isNull() && remIt->second.type() == APE::Item::Text && !properties.contains(key)) toRemove.append(remIt->first); @@ -238,9 +238,12 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) // now sync in the "forward direction" PropertyMap::ConstIterator it = properties.begin(); + PropertyMap invalid; for(; it != properties.end(); ++it) { const String &tagName = it->first; - if(!(itemListMap().contains(tagName)) || !(itemListMap()[tagName].values() == it->second)) { + if(!checkKey(tagName)) + invalid.insert(it->first, it->second); + else if(!(itemListMap().contains(tagName)) || !(itemListMap()[tagName].values() == it->second)) { if(it->second.size() == 0) removeItem(tagName); else { @@ -252,7 +255,21 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) } } } - return PropertyMap(); + return invalid; +} + +bool APE::Tag::checkKey(const String &key) +{ + if(key.size() < 2 or 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; } APE::Footer *APE::Tag::footer() const diff --git a/taglib/ape/apetag.h b/taglib/ape/apetag.h index 1a8c30c9..c1f12e29 100644 --- a/taglib/ape/apetag.h +++ b/taglib/ape/apetag.h @@ -123,10 +123,17 @@ namespace TagLib { /*! * Implements the unified tag dictionary interface -- import function. The same - * comments as for the export function apply. + * comments as for the export function apply; additionally note that the APE tag + * specification requires keys to have between 2 and 16 printable ASCII characters + * with the exception of the fixed strings "ID3", "TAG", "OGGS", and "MP+". */ PropertyMap setProperties(const PropertyMap &); + /*! + * Check if the given String is a valid APE tag key. + */ + static bool checkKey(const String&); + /*! * Returns a pointer to the tag's footer. */ diff --git a/taglib/mpeg/id3v2/frames/commentsframe.cpp b/taglib/mpeg/id3v2/frames/commentsframe.cpp index 2c6c49f9..18195531 100644 --- a/taglib/mpeg/id3v2/frames/commentsframe.cpp +++ b/taglib/mpeg/id3v2/frames/commentsframe.cpp @@ -112,7 +112,7 @@ void CommentsFrame::setTextEncoding(String::Type encoding) PropertyMap CommentsFrame::asProperties() const { - String key = PropertyMap::prepareKey(description()); + String key = description().upper(); PropertyMap map; if(key.isEmpty() || key == "COMMENT") map.insert("COMMENT", text()); diff --git a/taglib/mpeg/id3v2/frames/textidentificationframe.cpp b/taglib/mpeg/id3v2/frames/textidentificationframe.cpp index b6a02b06..b01f5129 100644 --- a/taglib/mpeg/id3v2/frames/textidentificationframe.cpp +++ b/taglib/mpeg/id3v2/frames/textidentificationframe.cpp @@ -289,7 +289,7 @@ PropertyMap TextIdentificationFrame::makeTMCLProperties() const } StringList l = fieldList(); for(StringList::ConstIterator it = l.begin(); it != l.end(); ++it) { - String instrument = PropertyMap::prepareKey(*it); + String instrument = it->upper(); if(instrument.isNull()) { // instrument is not a valid key -> frame unsupported map.clear(); @@ -382,7 +382,7 @@ PropertyMap UserTextIdentificationFrame::asProperties() const String tagName = description(); PropertyMap map; - String key = map.prepareKey(tagName); + String key = tagName.upper(); if(key.isNull()) // this frame's description is not a valid PropertyMap key -> add to unsupported list map.unsupportedData().append(L"TXXX/" + description()); else { diff --git a/taglib/mpeg/id3v2/frames/unsynchronizedlyricsframe.cpp b/taglib/mpeg/id3v2/frames/unsynchronizedlyricsframe.cpp index 9d76164d..3f4d6a79 100644 --- a/taglib/mpeg/id3v2/frames/unsynchronizedlyricsframe.cpp +++ b/taglib/mpeg/id3v2/frames/unsynchronizedlyricsframe.cpp @@ -116,7 +116,7 @@ void UnsynchronizedLyricsFrame::setTextEncoding(String::Type encoding) PropertyMap UnsynchronizedLyricsFrame::asProperties() const { PropertyMap map; - String key = PropertyMap::prepareKey(description()); + String key = description().upper(); if(key.isEmpty() || key.upper() == "LYRICS") map.insert("LYRICS", text()); else if(key.isNull()) diff --git a/taglib/mpeg/id3v2/frames/urllinkframe.cpp b/taglib/mpeg/id3v2/frames/urllinkframe.cpp index 5e4f2db7..6bcbbda4 100644 --- a/taglib/mpeg/id3v2/frames/urllinkframe.cpp +++ b/taglib/mpeg/id3v2/frames/urllinkframe.cpp @@ -156,7 +156,7 @@ void UserUrlLinkFrame::setDescription(const String &s) PropertyMap UserUrlLinkFrame::asProperties() const { PropertyMap map; - String key = PropertyMap::prepareKey(description()); + String key = description().upper(); if(key.isEmpty() || key.upper() == "URL") map.insert("URL", url()); else if(key.isNull()) diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index 5a6feef3..675614b3 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -205,11 +205,14 @@ PropertyMap Ogg::XiphComment::setProperties(const PropertyMap &properties) for(StringList::ConstIterator it = toRemove.begin(); it != toRemove.end(); ++it) removeField(*it); - // now go through keys in \a properties and check that the values match those in the xiph comment */ + // now go through keys in \a properties and check that the values match those in the xiph comment + PropertyMap invalid; PropertyMap::ConstIterator it = properties.begin(); for(; it != properties.end(); ++it) { - if(!d->fieldListMap.contains(it->first) || !(it->second == d->fieldListMap[it->first])) { + if(!checkKey(it->first)) + invalid.insert(it->first, it->second); + else if(!d->fieldListMap.contains(it->first) || !(it->second == d->fieldListMap[it->first])) { const StringList &sl = it->second; if(sl.size() == 0) // zero size string list -> remove the tag with all values @@ -224,7 +227,18 @@ PropertyMap Ogg::XiphComment::setProperties(const PropertyMap &properties) } } } - return PropertyMap(); + return invalid; +} + +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) + return false; + return true; } String Ogg::XiphComment::vendorID() const diff --git a/taglib/ogg/xiphcomment.h b/taglib/ogg/xiphcomment.h index 3b44086e..54f3070b 100644 --- a/taglib/ogg/xiphcomment.h +++ b/taglib/ogg/xiphcomment.h @@ -151,10 +151,18 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * The tags from the given map will be stored one-to-one in the file. + * The tags from the given map will be stored one-to-one in the file, + * except for invalid keys (less than one character, non-ASCII, or + * containing '=' or '~') in which case the according values will + * be contained in the returned PropertyMap. */ PropertyMap setProperties(const PropertyMap&); + /*! + * Check if the given String is a valid Xiph comment key. + */ + static bool checkKey(const String&); + /*! * Returns the vendor ID of the Ogg Vorbis encoder. libvorbis 1.0 as the * most common case always returns "Xiph.Org libVorbis I 20020717". diff --git a/taglib/toolkit/tpropertymap.cpp b/taglib/toolkit/tpropertymap.cpp index 1743a0b2..3317cc92 100644 --- a/taglib/toolkit/tpropertymap.cpp +++ b/taglib/toolkit/tpropertymap.cpp @@ -34,7 +34,7 @@ PropertyMap::PropertyMap(const PropertyMap &m) : SimplePropertyMap(m), unsupport PropertyMap::PropertyMap(const SimplePropertyMap &m) { for(SimplePropertyMap::ConstIterator it = m.begin(); it != m.end(); ++it){ - String key = prepareKey(it->first); + String key = it->first.upper(); if(!key.isNull()) insert(it->first, it->second); else @@ -48,10 +48,7 @@ PropertyMap::~PropertyMap() bool PropertyMap::insert(const String &key, const StringList &values) { - String realKey = prepareKey(key); - if(realKey.isNull()) - return false; - + String realKey = key.upper(); Iterator result = SimplePropertyMap::find(realKey); if(result == end()) SimplePropertyMap::insert(realKey, values); @@ -62,9 +59,7 @@ bool PropertyMap::insert(const String &key, const StringList &values) bool PropertyMap::replace(const String &key, const StringList &values) { - String realKey = prepareKey(key); - if(realKey.isNull()) - return false; + String realKey = key.upper(); SimplePropertyMap::erase(realKey); SimplePropertyMap::insert(realKey, values); return true; @@ -72,26 +67,17 @@ bool PropertyMap::replace(const String &key, const StringList &values) PropertyMap::Iterator PropertyMap::find(const String &key) { - String realKey = prepareKey(key); - if(realKey.isNull()) - return end(); - return SimplePropertyMap::find(realKey); + return SimplePropertyMap::find(key.upper()); } PropertyMap::ConstIterator PropertyMap::find(const String &key) const { - String realKey = prepareKey(key); - if(realKey.isNull()) - return end(); - return SimplePropertyMap::find(realKey); + return SimplePropertyMap::find(key.upper()); } bool PropertyMap::contains(const String &key) const { - String realKey = prepareKey(key); - if(realKey.isNull()) - return false; - return SimplePropertyMap::contains(realKey); + return SimplePropertyMap::contains(key.upper()); } bool PropertyMap::contains(const PropertyMap &other) const @@ -107,9 +93,7 @@ bool PropertyMap::contains(const PropertyMap &other) const PropertyMap &PropertyMap::erase(const String &key) { - String realKey = prepareKey(key); - if(!realKey.isNull()) - SimplePropertyMap::erase(realKey); + SimplePropertyMap::erase(key.upper()); return *this; } @@ -122,23 +106,20 @@ PropertyMap &PropertyMap::erase(const PropertyMap &other) PropertyMap &PropertyMap::merge(const PropertyMap &other) { - for(PropertyMap::ConstIterator it = other.begin(); it != other.end(); ++it) { + for(PropertyMap::ConstIterator it = other.begin(); it != other.end(); ++it) insert(it->first, it->second); - } unsupported.append(other.unsupported); return *this; } const StringList &PropertyMap::operator[](const String &key) const { - String realKey = prepareKey(key); - return SimplePropertyMap::operator[](realKey); + return SimplePropertyMap::operator[](key.upper()); } StringList &PropertyMap::operator[](const String &key) { - String realKey = prepareKey(key); - return SimplePropertyMap::operator[](realKey); + return SimplePropertyMap::operator[](key.upper()); } bool PropertyMap::operator==(const PropertyMap &other) const @@ -190,13 +171,3 @@ const StringList &PropertyMap::unsupportedData() const { return unsupported; } - -String PropertyMap::prepareKey(const String &proposed) { - if(proposed.isEmpty()) - return String::null; - for (String::ConstIterator it = proposed.begin(); it != proposed.end(); it++) - // forbid non-printable, non-ascii, '=' (#61) and '~' (#126) - if (*it < 32 || *it >= 128 || *it == 61 || *it == 126) - return String::null; - return proposed.upper(); -} diff --git a/taglib/toolkit/tpropertymap.h b/taglib/toolkit/tpropertymap.h index b955739b..7f59b21e 100644 --- a/taglib/toolkit/tpropertymap.h +++ b/taglib/toolkit/tpropertymap.h @@ -36,15 +36,10 @@ namespace TagLib { * ("tags") realized as pairs of a case-insensitive key * and a nonempty list of corresponding values, each value being an an arbitrary * unicode String. - * The key has the same restrictions as in the vorbis comment specification, - * i.e. it must contain at least one character; all printable ASCII characters - * except '=' and '~' are allowed. - * - * In order to be safe with other formats, keep these additional restrictions in mind: - * - * - APE only allows keys from 2 to 16 printable ASCII characters (including space), - * with the exception of these strings: ID3, TAG, OggS, MP+ * + * Note that most metadata formats pose additional conditions on the tag keys. The + * most popular ones (Vorbis, APE, ID3v2) should support all ASCII only words of + * length between 2 and 16. */ class TAGLIB_EXPORT PropertyMap: public SimplePropertyMap @@ -126,16 +121,17 @@ namespace TagLib { /*! * Returns a reference to the value associated with \a key. * - * \note: This has undefined behavior if the key is not valid or not - * present in the map. + * \note: If \a key is not contained in the map, an empty + * StringList is returned without error. */ const StringList &operator[](const String &key) const; /*! * Returns a reference to the value associated with \a key. * - * \note: This has undefined behavior if the key is not valid or not - * present in the map. + * \note: If \a key is not contained in the map, an empty + * StringList is returned. You can also directly add entries + * by using this function as an lvalue. */ StringList &operator[](const String &key); @@ -168,12 +164,6 @@ namespace TagLib { String toString() const; - /*! - * Converts \a proposed into another String suitable to be used as - * a key, or returns String::null if this is not possible. - */ - static String prepareKey(const String &proposed); - private: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ed4eec0d..38ba4d25 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -35,6 +35,7 @@ SET(test_runner_SRCS test_bytevectorlist.cpp test_bytevectorstream.cpp test_string.cpp + test_propertymap.cpp test_fileref.cpp test_id3v1.cpp test_id3v2.cpp diff --git a/tests/test_apetag.cpp b/tests/test_apetag.cpp index c4696703..6cda5e52 100644 --- a/tests/test_apetag.cpp +++ b/tests/test_apetag.cpp @@ -19,6 +19,7 @@ class TestAPETag : public CppUnit::TestFixture CPPUNIT_TEST(testIsEmpty2); CPPUNIT_TEST(testPropertyInterface1); CPPUNIT_TEST(testPropertyInterface2); + CPPUNIT_TEST(testInvalidKeys); CPPUNIT_TEST_SUITE_END(); public: @@ -76,12 +77,27 @@ public: APE::Item item3 = APE::Item("TRACKNUMBER", "29"); tag.setItem("TRACKNUMBER", item3); properties = tag.properties(); - CPPUNIT_ASSERT_EQUAL(2u, properties["TRACKNUMBER"].size()); + CPPUNIT_ASSERT_EQUAL(uint(2), properties["TRACKNUMBER"].size()); CPPUNIT_ASSERT_EQUAL(String("17"), properties["TRACKNUMBER"][0]); CPPUNIT_ASSERT_EQUAL(String("29"), properties["TRACKNUMBER"][1]); } + void testInvalidKeys() + { + PropertyMap properties; + properties["A"] = String("invalid key: one character"); + properties["MP+"] = String("invalid key: forbidden string"); + properties["A B~C"] = String("valid key: space and tilde"); + properties["ARTIST"] = String("valid key: normal one"); + + APE::Tag tag; + PropertyMap unsuccessful = tag.setProperties(properties); + CPPUNIT_ASSERT_EQUAL(uint(2), unsuccessful.size()); + CPPUNIT_ASSERT(unsuccessful.contains("A")); + CPPUNIT_ASSERT(unsuccessful.contains("MP+")); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestAPETag); diff --git a/tests/test_propertymap.cpp b/tests/test_propertymap.cpp new file mode 100644 index 00000000..d0b2fbb7 --- /dev/null +++ b/tests/test_propertymap.cpp @@ -0,0 +1,32 @@ +#include +#include +class TestPropertyMap : public CppUnit::TestFixture +{ + CPPUNIT_TEST_SUITE(TestPropertyMap); + CPPUNIT_TEST(testInvalidKeys); + CPPUNIT_TEST_SUITE_END(); + +public: + void testInvalidKeys() + { + TagLib::PropertyMap map1; + CPPUNIT_ASSERT(map1.isEmpty()); + map1["ÄÖÜ"].append("test"); + CPPUNIT_ASSERT_EQUAL(map1.size(), 1u); + + TagLib::PropertyMap map2; + map2["ÄÖÜ"].append("test"); + CPPUNIT_ASSERT(map1 == map2); + CPPUNIT_ASSERT(map1.contains(map2)); + + map2["ARTIST"] = TagLib::String("Test Artist"); + CPPUNIT_ASSERT(map1 != map2); + CPPUNIT_ASSERT(map2.contains(map1)); + + map2["ÄÖÜ"].append("test 2"); + CPPUNIT_ASSERT(!map2.contains(map1)); + + } +}; + +CPPUNIT_TEST_SUITE_REGISTRATION(TestPropertyMap); diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index 359206e8..a98aea61 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include "utils.h" @@ -15,6 +16,7 @@ class TestXiphComment : public CppUnit::TestFixture CPPUNIT_TEST(testSetYear); CPPUNIT_TEST(testTrack); CPPUNIT_TEST(testSetTrack); + CPPUNIT_TEST(testInvalidKeys); CPPUNIT_TEST_SUITE_END(); public: @@ -59,6 +61,19 @@ public: CPPUNIT_ASSERT_EQUAL(String("3"), cmt.fieldListMap()["TRACKNUMBER"].front()); } + void testInvalidKeys() + { + PropertyMap map; + map[""] = String("invalid key: empty string"); + map["A=B"] = String("invalid key: contains '='"); + map["A~B"] = String("invalid key: contains '~'"); + + Ogg::XiphComment cmt; + PropertyMap unsuccessful = cmt.setProperties(map); + CPPUNIT_ASSERT_EQUAL(TagLib::uint(3), unsuccessful.size()); + CPPUNIT_ASSERT(cmt.properties().isEmpty()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestXiphComment);