GNOME Bugzilla – Bug 606657
mpegtsmux crashes with h264parse in byte-stream mode
Last modified: 2012-05-18 15:02:55 UTC
Created attachment 151181 [details] Custom traces from tsmuxtream.c (around line 430: memcpy()) mpegtsmux crashes if h264parse output-format property is set to 1 (Bytestream Format) and added into pipeline to convert raw NAL unit format type of h264 stream (from MP4 container) into byte-stream format (suitable for MPEG2-TS container). This can be reproduced with the following pipeline: filesrc location=h264.mp4 ! qtdemux name=demux demux.video_00 ! queue ! h264parse output-format=1 ! mux.sink_0 demux.audio_00 ! queue ! mux.sink_1 mpegtsmux name=mux ! filesink location=transcoded.ts Callstack: 16 memcpy() 0x004da071 <- Segmentation fault 15 tsmux_stream_get_data() /usr/include/bits/string3.h:52 0x005b1d78 (should be tsmuxstream.c:430) 14 tsmux_write_stream_packet() /gst-plugins-bad/gst/mpegtsmux/tsmux/tsmux.c:787 0x005b0709 13 mpegtsmux_collected() /gst-plugins-bad/gst/mpegtsmux/mpegtsmux.c:660 0x005aeae8 12 gst_collect_pads_check_collected() /GStreamer/libs/gst/base/gstcollectpads.c:1139 0x00411bd7 11 gst_collect_pads_chain() /GStreamer/libs/gst/base/gstcollectpads.c:1367 0x00413ae7 10 gst_pad_chain_data_unchecked() /GStreamer/gst/gstpad.c:4122 0x003146d5 9 gst_pad_push_data() /GStreamer/gst/gstpad.c:4351 0x00315240 8 gst_queue_push_one() /GStreamer/plugins/elements/gstqueue.c:1083 0x003c6174 7 gst_queue_loop() /GStreamer/plugins/elements/gstqueue.c:1185 0x003c6174 6 gst_task_func() /GStreamer/gst/gsttask.c:238 0x0033bea5 5 default_func() /GStreamer/gst/gsttaskpool.c:70 0x0033d7a7 4 <symbol is not available> 0x0077a9af 3 <symbol is not available> 0x0077937f 2 start_thread() 0x0038d80e 1 clone() 0x005318de If ffmux_mpegts is used instead of mpegtsmux, everything goes smoothly; e.g. pipeline: location=h264.mp4 ! qtdemux name=demux demux.video_00 ! queue ! h264parse output-format=1 ! mux.video_0 demux.audio_00 ! queue ! mux.audio_0 ffmux_mpegts name=mux ! filesink location=transcoded.ts See attached trace files without h264parse byte-stream mode (no crash) and with byte-stream mode (crash). Code snippet from tsmuxstream.c (note added traces): ... /* Take as much as we can from the current buffer */ avail = stream->cur_buffer->size - stream->cur_buffer_consumed; cur = stream->cur_buffer->data + stream->cur_buffer_consumed; if (avail < len) { memcpy (buf, cur, avail); tsmux_stream_consume (stream, avail); buf += avail; len -= avail; } else { g_printf( "avail >= len (buf=0x%08x, cur=0x%08x, avail=%d, len=%u)\n", buf, cur, avail, len ); //hack trace memcpy (buf, cur, len); //430: crashes here!! g_printf( "memcpy ok\n" ); //hack trace tsmux_stream_consume (stream, len); len = 0; } ...
It seems that mpegtsmux is receiving buffers with invalid sizes, just logged one run that had a buffer with size=268435501 which looks suspicious.
Fixed by those commits. Module: gst-plugins-bad Branch: master Commit: ddeb6e17fda2184b39353ef1472845654eab3339 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=ddeb6e17fda2184b39353ef1472845654eab3339 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Tue Jan 12 09:35:24 2010 -0300 mpegtsmux: Do not crash on misinterpreted h264 Avoid crashing when bytestream h264 is interpreted as avc format h264 Fixes #606657 commit e6ab0787856558588c9141afd25ab4fab30e8bc0 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Tue Jan 12 09:58:48 2010 -0300 h264parse: remove codec_data if output is bytestream codec_data should be removed from the src pad caps if the output is in bytestream format Fixes #606657
Thiago, is your fix really enough? I suppose the buffer over-write can still happen, and corrupting memory, because the check is done in the very end of the function. Am I missing something?
You're right, I'm working on it.
In fact, are you having problems? There is a 'MIN' over there that should prevent it from happening, the problem was indeed that it prevented and then increased the buffer size even though it shouldn't.
Yes, I am getting what looks like over-write according to valgrind when using an h264 non "byte-stream" format. Using h264parse to transform in byte-stream solved the issue (I think it skip the h264 part of tsmux in that case, I am not sure). I can provide a backtrace tomorrow.
A sample and the pipeline would be useful as well.
I don't know enough about es/byte-stream/ts/avc/etc.. so excuse my ignorance in this domain, I don't care much about the crash anymore, but it's still nicer to avoid crashing. Anyway, here is file with the corresponding pipeline that cause memory corruption and ultimately crashes. http://www.gnome.org/~malureau/606657.gdp GST_DEBUG=*tsmux*:3 gst-launch filesrc location=./606657.gdp ! gdpdepay ! mpegtsmux ! fakesink -v
This fix has just been committed, are you still having problems with git version? Module: gst-plugins-bad Branch: master Commit: 6f1ee59df6026b3a3f2defb1407457d56c3fa2fc URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=6f1ee59df6026b3a3f2defb1407457d56c3fa2fc Author: Julien Moutte <julien@fluendo.com> Date: Tue Feb 2 12:23:24 2010 +0100 mpegtsmux: generate SPS/PPS header once and fix overflow Some H264 packets can be as small as 5 bytes for repeated frames. In such a situation the output buffer size was not big enough (5*2) to fit the SPS/PPS header and the start codes. This corrupts the ES stream. We now generate the SPS/PPS only once which is much more optimal and we now know the size of the header to calculate the output buffer size more safely.
For the record, valgrind's happy with the file and command line in comment 8 as of today's git.
And valgrind is still happy these days, and also with freshly encoded (byte-stream) input from x264enc, so all of the above will then have taken care of this. Feel free to re-open if evidence to the contrary.