Bug 137635

Summary: [PATCH] crash reading broken/short text frame
Product: [Unmaintained] taglib Reporter: Tim-Philipp M <t.i.m>
Component: generalAssignee: Scott Wheeler <wheeler>
Status: RESOLVED FIXED    
Severity: crash    
Priority: NOR    
Version First Reported In: unspecified   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:
Attachments: proposed fix: don't crash if there isn't enough data

Description Tim-Philipp M 2006-11-20 20:22:45 UTC 
Version:            (using KDE Devel)
Installed from:    Compiled sources
OS:                Linux

TextIdentificationFrame::parseFields(const ByteVector &data) doesn't check for short data before accessing the data. This can lead to crashes when reading broken ID3v2 frames.

See here for one such case:

 http://bugzilla.gnome.org/show_bug.cgi?id=374036


The GStreamer id3v2mux plugin code does something like this in this case:

  ID3v2::FrameFactory * factory = ID3v2::FrameFactory::instance ();
  ByteVector bytes ((char *) tenc_frame_data, 11);
  frame = factory->createFrame (bytes, (TagLib::uint) version);

where tenc_frame_data contains a too short TENC frame (exactly as it is found in the file).

This crashes right at the beginning of parseFields() at
 
   d->textEncoding = String::Type(data[0]);

data.size() is 0 here.

Under normal circumstances this is unlikely to be noticed, since there's usually data from the following frames in the byte vector. In this case, it's not, however.

Straight-forward patch that fixes the issue attached.
Comment 1 Tim-Philipp M 2006-11-20 20:23:39 UTC 
Created attachment 18629 [details]
proposed fix: don't crash if there isn't enough data
Comment 2 Lukáš Lalinský 2007-02-05 10:29:28 UTC 
I think this is a better solution:

=== modified file 'taglib/mpeg/id3v2/id3v2framefactory.cpp'
--- taglib/mpeg/id3v2/id3v2framefactory.cpp     2007-02-04 21:52:47 +0000
+++ taglib/mpeg/id3v2/id3v2framefactory.cpp     2007-02-05 09:27:44 +0000
@@ -70,7 +70,7 @@
   // A quick sanity check -- make sure that the frameID is 4 uppercase Latin1
   // characters.  Also make sure that there is data in the frame.
 
-  if(!frameID.size() == (version < 3 ? 3 : 4) || header->frameSize() <= 0) {
+  if(!frameID.size() == (version < 3 ? 3 : 4) || header->frameSize() <= (header->dataLengthIndicator() ? 4 : 0)) {
     delete header;
     return 0;
   }
Comment 3 Scott Wheeler 2007-02-13 10:58:40 UTC 
SVN commit 633133 by wheeler:

Don't try to parse invalid frames.

BUG:137635


 M  +5 -0      textidentificationframe.cpp  


--- trunk/kdesupport/taglib/taglib/mpeg/id3v2/frames/textidentificationframe.cpp #633132:633133
@@ -94,6 +94,11 @@
 
 void TextIdentificationFrame::parseFields(const ByteVector &data)
 {
+  // Don't try to parse invalid frames
+
+  if(data.size() < 2)
+    return;
+
   // read the string data type (the first byte of the field data)
 
   d->textEncoding = String::Type(data[0]);