allow empty ID3v2 frames

[Ralf Engels]

Hi Peter,

I am sorry. The patch that I submitted some days ago for the same
problem was better in my opinion.

Your second change allows null byte frames to be parsed.

if(!frameID.size() == (version < 3 ? 3 : 4) ||
-     header->frameSize() <= uint(header->dataLengthIndicator() ? 4 : 0) ||
+     header->frameSize() < uint(header->dataLengthIndicator() ? 4 : 0) ||
      header->frameSize() > data.size())

However you did not notice that according to the spec the frame length is excluding the header.
Even if that would not be the case the original check for the dataLengthIndicator was wrong as it did not check for the different length of the frame headers.

A better check could be found in my patch:

   if(!frameID.size() == (version < 3 ? 3 : 4) ||
-     header->frameSize() <= uint(header->dataLengthIndicator() ? 4 : 0) ||
-     header->frameSize() > data.size())
+     header->frameSize() > data.size() - Frame::Header::size( version ) )
   {
+      debug("FrameFactory::createFrame() -- Invalid frame size "+frameID);
     delete header;
     return 0;
   }


As you see I am checking instead if the given data has enough bytes available to contain the complete frame.

Next a null bytes frame will usually not be parsed correctly.
I even got errors in the Wxxx frames as a null termination can not be found.
Other frames (POPM) come to mind where a null byte frame will never be valid.

Thus: also contained in my patch:

+  // Windows media player writes empty tags even though the
+  // spec states that frames must have at least one byte.
+  // We just ignore it for now.
+  if( header->frameSize() == 0 ) {
+    debug("Empty frame "+frameID+" even though spec forbids it.");
+    return new UnknownFrame(data, header);
+  }
+

I just return an UnknownFrame in that case. What else should I do?


And then of course id3v2tag.cpp needs to be fixed.
After your change null byte tags are not longer deleted and the parsing
continues.

Because I return an UnknownFrame for frame headers where another object
type is to be expected I now need to deleted the frames instead like
this:

+    // (windows media player tends to write null byte frames 
+    // even though the spec forbids it
+    // We delete those frames because they are mostly broken
+    // (e.g. no null termination)
     if(frame->size() <= 0) {
-      delete frame;
-      return;
+       delete frame;
+    } else {
+      addFrame(frame);
     }
-
-    frameDataPosition += frame->size() +
Frame::headerSize(d->header.majorVersion());
-    addFrame(frame);



I am wondering Peter.
Did my patch get lost?


BR,
Ralf


> 
> Message: 2
> Date: Fri, 23 Jan 2009 10:43:01 -0800 (PST)
> From: Peter van Hardenberg <pvh at songbirdnest.com>
> Subject: [patch] allow empty ID3v2 frames
> To: taglib-devel <taglib-devel at kde.org>
> Message-ID: <18393694.311681232736181338.JavaMail.root at mail>
> Content-Type: text/plain; charset="utf-8"
> 
> Some media software out there is creating ID3v2 tags with a 0-length data field. This patch keeps TagLib from failing to parse those tags by allowing a 0-length data field. The ID3v2.4 spec I consulted specifically states that the length should be non-zero, but obviously improving support for data in the wild is desirable.
> 
> This passed all our tests and is working fine in Songbird, but I'm posting it here for wider consideration.
> 
> -pvh
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: allowemptyid3v2frames.patch
> Type: application/octet-stream
> Size: 980 bytes
> Desc: not available
> Url : http://mail.kde.org/pipermail/taglib-devel/attachments/20090123/a4a6b2d6/attachment-0001.dll 
> 
> ------------------------------
> 
> _______________________________________________
> taglib-devel mailing list
> taglib-devel at kde.org
> https://mail.kde.org/mailman/listinfo/taglib-devel
> 
> 
> End of taglib-devel Digest, Vol 55, Issue 12
> ********************************************