GNOME Bugzilla – Bug 510982
gst_tag_demux_trim_buffer: invalid return value
Last modified: 2008-07-06 18:55:47 UTC
Hi, I continue my fuzzing party (see #510592). Today I found a bug in ID3 parser. See attached patch to fix the bug.
Created attachment 103309 [details] [review] Path to fix the bug: in case of error, use FALSE return value
Created attachment 103311 [details] MP3 (ID3) raising "** ERROR **: file gstid3demux.c: line 1112 (gst_id3demux_read_range): assertion failed: (*buffer != NULL)" assertion
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?
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.
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.
It's not a blocker in any case - it's neither a regression, nor something that's likely to occur in normal usage.
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.
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.
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.
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.