GNOME Bugzilla – Bug 595590
Conditional jump or move depends on uninitialised value in ff_h264_find_frame_end
Last modified: 2011-04-05 12:15:01 UTC
Original bug report: https://bugs.edge.launchpad.net/ubuntu/+source/gstreamer0.10-ffmpeg/+bug/432489 Got this error when valgrinding an audio/video call in Empathy: ==6603== Thread 19: ==6603== Conditional jump or move depends on uninitialised value(s) ==6603== at 0x3626673D: ff_h264_find_frame_end (h264_parser.c:54) ==6603== by 0x36266979: h264_parse (h264_parser.c:248) ==6603== by 0x360A26CB: av_parser_parse (parser.c:160) ==6603== by 0x35ACECCC: gst_ffmpegdec_chain (gstffmpegdec.c:2498) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x2B34A09C: gst_rtp_h264_depay_process (gstrtph264depay.c:622) ==6603== by 0x22F9D65E: (within /usr/lib/libgstrtp-0.10.so.0.18.0) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0xC43AB71: gst_base_transform_chain (gstbasetransform.c:2081) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x235D817E: gst_valve_chain (gstvalve.c:214) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== ==6603== Invalid read of size 8 ==6603== at 0x3626672B: ff_h264_find_frame_end (h264_parser.c:54) ==6603== by 0x36266979: h264_parse (h264_parser.c:248) ==6603== by 0x360A26CB: av_parser_parse (parser.c:160) ==6603== by 0x35ACECCC: gst_ffmpegdec_chain (gstffmpegdec.c:2498) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x22F9D672: (within /usr/lib/libgstrtp-0.10.so.0.18.0) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0xC43AB71: gst_base_transform_chain (gstbasetransform.c:2081) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x235D817E: gst_valve_chain (gstvalve.c:214) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== Address 0x170842b0 is 0 bytes inside a block of size 6 alloc'd ==6603== at 0x4C268FE: malloc (vg_replace_malloc.c:207) ==6603== by 0xB4C8382: g_malloc (gmem.c:131) ==6603== by 0x6E587A6: gst_buffer_new_and_alloc (gstbuffer.c:308) ==6603== by 0x2B34A01A: gst_rtp_h264_depay_process (gstrtph264depay.c:604) ==6603== by 0x22F9D65E: (within /usr/lib/libgstrtp-0.10.so.0.18.0) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0xC43AB71: gst_base_transform_chain (gstbasetransform.c:2081) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x235D817E: gst_valve_chain (gstvalve.c:214) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x231B4FE3: gst_rtp_jitter_buffer_loop (gstrtpjitterbuffer.c:1655) ==6603== ==6603== Invalid read of size 1 ==6603== at 0x4C27508: memcpy (mc_replace_strmem.c:402) ==6603== by 0x360A23B6: ff_combine_frame (string3.h:52) ==6603== by 0x36266994: h264_parse (h264_parser.c:250) ==6603== by 0x360A26CB: av_parser_parse (parser.c:160) ==6603== by 0x35ACECCC: gst_ffmpegdec_chain (gstffmpegdec.c:2498) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x22F9D672: (within /usr/lib/libgstrtp-0.10.so.0.18.0) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0xC43AB71: gst_base_transform_chain (gstbasetransform.c:2081) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x235D817E: gst_valve_chain (gstvalve.c:214) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== Address 0x17bcd327 is 1 bytes after a block of size 6 alloc'd ==6603== at 0x4C268FE: malloc (vg_replace_malloc.c:207) ==6603== by 0xB4C8382: g_malloc (gmem.c:131) ==6603== by 0x6E587A6: gst_buffer_new_and_alloc (gstbuffer.c:308) ==6603== by 0x2B34A01A: gst_rtp_h264_depay_process (gstrtph264depay.c:604) ==6603== by 0x22F9D65E: (within /usr/lib/libgstrtp-0.10.so.0.18.0) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0xC43AB71: gst_base_transform_chain (gstbasetransform.c:2081) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x235D817E: gst_valve_chain (gstvalve.c:214) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x6E8506C: gst_pad_chain_data_unchecked (gstpad.c:4042) ==6603== by 0x6E85B3D: gst_pad_push_data (gstpad.c:4271) ==6603== by 0x231B4FE3: gst_rtp_jitter_buffer_loop (gstrtpjitterbuffer.c:1655) ProblemType: Bug Architecture: amd64 Date: Fri Sep 18 14:36:43 2009 DistroRelease: Ubuntu 9.10 Package: gstreamer0.10-ffmpeg 0.10.8.2-1 ProcEnviron: PATH=(custom, user) LANG=fr_FR.UTF-8 SHELL=/bin/bash ProcVersionSignature: Ubuntu 2.6.31-10.32-generic SourcePackage: gstreamer0.10-ffmpeg Uname: Linux 2.6.31-10-generic x86_64
This looks like ffmpeg's h264 parser is just reading behind the provided memory areas... please file this bug against ffmpeg at https://roundup.ffmpeg.org/roundup/ffmpeg
We get the same fault in our Windows OSSBuild environment. Fault appears to be that the function 'gst_ffmpegdec_chain' in file gstffmpegdec.c does not resize the data buffer to include padding BEFORE calling 'av_parser_parse'. If the buffer is resized first we don't get a crash and the video is decoded correctly. Our fix is to move the block: if (ffmpegdec->do_padding) { /* add padding */ if (ffmpegdec->padded_size < bsize + FF_INPUT_BUFFER_PADDING_SIZE) { ffmpegdec->padded_size = bsize + FF_INPUT_BUFFER_PADDING_SIZE; ffmpegdec->padded = g_realloc (ffmpegdec->padded, ffmpegdec->padded_size); GST_LOG_OBJECT (ffmpegdec, "resized padding buffer to %d", ffmpegdec->padded_size); } memcpy (ffmpegdec->padded, bdata, bsize); memset (ffmpegdec->padded + bsize, 0, FF_INPUT_BUFFER_PADDING_SIZE); bdata = ffmpegdec->padded; } from below to above the call to av_parser_parse. This additionally needs the lines: // Assign pdata to the data variable allocated above. Reduces code changes required to move the buffer resizing code. pdata = data; to be inserted where the original padding block of code resided.
Can this please be reevaluated? Stephen, can you reply with a patch against the current git so the OSSBuild folks can get this included before their next release?
So #633831 should be closed as a duplicated of this bug, right?
I have been debugging this problem in windows these days without noticiying this bug report. At the end I get the same solution. As avcodec.h says 490 /** 491 * Required number of additionally allocated bytes at the end of the input bitstream for decoding. 492 * This is mainly needed because some optimized bitstream readers read 493 * 32 or 64 bit at once and could read over the end.<br> 494 * Note: If the first 23 bits of the additional bytes are not 0, then damaged 495 * MPEG bitstreams could cause overread and segfault. 496 */ 497 #define FF_INPUT_BUFFER_PADDING_SIZE 8 Also instead of calling do_padding, probably would be better to have a custom allocation buffer function in the sink pad. Also here it is my stack trace: (gdb) bt
+ Trace 226225
I'm still running with our patch in place on our build of OSSbuild but unfortunately I'm a bit out of sync with this area of the code. The current build we have is based on OSSBuild revision 921 (28/10/2010) so we're not that up-to-date either. I will try over the next few days to evaluate whether or not we still need the fix in place, since it was originally applied to a much older version of OSSBuild than we currently use (and the change was just re-applied automatically after updating OSSBuild). With regards to the custom allocation function on the sink pad, I'm unfortunately not familiar enough myself to put this in - our work with GStreamer really scratches the surface rather than getting deeply immersed.
Created attachment 182817 [details] [review] Stephen McNamara comment converted into a patch
I don't think this patch is 100% correct. The padding apparently needs to be done for the parsing too but if you only do it for the parsing the data that is passed to the decoder later might not have the correct padding.
Created attachment 185112 [details] [review] Fixed and improved buffer padding I have asked about ffmpeg usage in irc but got no answer. For this patch i assume this: - av_parser_parse output needs to be padded. - av_parser_parse returns a pointer that points inside the input buffer.
commit f63d36ade8fffc4a480d4637cf6649cee3dbac77 Author: Miguel Angel Cabrera Moya <madmac2501@gmail.com> Date: Mon Apr 4 16:37:42 2011 +0200 ffmpegdec: do buffer padding before parsing and before decoding FFMpeg parsing and decoding calls require to additionally allocate bytes at the end of the input bitstream and this padding must be initialized to zero. https://bugzilla.gnome.org/show_bug.cgi?id=595590