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

@@ -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

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.
        */

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());

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 {

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())

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())

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

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".

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();
-}

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:
 
 

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

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);

tests/test_propertymap.cpp

@@ -0,0 +1,32 @@
+#include <cppunit/extensions/HelperMacros.h>
+#include <tpropertymap.h>
+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);

tests/test_xiphcomment.cpp

@@ -2,6 +2,7 @@
 #include <string>
 #include <stdio.h>
 #include <xiphcomment.h>
+#include <tpropertymap.h>
 #include <tdebug.h>
 #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);