[FFmpeg-devel] avcodec/alsdec: properly allocate raw_buffer

Submitted by Paul B Mahol on July 1, 2017, 1:38 a.m.

Details

Message ID 20170701013821.5448-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol July 1, 2017, 1:38 a.m.
This also reverts 18f94df8.

Fixes #5297.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/alsdec.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Michael Niedermayer July 1, 2017, 12:23 p.m.
On Sat, Jul 01, 2017 at 03:38:21AM +0200, Paul B Mahol wrote:
> This also reverts 18f94df8.
> 
> Fixes #5297.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/alsdec.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

This avoids the crash with the sample, i will leave review to thilo
as he knows ALS much better than i do

thx

[...]
Thilo Borgmann July 1, 2017, 4:47 p.m.
Am 01.07.17 um 14:23 schrieb Michael Niedermayer:
> On Sat, Jul 01, 2017 at 03:38:21AM +0200, Paul B Mahol wrote:
>> This also reverts 18f94df8.
>>
>> Fixes #5297.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavcodec/alsdec.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> This avoids the crash with the sample, i will leave review to thilo
> as he knows ALS much better than i do

Will test tomorrow, also the other ALS patch.

However, I suspect it works for the fuzzed sample only becuase so many values
are not actually used thus assuming/defining them equal to 0 works. Will see if
I can create a valid sample that actually uses these since specs allow for it.

Might take a while to figure out and if this backfires some day because we don't
handle these extra samples properly the bug will not be found this easily...

-Thilo
Thilo Borgmann July 2, 2017, 2:03 p.m.
Am 01.07.17 um 03:38 schrieb Paul B Mahol:
> This also reverts 18f94df8.
> 
> Fixes #5297.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/alsdec.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> [...]
> @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>      channel_size      = sconf->frame_length + sconf->max_order;
>  
>      ctx->prev_raw_samples = av_malloc_array(sconf->max_order, sizeof(*ctx->prev_raw_samples));
> -    ctx->raw_buffer       = av_mallocz_array(avctx->channels * channel_size, sizeof(*ctx->raw_buffer));
> +    ctx->raw_buffer       = av_mallocz_array(avctx->channels * channel_size + sconf->max_order, sizeof(*ctx->raw_buffer));
>      ctx->raw_samples      = av_malloc_array(avctx->channels, sizeof(*ctx->raw_samples));

This looks like guessing only about a needed overhead to the raw buffer. What is
needed and actually done, is an overhead of max_order and that is already added
to the buffer length three lines above the proposed change. So this is nothing
more than guessing an overhead of 2*max_order should be enough but it does not
resolve knowingly any cause of the bug.

Therefore no, this still does not get applied until the actual reason of the bug
is found and properly fixed. I've got less time than I thought today and I could
not dive in as deep as I want. I cannot predict if there will be more time for
me to do it soon. So Paul, I'd appreciate if you could solve this bug but I will
only accept a proper patch and no guesswork.

-Thilo

p.s. I have the original pred.wav used by the ticket author and for me both
rm22rev2 and rm23 of the reference codec fail to decode pred.mp4 properly. Does
anyone else have the same problem with the reference codecs?
Paul B Mahol July 2, 2017, 2:13 p.m.
On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> Am 01.07.17 um 03:38 schrieb Paul B Mahol:
>> This also reverts 18f94df8.
>>
>> Fixes #5297.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavcodec/alsdec.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>> [...]
>> @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext
>> *avctx)
>>      channel_size      = sconf->frame_length + sconf->max_order;
>>
>>      ctx->prev_raw_samples = av_malloc_array(sconf->max_order,
>> sizeof(*ctx->prev_raw_samples));
>> -    ctx->raw_buffer       = av_mallocz_array(avctx->channels *
>> channel_size, sizeof(*ctx->raw_buffer));
>> +    ctx->raw_buffer       = av_mallocz_array(avctx->channels *
>> channel_size + sconf->max_order, sizeof(*ctx->raw_buffer));
>>      ctx->raw_samples      = av_malloc_array(avctx->channels,
>> sizeof(*ctx->raw_samples));
>
> This looks like guessing only about a needed overhead to the raw buffer.
> What is
> needed and actually done, is an overhead of max_order and that is already
> added
> to the buffer length three lines above the proposed change. So this is
> nothing
> more than guessing an overhead of 2*max_order should be enough but it does
> not
> resolve knowingly any cause of the bug.
>
> Therefore no, this still does not get applied until the actual reason of the
> bug
> is found and properly fixed. I've got less time than I thought today and I
> could
> not dive in as deep as I want. I cannot predict if there will be more time
> for
> me to do it soon. So Paul, I'd appreciate if you could solve this bug but I
> will
> only accept a proper patch and no guesswork.

