ID3v2: Skip an unreadable frame instead of ending the tag (#1483)

A frame that cannot be read, such as one with an invalid ID, is now
skipped when another frame, padding or the end of the frames follows
where its size ends; anything else still ends the tag as before.
Previously every frame after it was dropped, and a save then deleted
them from the file.

taglib/mpeg/id3v2/id3v2tag.cpp

@@ -119,6 +119,18 @@ namespace
            (!frame2 || frame2->textEncoding() == String::Latin1)
        ? String::Latin1 : String::UTF16;
   }
+
+  // Whether a frame ID of the length \a version uses, made of uppercase
+  // letters and digits, starts at \a offset in \a data.
+  bool startsWithFrameID(const ByteVector &data, unsigned int offset, unsigned int version)
+  {
+    const unsigned int idSize = version < 3 ? 3 : 4;
+    if(offset + idSize > data.size())
+      return false;
+
+    return std::none_of(data.begin() + offset, data.begin() + offset + idSize,
+      [](auto c) { return (c < 'A' || c > 'Z') && (c < '0' || c > '9'); });
+  }
 }  // namespace
 
 class ID3v2::Tag::TagPrivate
@@ -957,8 +969,21 @@ void ID3v2::Tag::parse(const ByteVector &origData)
     unsigned int headerVersion = tagHeader->majorVersion();
     Frame *frame = d->factory->createFrame(origData, tagHeader);
 
-    if(!frame)
+    if(!frame) {
+      // A frame that cannot be read, such as one with an invalid ID, is skipped
+      // when what follows where its size ends is another frame, padding or the
+      // end of the frames. Anything else is taken for corruption and ends the tag.
+      const Frame::Header frameHeader(origData, headerVersion);
+      const unsigned long long next = static_cast<unsigned long long>(frameDataPosition) +
+                                      frameHeader.size() + frameHeader.frameSize();
+      if(next <= frameDataLength &&
+         (next == frameDataLength || data.at(static_cast<unsigned int>(next)) == 0 ||
+          startsWithFrameID(data, static_cast<unsigned int>(next), headerVersion))) {
+        frameDataPosition = static_cast<unsigned int>(next);
+        continue;
+      }
       return;
+    }
 
     if(frame->header()->version() == headerVersion) {
       frameDataPosition += frame->size() + frame->headerSize();

tests/test_id3v2.cpp

@@ -28,6 +28,8 @@
 #include <cassert>
 
 #include "tpropertymap.h"
+#include "tfilestream.h"
+#include "id3v2synchdata.h"
 #include "tzlib.h"
 #include "id3v2tag.h"
 #include "mpegfile.h"
@@ -141,6 +143,7 @@ class TestID3v2 : public CppUnit::TestFixture
   CPPUNIT_TEST(testRenderTableOfContentsFrameWithManyChildren);
   CPPUNIT_TEST(testShrinkPadding);
   CPPUNIT_TEST(testEmptyFrame);
+  CPPUNIT_TEST(testSkipUnreadableFrame);
   CPPUNIT_TEST(testDuplicateTags);
   CPPUNIT_TEST(testParseTOCFrameWithManyChildren);
   CPPUNIT_TEST(testInvalidID3v2Version);
@@ -1877,6 +1880,52 @@ public:
     }
   }
 
+  void testSkipUnreadableFrame()
+  {
+    const auto frame = [](const ByteVector &id, const ByteVector &text) {
+      const ByteVector body = ByteVector(1, '\0') + text;
+      return id + ID3v2::SynchData::fromUInt(body.size()) + ByteVector(2, '\0') + body;
+    };
+    const auto writeTag = [](const string &name, const ByteVector &frames) {
+      {
+        MPEG::File f(name.c_str());
+        f.strip();
+      }
+      FileStream stream(name.c_str());
+      stream.insert(ByteVector("ID3\x04\x00\x00", 6) + ID3v2::SynchData::fromUInt(frames.size()) + frames,
+                    0, 0);
+    };
+
+    ScopedFileCopy copy("xing", ".mp3");
+    string newname = copy.fileName();
+
+    // An invalid ID and a v2.2 ID padded with a space, each followed by a
+    // valid frame, are skipped, and the frames after them read and saved.
+    writeTag(newname, frame("TIT2", "Title") + frame("\xff" "LEN", "1") +
+             frame("TPE1", "Artist") + frame("CM1 ", "x") + frame("TALB", "Album"));
+    for(int pass = 0; pass < 2; ++pass) {
+      MPEG::File f(newname.c_str());
+      ID3v2::Tag *tag = f.ID3v2Tag();
+      CPPUNIT_ASSERT_EQUAL(3U, tag->frameList().size());
+      CPPUNIT_ASSERT_EQUAL(String("Title"), tag->title());
+      CPPUNIT_ASSERT_EQUAL(String("Artist"), tag->artist());
+      CPPUNIT_ASSERT_EQUAL(String("Album"), tag->album());
+      if(pass == 0)
+        CPPUNIT_ASSERT(f.save());
+    }
+
+    // A frame followed by anything else still ends the tag.
+    writeTag(newname, frame("TIT2", "Title") + frame("\xff" "LEN", "1") +
+             ByteVector("\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a", 10) +
+             frame("TALB", "Album"));
+    {
+      MPEG::File f(newname.c_str());
+      ID3v2::Tag *tag = f.ID3v2Tag();
+      CPPUNIT_ASSERT_EQUAL(1U, tag->frameList().size());
+      CPPUNIT_ASSERT_EQUAL(String("Title"), tag->title());
+    }
+  }
+
   void testDuplicateTags()
   {
     ScopedFileCopy copy("duplicate_id3v2", ".mp3");