[FFmpeg-devel] Buffer overflow in ALS decoder

[Thilo Borgmann]

Am 18.02.10 00:07, schrieb Reimar D?ffinger:
> On Wed, Feb 17, 2010 at 11:27:43PM +0100, Thilo Borgmann wrote:
>> Index: libavcodec/alsdec.c
>> ===================================================================
>> --- libavcodec/alsdec.c	(Revision 21849)
>> +++ libavcodec/alsdec.c	(Arbeitskopie)
>> @@ -1563,7 +1563,7 @@
>>      // allocate and assign channel data buffer for mcc mode
>>      if (sconf->mc_coding) {
>>          ctx->chan_data_buffer  = av_malloc(sizeof(*ctx->chan_data_buffer) *
>> -                                           num_buffers);
>> +                                           num_buffers * num_buffers);
>>          ctx->chan_data         = av_malloc(sizeof(ALSChannelData) *
>>                                             num_buffers);
> 
> Just fix this as well, you are allocating too much, chan_data is ALSChannelData **,
> thus it should be sizeof(ALSChannelData *), not sizeof(ALSChannelData) - or
> actually better for consistency sizeof(*ctx->chan_data).

Indeed! Tested & applied.


> Though assuming there is an upper limit on num_buffers and its not above
> 40 or so, I'd suggest to have chan_data a fixed-size array in the context
> instead of allocating it.

Well there is none and conformance files include one with 512 channels (
= num_buffers in that case)...

> 
>> @@ -1576,7 +1576,7 @@
>>          }
>>  
>>          for (c = 0; c < num_buffers; c++)
>> -            ctx->chan_data[c] = ctx->chan_data_buffer + c;
>> +            ctx->chan_data[c] = ctx->chan_data_buffer + c * num_buffers;
> 
> That was actually one possibility I was considering, I just thought it
> couldn't be something that simple.

It didn't reveal it's simple nature during development so it was very
good in hiding. Actually I wonder how even the two channel files have
worked well so far... in fact it was a _big_ logical error.

-Thilo