[FFmpeg-devel] [PATCH] prevent buffer overflow with large a/mulaw frames

[Baptiste Coudurier]

On 07/25/2009 11:45 PM, Peter Ross wrote:
> On Sun, Jul 26, 2009 at 03:32:59PM +1000, Peter Ross wrote:
>> On Sat, Jul 25, 2009 at 09:42:52PM -0700, Baptiste Coudurier wrote:
>>> Hi Peter,
>>>
>>> On 07/25/2009 09:19 PM, Peter Ross wrote:
>>>> Hi,
>>>>
>>>> This patch prevents alaw/mulaw decoders from writing beyond the output buffer.
>>>>
>>> I think output buffer size is stored in *data_size.
>>> Code should check against this, but it seems it is already. Is the check
>>> wrong ?
>>>
>>> Code is:
>>>      buf_size= FFMIN(buf_size, *data_size/2);
>>>      *data_size=0;
>>>
>>>      n = buf_size/sample_size;
>> You are correct, the bug actually exists in the *encoder* where there is no
>> such constraint on n. Updated patch enclosed.
>
> Actually... this bug originates within ffmpeg. The audio output buf_size passed
> to codec->encode does NOT reflect the size of the samples buffer.
>
> Per suggestion from Baptiste, I have modified ffmpeg.c to auto realloc the
> encoder buffer. Please disregard the previous libavcodec/pcm.c diffs in this
> email thread.
>
> -- Peter
> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
>
>
> ------------------------------------------------------------------------
>
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 19454)
> +++ ffmpeg.c	(working copy)
> @@ -226,6 +226,8 @@
>   static uint8_t *audio_buf;
>   static uint8_t *audio_out;
>   static uint8_t *audio_out2;
> +static int audio_out_size;
> +static int audio_out2_size;
>
>   static short *samples;
>
> @@ -559,7 +561,6 @@
>                            unsigned char *buf, int size)
>   {
>       uint8_t *buftmp;
> -    const int audio_out_size= 4*MAX_AUDIO_PACKET_SIZE;
>
>       int size_out, frame_bytes, ret;
>       AVCodecContext *enc= ost->st->codec;
> @@ -570,8 +571,10 @@
>       /* SC: dynamic allocation of buffers */
>       if (!audio_buf)
>           audio_buf = av_malloc(2*MAX_AUDIO_PACKET_SIZE);
> -    if (!audio_out)
> -        audio_out = av_malloc(audio_out_size);
> +    if (!audio_out || size>  audio_out_size) {

Nitpick: Space before >

> +        audio_out_size = FFMAX(size, 4*MAX_AUDIO_PACKET_SIZE);
> +        audio_out = av_realloc(audio_out, audio_out_size);
> +    }
>       if (!audio_buf || !audio_out)
>           return;               /* Should signal an error ! */
>
> @@ -596,9 +599,11 @@
>   #define MAKE_SFMT_PAIR(a,b) ((a)+SAMPLE_FMT_NB*(b))
>       if (!ost->audio_resample&&  dec->sample_fmt!=enc->sample_fmt&&
>           MAKE_SFMT_PAIR(enc->sample_fmt,dec->sample_fmt)!=ost->reformat_pair) {
> +        if (!audio_out2 || size>  audio_out2_size) {
> +            audio_out2_size = FFMAX(size, 4*MAX_AUDIO_PACKET_SIZE);
> +            audio_out2 = av_realloc(audio_out2, audio_out2_size);
> +        }

av_fast_realloc may look cleaner in this situation.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org