After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 510982 - gst_tag_demux_trim_buffer: invalid return value
gst_tag_demux_trim_buffer: invalid return value
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-01-21 09:05 UTC by Victor STINNER
Modified: 2008-07-06 18:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Path to fix the bug: in case of error, use FALSE return value (486 bytes, patch)
2008-01-21 09:06 UTC, Victor STINNER
none Details | Review
MP3 (ID3) raising "** ERROR **: file gstid3demux.c: line 1112 (gst_id3demux_read_range): assertion failed: (*buffer != NULL)" assertion (2.94 KB, application/octet-stream)
2008-01-21 09:10 UTC, Victor STINNER
  Details

Description Victor STINNER 2008-01-21 09:05:59 UTC
Hi, I continue my fuzzing party (see #510592). Today I found a bug in ID3 parser. See attached patch to fix the bug.
Comment 1 Victor STINNER 2008-01-21 09:06:52 UTC
Created attachment 103309 [details] [review]
Path to fix the bug: in case of error, use FALSE return value
Comment 2 Victor STINNER 2008-01-21 09:10:20 UTC
Created attachment 103311 [details]
MP3 (ID3) raising "** ERROR **: file gstid3demux.c: line 1112 (gst_id3demux_read_range): assertion failed: (*buffer != NULL)" assertion
Comment 3 Tim-Philipp Müller 2008-01-21 12:12:22 UTC
With CVS of -base and -good I get:

$ gst-launch-0.10 playbin uri=file:///home/tpm/samples/510982-fuzzed.mp3
...
ERROR: from element /id3demux0: Failed to read tag: not enough data
Additional debug info:
gsttagdemux.c(1039): gst_tag_demux_pull_start_tag (): /playbin0/decodebin0/id3demux0
ERROR: pipeline doesn't want to preroll.

What versions of -base and -good are you using?
Comment 4 Victor STINNER 2008-01-21 12:56:18 UTC
Hi, I'm using:
 * gstreamer0.10-plugins-base : 0.10.14-1ubuntu3
 * gstreamer0.10-plugins-good : 0.10.6-0ubuntu4

Attached MP3 may only fails with these versions (maybe also older versions), but the fix is still valid with CVS (did you read it?). My patch is for CVS HEAD.
Comment 5 Tim-Philipp Müller 2008-01-21 13:28:30 UTC
Of course I read the patch. It did not immediately look obviously correct to me though, so at this point I'm mostly just trying to determine whether this is a blocker or not.

So I'm basically looking for (a) either a sample that causes the abort() with CVS of -base and either -good 0.10.6 or -good CVS, (b) an analysis that shows under what circumstances the g_assert() line in question could still be triggered.
Comment 6 Jan Schmidt 2008-01-21 17:31:14 UTC
It's not a blocker in any case - it's neither a regression, nor something that's likely to occur in normal usage.
Comment 7 Victor STINNER 2008-02-03 23:15:27 UTC
I don't understand why you discuss about bug priority since my patch just changes one instruction. The patch is short, easy to understand and fix a real bug.
Comment 8 Jan Schmidt 2008-02-07 20:32:48 UTC
We were discussing the bug priority because you set the blocker flag - we reserve the blocker flag for use on bugs of sufficient priority as to prevent an impending release, and I was making releases at the time.

Someone needs to actually spend time on this bug - as Tim explained, it's not clear to us that the patch actually fixes a bug. Neither of us can see the crash you describe with CVS.
Comment 9 Tim-Philipp Müller 2008-02-07 20:49:39 UTC
I told him on IRC to set the blocker flag, so that's on me.

I haven't committed this yet for two reasons:

 - I think the patch may not be correct in its current form:
   in pull mode, this situation should never happen, unless
   there's an integer overflow (which should be caught at the
   right spot then and not here); in push mode, the TRUE
   return + NULL buffer is used to signal 'need more data',
   during typefinding, so returning FALSE would break things
   if buffers come in in certain chunkings. (Note: this is
   all from memory, I looked at it a while back)

 - it's not obvious to me we still need to handle this seeing
   that it's caught elsewhere.
Comment 10 Sebastian Dröge (slomo) 2008-07-06 18:55:47 UTC
Ok, looking at it now the patch is not necessary anymore and instead will break things. As Tim has already written the TRUE + NULL means that we need more data and will only ever happen in push mode. If it happens in pull mode something is really really going wrong and the assertion is fine (and the reason why it was hit should be fixed).

Closing this bug as OBSOLETE now.