[FFmpeg-devel] avcodec/alsdec: use correct variable when checking for overwrite

Submitted by Paul B Mahol on June 30, 2017, 4:43 p.m.

Details

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

Commit Message

Paul B Mahol June 30, 2017, 4:43 p.m.
Fixes #5297.

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

Comments

Thilo Borgmann June 30, 2017, 5:14 p.m.
Am 30.06.17 um 18:43 schrieb Paul B Mahol:
> Fixes #5297.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/alsdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> index d95e30d..a925502 100644
> --- a/libavcodec/alsdec.c
> +++ b/libavcodec/alsdec.c
> @@ -705,8 +705,8 @@ 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;
> +        if (*bd->opt_order > sconf->max_order) {
> +            *bd->opt_order = sconf->max_order;
>              av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
>              return AVERROR_INVALIDDATA;
>          }


This check will never fire because in all cases this check has already been
applied or opt_order is explicitly set to equal max_order. See code above.

Paul, seriously, are you just trying to get me even more busy?

-Thilo
Paul B Mahol June 30, 2017, 5:34 p.m.
On 6/30/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> Am 30.06.17 um 18:43 schrieb Paul B Mahol:
>> Fixes #5297.
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavcodec/alsdec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
>> index d95e30d..a925502 100644
>> --- a/libavcodec/alsdec.c
>> +++ b/libavcodec/alsdec.c
>> @@ -705,8 +705,8 @@ 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;
>> +        if (*bd->opt_order > sconf->max_order) {
>> +            *bd->opt_order = sconf->max_order;
>>              av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
>>              return AVERROR_INVALIDDATA;
>>          }
>
>
> This check will never fire because in all cases this check has already been
> applied or opt_order is explicitly set to equal max_order. See code above.
>
> Paul, seriously, are you just trying to get me even more busy?

Does it fixes crash or not?
Thilo Borgmann June 30, 2017, 5:36 p.m.
Am 30.06.17 um 19:34 schrieb Paul B Mahol:
> On 6/30/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>> Am 30.06.17 um 18:43 schrieb Paul B Mahol:
>>> Fixes #5297.
>>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>  libavcodec/alsdec.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
>>> index d95e30d..a925502 100644
>>> --- a/libavcodec/alsdec.c
>>> +++ b/libavcodec/alsdec.c
>>> @@ -705,8 +705,8 @@ 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;
>>> +        if (*bd->opt_order > sconf->max_order) {
>>> +            *bd->opt_order = sconf->max_order;
>>>              av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
>>>              return AVERROR_INVALIDDATA;
>>>          }
>>
>>
>> This check will never fire because in all cases this check has already been
>> applied or opt_order is explicitly set to equal max_order. See code above.
>>
>> Paul, seriously, are you just trying to get me even more busy?
> 
> Does it fixes crash or not?

I assume you tested and it does for #5297. However, as I tried to outline, your
patch here is equal to remove this check alltogether and therefore does not fix
why it has been added.

-Thilo
Michael Niedermayer June 30, 2017, 10:14 p.m.
On Fri, Jun 30, 2017 at 07:34:32PM +0200, Paul B Mahol wrote:
> On 6/30/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> > Am 30.06.17 um 18:43 schrieb Paul B Mahol:
> >> Fixes #5297.
> >>
> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> ---
> >>  libavcodec/alsdec.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> >> index d95e30d..a925502 100644
> >> --- a/libavcodec/alsdec.c
> >> +++ b/libavcodec/alsdec.c
> >> @@ -705,8 +705,8 @@ 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;
> >> +        if (*bd->opt_order > sconf->max_order) {
> >> +            *bd->opt_order = sconf->max_order;
> >>              av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
> >>              return AVERROR_INVALIDDATA;
> >>          }
> >
> >
> > This check will never fire because in all cases this check has already been
> > applied or opt_order is explicitly set to equal max_order. See code above.
> >
> > Paul, seriously, are you just trying to get me even more busy?
> 
> Does it fixes crash or not?

i can confirm that this does not fix it.
It crashes with the patch


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
index d95e30d..a925502 100644
--- a/libavcodec/alsdec.c
+++ b/libavcodec/alsdec.c
@@ -705,8 +705,8 @@  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;
+        if (*bd->opt_order > sconf->max_order) {
+            *bd->opt_order = sconf->max_order;
             av_log(avctx, AV_LOG_ERROR, "Predictor order too large.\n");
             return AVERROR_INVALIDDATA;
         }