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_id3v2.cpp b/tests/test_id3v2.cpp index 9cf2e50e..29758aa4 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -72,6 +72,7 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testW000); CPPUNIT_TEST(testPropertyInterface); CPPUNIT_TEST(testPropertyInterface2); + CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST_SUITE_END(); @@ -640,6 +641,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"); 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);