GNOME Bugzilla – Bug 573649
Buffer overflow in gst gstffmpegaudioresample
Last modified: 2009-03-06 11:49:54 UTC
To reproduce, run: gst-launch neonhttpsrc location=http://www.cs.ucl.ac.uk/teaching/GZ05/samples/tone.wav ! wavparse ! audioconvert ! ffaudioresample ! audio/x-raw-int,rate=44100 ! autoaudiosink If you run this pipeline using Valgrind, you'll see output like: ==6700== Invalid write of size 2 ==6700== at 0x6D4DEED: audio_resample (in /usr/lib/i686/cmov/libavcodec.so.51.50.0) ==6700== Address 0x77ebb9e is 2 bytes after a block of size 12,700 alloc'd ==6700== at 0x4025D2E: malloc (vg_replace_malloc.c:207) ==6700== by 0x5126B67: g_try_malloc (gmem.c:199) ==6700== by 0x4ED4DE4: gst_buffer_try_new_and_alloc (in /usr/lib/libgstreamer-0.10.so.0.18.0) To narrow down the problem, apply this patch, and the assertion will hit when you run the above pipeline: --- gstffmpegaudioresample.c 2008-11-08 16:45:25.000000000 +0100 +++ ../../../gst-ffmpeg-0.10.6.orig/ext/ffmpeg/gstffmpegaudioresample.c 2009-03-01 17:02:02.000000000 +0100 @@ -281,6 +281,9 @@ gst_ffmpegaudioresample_transform (GstBa GST_BUFFER_DURATION (outbuf) = gst_util_uint64_scale (ret, GST_SECOND, resample->out_rate); + + g_assert(ret * 2 * resample->out_channels <= GST_BUFFER_SIZE(outbuf)); + GST_BUFFER_SIZE (outbuf) = ret * 2 * resample->out_channels; GST_LOG_OBJECT (resample, "Output buffer duration:%" GST_TIME_FORMAT,
Created attachment 129794 [details] [review] patch in report that exposes the bug
Adding a few bytes (I tried 16) to the size returned by gst_ffmpegaudioresample_transform_size() fixes this bug, so I think there is a discrepancy between the way Gstreamer and ffmpeg calculate the output bytes and/or samples.
I do indeed see the "Invalid write of size 2" when run through valgrind... but it doesn't fail or segfault. Is it as critical an issue as that ?
Sometimes it segfaults, sometimes it doesn't. That's the thing with small buffer overflows. I think on OpenBSD it's guaranteed to segfault. Whether you consider a "sometimes segfault" a critical issue or not I leave to you.
Definitely critical then. Will cook up a patch.
Created attachment 130185 [details] [review] Add a 64bytes padding to the output buffer This patch adds 64 bytes to the output buffer used by the resampling function. I noticed at the same time that we might want to switch to av_resample* (as opposed to audio_resample*).
commit df6fb6867ed19bfc5f735518452153d5e81efc04 Author: Edward Hervey <bilboed@bilboed.com> Date: Fri Mar 6 12:47:12 2009 +0100 ffaudioresample: Add padding to output buffer. Fixes #573649 The internal resampling functions seem to require a slightly bigger buffer for output than what we require. Therefore we give it an extra 64bytes (although 16 should have been enough).