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 595590 - Conditional jump or move depends on uninitialised value in ff_h264_find_frame_end
Conditional jump or move depends on uninitialised value in ff_h264_find_frame...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-libav
git master
Other Linux
: Normal normal
: 0.10.12
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-09-18 14:25 UTC by Guillaume Desmottes
Modified: 2011-04-05 12:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Stephen McNamara comment converted into a patch (2.75 KB, patch)
2011-03-08 13:22 UTC, Miguel Angel Cabrera Moya
none Details | Review
Fixed and improved buffer padding (3.11 KB, patch)
2011-04-04 14:48 UTC, Miguel Angel Cabrera Moya
committed Details | Review

Description Guillaume Desmottes 2009-09-18 14:25:43 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
Comment 1 Sebastian Dröge (slomo) 2009-09-22 06:42:08 UTC
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
Comment 2 Stephen McNamara 2009-12-14 14:56:19 UTC
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.
Comment 3 David Hoyt 2011-03-07 19:17:54 UTC
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?
Comment 4 Andoni Morales 2011-03-07 20:14:13 UTC
So #633831 should be closed as a duplicated of this bug, right?
Comment 5 Miguel Angel Cabrera Moya 2011-03-08 09:06:06 UTC
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
  • #0 ff_h264_find_frame_end
  • #1 h264_parse
  • #2 av_parser_parse2
  • #3 av_parser_parse
  • #4 ??
    from C:\Archivos
  • #5 libgstreamer-0!gst_pad_load_and_link
    from C:\Archivos
  • #6 ??
  • #7 libgstreamer-0!gst_pad_chain_list
    from C:\Archivos
  • #8 ??
  • #9 ??
  • #10 libgstreamer-0!gst_pad_push
    from C:\Archivos
  • #11 ??
  • #12 ??
  • #13 libgstrtp-0!gst_base_rtp_depayload_push
    from C:\Archivos
  • #14 ??

Comment 6 Stephen McNamara 2011-03-08 09:35:39 UTC
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.
Comment 7 Miguel Angel Cabrera Moya 2011-03-08 13:22:02 UTC
Created attachment 182817 [details] [review]
Stephen McNamara comment converted into a patch
Comment 8 Sebastian Dröge (slomo) 2011-04-01 09:14:47 UTC
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.
Comment 9 Miguel Angel Cabrera Moya 2011-04-04 14:48:33 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2011-04-05 12:15:01 UTC
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