Bug 137635 - [PATCH] crash reading broken/short text frame
Summary: [PATCH] crash reading broken/short text frame
Status: RESOLVED FIXED
Alias: None
Product: taglib
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR crash
Target Milestone: ---
Assignee: Scott Wheeler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-20 20:22 UTC by Tim-Philipp M
Modified: 2007-02-13 10:58 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
proposed fix: don't crash if there isn't enough data (517 bytes, patch)
2006-11-20 20:23 UTC, Tim-Philipp M
Details

Note You need to log in before you can comment on or make changes to this bug.
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]);