This is not guesswork, The raw buffers needs this enough bytes.

See this line:
    ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order;

>
> -Thilo
>
> p.s. I have the original pred.wav used by the ticket author and for me both
> rm22rev2 and rm23 of the reference codec fail to decode pred.mp4 properly.
> Does
> anyone else have the same problem with the reference codecs?

Reference code crashes all the time.
Thilo Borgmann July 2, 2017, 2:16 p.m.
Am 02.07.17 um 16:13 schrieb Paul B Mahol:
> On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>> Am 01.07.17 um 03:38 schrieb Paul B Mahol:
>>> This also reverts 18f94df8.
>>>
>>> Fixes #5297.
>>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>  libavcodec/alsdec.c | 7 +------
>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>> [...]
>>> @@ -2062,7 +2057,7 @@ static av_cold int decode_init(AVCodecContext
>>> *avctx)
>>>      channel_size      = sconf->frame_length + sconf->max_order;
>>>
>>>      ctx->prev_raw_samples = av_malloc_array(sconf->max_order,
>>> sizeof(*ctx->prev_raw_samples));
>>> -    ctx->raw_buffer       = av_mallocz_array(avctx->channels *
>>> channel_size, sizeof(*ctx->raw_buffer));
>>> +    ctx->raw_buffer       = av_mallocz_array(avctx->channels *
>>> channel_size + sconf->max_order, sizeof(*ctx->raw_buffer));
>>>      ctx->raw_samples      = av_malloc_array(avctx->channels,
>>> sizeof(*ctx->raw_samples));
>>
>> This looks like guessing only about a needed overhead to the raw buffer.
>> What is
>> needed and actually done, is an overhead of max_order and that is already
>> added
>> to the buffer length three lines above the proposed change. So this is
>> nothing
>> more than guessing an overhead of 2*max_order should be enough but it does
>> not
>> resolve knowingly any cause of the bug.
>>
>> Therefore no, this still does not get applied until the actual reason of the
>> bug
>> is found and properly fixed. I've got less time than I thought today and I
>> could
>> not dive in as deep as I want. I cannot predict if there will be more time
>> for
>> me to do it soon. So Paul, I'd appreciate if you could solve this bug but I
>> will
>> only accept a proper patch and no guesswork.
> 
> This is not guesswork, The raw buffers needs this enough bytes.
> 
> See this line:
>     ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order;

No. The first max_order samples in the raw_buffer are used and the additional
size needed for them is already accounted for in calculation of channel_size
some lines above the allocation.
This leads to raw_samples[0][0] is max_order sample values after the beginning
of the buffer so that negative indices of raw_samples[0] work properly.

-Thilo

Patch hide | download patch | download mbox

diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
index d95e30d..4a8f13d 100644
--- a/libavcodec/alsdec.c
+++ b/libavcodec/alsdec.c
@@ -705,11 +705,6 @@  static int read_var_block_data(ALSDecContext *ctx, ALSBlockData *bd)
         } else {
             *bd->opt_order = sconf->max_order;
         }
-        if (*bd->opt_order > bd->block_length) {
-            *bd->opt_order = bd->block_length;
-            av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
-            return AVERROR_INVALIDDATA;
-        }
         opt_order = *bd->opt_order;
 
         if (opt_order) {
@@ -2062,7 +2057,7 @@  static av_cold int decode_init(AVCodecContext *avctx)
     channel_size      = sconf->frame_length + sconf->max_order;
 
     ctx->prev_raw_samples = av_malloc_array(sconf->max_order, sizeof(*ctx->prev_raw_samples));
-    ctx->raw_buffer       = av_mallocz_array(avctx->channels * channel_size, sizeof(*ctx->raw_buffer));
+    ctx->raw_buffer       = av_mallocz_array(avctx->channels * channel_size + sconf->max_order, sizeof(*ctx->raw_buffer));
     ctx->raw_samples      = av_malloc_array(avctx->channels, sizeof(*ctx->raw_samples));
 
     if (sconf->floating) {