[FFmpeg-devel,4/4] avcodec/apedec: Fix multiple integer overflows in filter_3800()

Submitted by Michael Niedermayer on June 16, 2019, 9:57 a.m.

Details

Message ID 20190616095701.31384-4-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 16, 2019, 9:57 a.m.
Fixes: left shift of negative value -4
Fixes: signed integer overflow: -15091694 * 167 cannot be represented in type 'int'
Fixes: signed integer overflow: 1898547155 + 453967445 cannot be represented in type 'int'
Fixes: 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/apedec.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Lynne June 16, 2019, 10:30 a.m.
Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:

> Fixes: left shift of negative value -4
> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in type 'int'
> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be represented in type 'int'
> Fixes: 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/apedec.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> index 61ebfdafd5..f1bfc634c2 100644
> --- a/libavcodec/apedec.c
> +++ b/libavcodec/apedec.c
> @@ -859,22 +859,22 @@ static av_always_inline int filter_3800(APEPredictor *p,
>  return predictionA;
>  }
>  d2 =  p->buf[delayA];
> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) << 3);
> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) * 8);
>  d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>  d4 =  p->buf[delayB];
>  
> -    predictionA = d0 * p->coeffsA[filter][0] +
> -                  d1 * p->coeffsA[filter][1] +
> -                  d2 * p->coeffsA[filter][2];
> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
> +                  d1 * (unsigned)p->coeffsA[filter][1] +
> +                  d2 * (unsigned)p->coeffsA[filter][2];
>  
>  sign = APESIGN(decoded);
>  p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>  p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>  p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>  
> -    predictionB = d3 * p->coeffsB[filter][0] -
> -                  d4 * p->coeffsB[filter][1];
> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
> +                  d4 * (unsigned)p->coeffsB[filter][1];
>  p->lastA[filter] = decoded + (predictionA >> 11);
>  sign = APESIGN(p->lastA[filter]);
>  p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer, int order, int shift, int len
>  dotprod = 0;
>  sign = APESIGN(buffer[i]);
>  for (j = 0; j < order; j++) {
> -            dotprod += delay[j] * coeffs[j];
> +            dotprod += delay[j] * (unsigned)coeffs[j];
>  coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>  }
>  buffer[i] -= dotprod >> shift;
>

This code is DSP, overflows should be allowed in case input is corrupt.
This isn't even the best way to solve this, you can just make the array types unsigned.

NAK to this patchset and all future patchsets fixing overflows in DSPs and timeouts until
you explain exactly how this is useful besides fixing spam from a rarely useful tool that
Google refuses to fix. Derek tried to ask them to ignore timeouts and they did nothing.
Reimar Döffinger June 16, 2019, 12:55 p.m.
On 16.06.2019, at 12:30, Lynne <dev@lynne.ee> wrote:

> Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:
> 
>> Fixes: left shift of negative value -4
>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in type 'int'
>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be represented in type 'int'
>> Fixes: 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>> 
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>> libavcodec/apedec.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>> index 61ebfdafd5..f1bfc634c2 100644
>> --- a/libavcodec/apedec.c
>> +++ b/libavcodec/apedec.c
>> @@ -859,22 +859,22 @@ static av_always_inline int filter_3800(APEPredictor *p,
>> return predictionA;
>> }
>> d2 =  p->buf[delayA];
>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) << 3);
>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) * 8);
>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>> d4 =  p->buf[delayB];
>> 
>> -    predictionA = d0 * p->coeffsA[filter][0] +
>> -                  d1 * p->coeffsA[filter][1] +
>> -                  d2 * p->coeffsA[filter][2];
>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
>> +                  d2 * (unsigned)p->coeffsA[filter][2];
>> 
>> sign = APESIGN(decoded);
>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>> 
>> -    predictionB = d3 * p->coeffsB[filter][0] -
>> -                  d4 * p->coeffsB[filter][1];
>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>> +                  d4 * (unsigned)p->coeffsB[filter][1];
>> p->lastA[filter] = decoded + (predictionA >> 11);
>> sign = APESIGN(p->lastA[filter]);
>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer, int order, int shift, int len
>> dotprod = 0;
>> sign = APESIGN(buffer[i]);
>> for (j = 0; j < order; j++) {
>> -            dotprod += delay[j] * coeffs[j];
>> +            dotprod += delay[j] * (unsigned)coeffs[j];
>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>> }
>> buffer[i] -= dotprod >> shift;
>> 
> 
> This code is DSP, overflows should be allowed in case input is corrupt.

Then get the C standard changed or use a different language.
But in C overflows on signed types absolutely are not Ok.
Paul B Mahol June 16, 2019, 3:12 p.m.
On 6/16/19, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>
>
> On 16.06.2019, at 12:30, Lynne <dev@lynne.ee> wrote:
>
>> Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:
>>
>>> Fixes: left shift of negative value -4
>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
>>> type 'int'
>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
>>> represented in type 'int'
>>> Fixes:
>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>>>
>>> Found-by: continuous fuzzing process
>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>> libavcodec/apedec.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>> index 61ebfdafd5..f1bfc634c2 100644
>>> --- a/libavcodec/apedec.c
>>> +++ b/libavcodec/apedec.c
>>> @@ -859,22 +859,22 @@ static av_always_inline int
>>> filter_3800(APEPredictor *p,
>>> return predictionA;
>>> }
>>> d2 =  p->buf[delayA];
>>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
>>> 3);
>>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
>>> 8);
>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>>> d4 =  p->buf[delayB];
>>>
>>> -    predictionA = d0 * p->coeffsA[filter][0] +
>>> -                  d1 * p->coeffsA[filter][1] +
>>> -                  d2 * p->coeffsA[filter][2];
>>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
>>> +                  d2 * (unsigned)p->coeffsA[filter][2];
>>>
>>> sign = APESIGN(decoded);
>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>>>
>>> -    predictionB = d3 * p->coeffsB[filter][0] -
>>> -                  d4 * p->coeffsB[filter][1];
>>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>>> +                  d4 * (unsigned)p->coeffsB[filter][1];
>>> p->lastA[filter] = decoded + (predictionA >> 11);
>>> sign = APESIGN(p->lastA[filter]);
>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
>>> int order, int shift, int len
>>> dotprod = 0;
>>> sign = APESIGN(buffer[i]);
>>> for (j = 0; j < order; j++) {
>>> -            dotprod += delay[j] * coeffs[j];
>>> +            dotprod += delay[j] * (unsigned)coeffs[j];
>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>>> }
>>> buffer[i] -= dotprod >> shift;
>>>
>>
>> This code is DSP, overflows should be allowed in case input is corrupt.
>
> Then get the C standard changed or use a different language.
> But in C overflows on signed types absolutely are not Ok.

I disagree, overflows in DSP are normal.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lynne June 16, 2019, 6:31 p.m.
Jun 16, 2019, 4:12 PM by onemda@gmail.com:

> On 6/16/19, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>
>>
>>
>> On 16.06.2019, at 12:30, Lynne <dev@lynne.ee> wrote:
>>
>>> Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:
>>>
>>>> Fixes: left shift of negative value -4
>>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
>>>> type 'int'
>>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
>>>> represented in type 'int'
>>>> Fixes:
>>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>>>>
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>> libavcodec/apedec.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>>> index 61ebfdafd5..f1bfc634c2 100644
>>>> --- a/libavcodec/apedec.c
>>>> +++ b/libavcodec/apedec.c
>>>> @@ -859,22 +859,22 @@ static av_always_inline int
>>>> filter_3800(APEPredictor *p,
>>>> return predictionA;
>>>> }
>>>> d2 =  p->buf[delayA];
>>>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>>>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
>>>> 3);
>>>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>>>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
>>>> 8);
>>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>>>> d4 =  p->buf[delayB];
>>>>
>>>> -    predictionA = d0 * p->coeffsA[filter][0] +
>>>> -                  d1 * p->coeffsA[filter][1] +
>>>> -                  d2 * p->coeffsA[filter][2];
>>>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>>>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
>>>> +                  d2 * (unsigned)p->coeffsA[filter][2];
>>>>
>>>> sign = APESIGN(decoded);
>>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>>>>
>>>> -    predictionB = d3 * p->coeffsB[filter][0] -
>>>> -                  d4 * p->coeffsB[filter][1];
>>>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>>>> +                  d4 * (unsigned)p->coeffsB[filter][1];
>>>> p->lastA[filter] = decoded + (predictionA >> 11);
>>>> sign = APESIGN(p->lastA[filter]);
>>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
>>>> int order, int shift, int len
>>>> dotprod = 0;
>>>> sign = APESIGN(buffer[i]);
>>>> for (j = 0; j < order; j++) {
>>>> -            dotprod += delay[j] * coeffs[j];
>>>> +            dotprod += delay[j] * (unsigned)coeffs[j];
>>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>>>> }
>>>> buffer[i] -= dotprod >> shift;
>>>>
>>>
>>> This code is DSP, overflows should be allowed in case input is corrupt.
>>>
>>
>> Then get the C standard changed or use a different language.
>> But in C overflows on signed types absolutely are not Ok.
>>
>
> I disagree, overflows in DSP are normal.
>

In some codecs like VP9 they're normative IIRC.
The C standard says they're undefined operations, and what the implementation
decides to do is something we don't care about as the input is invalid anyway and we
can't salvage it.
Michael Niedermayer June 16, 2019, 8:35 p.m.
On Sun, Jun 16, 2019 at 08:31:37PM +0200, Lynne wrote:
> Jun 16, 2019, 4:12 PM by onemda@gmail.com:
> 
> > On 6/16/19, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> >
> >>
> >>
> >> On 16.06.2019, at 12:30, Lynne <dev@lynne.ee> wrote:
> >>
> >>> Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:
> >>>
> >>>> Fixes: left shift of negative value -4
> >>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
> >>>> type 'int'
> >>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
> >>>> represented in type 'int'
> >>>> Fixes:
> >>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
> >>>>
> >>>> Found-by: continuous fuzzing process
> >>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>> libavcodec/apedec.c | 16 ++++++++--------
> >>>> 1 file changed, 8 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> >>>> index 61ebfdafd5..f1bfc634c2 100644
> >>>> --- a/libavcodec/apedec.c
> >>>> +++ b/libavcodec/apedec.c
> >>>> @@ -859,22 +859,22 @@ static av_always_inline int
> >>>> filter_3800(APEPredictor *p,
> >>>> return predictionA;
> >>>> }
> >>>> d2 =  p->buf[delayA];
> >>>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
> >>>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
> >>>> 3);
> >>>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
> >>>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
> >>>> 8);
> >>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
> >>>> d4 =  p->buf[delayB];
> >>>>
> >>>> -    predictionA = d0 * p->coeffsA[filter][0] +
> >>>> -                  d1 * p->coeffsA[filter][1] +
> >>>> -                  d2 * p->coeffsA[filter][2];
> >>>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
> >>>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
> >>>> +                  d2 * (unsigned)p->coeffsA[filter][2];
> >>>>
> >>>> sign = APESIGN(decoded);
> >>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
> >>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
> >>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
> >>>>
> >>>> -    predictionB = d3 * p->coeffsB[filter][0] -
> >>>> -                  d4 * p->coeffsB[filter][1];
> >>>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
> >>>> +                  d4 * (unsigned)p->coeffsB[filter][1];
> >>>> p->lastA[filter] = decoded + (predictionA >> 11);
> >>>> sign = APESIGN(p->lastA[filter]);
> >>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
> >>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
> >>>> int order, int shift, int len
> >>>> dotprod = 0;
> >>>> sign = APESIGN(buffer[i]);
> >>>> for (j = 0; j < order; j++) {
> >>>> -            dotprod += delay[j] * coeffs[j];
> >>>> +            dotprod += delay[j] * (unsigned)coeffs[j];
> >>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
> >>>> }
> >>>> buffer[i] -= dotprod >> shift;
> >>>>
> >>>
> >>> This code is DSP, overflows should be allowed in case input is corrupt.
> >>>
> >>
> >> Then get the C standard changed or use a different language.
> >> But in C overflows on signed types absolutely are not Ok.
> >>
> >
> > I disagree, overflows in DSP are normal.
> >
> 
> In some codecs like VP9 they're normative IIRC.
> The C standard says they're undefined operations, and what the implementation
> decides to do is something we don't care about as the input is invalid anyway and we
> can't salvage it.

ISO/IEC 9899:2017
  3.4.3
1 undefined behavior
  behavior, upon use of a nonportable or erroneous program construct or of erroneous data, for which
  this International Standard imposes no requirements
2 Note 1 to entry: Possible undefined behavior ranges from ignoring the situation completely with unpredictable results,
  to behaving during translation or program execution in a documented manner characteristic of the environment (with or
  without the issuance of a diagnostic message), to terminating a translation or execution (with the issuance of a diagnostic
  message).
3 EXAMPLE An example of undefined behavior is the behavior on integer overflow.

[...]
Reimar Döffinger June 16, 2019, 10:20 p.m.
On 16.06.2019, at 17:12, Paul B Mahol <onemda@gmail.com> wrote:

> On 6/16/19, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>> 
>> 
>> On 16.06.2019, at 12:30, Lynne <dev@lynne.ee> wrote:
>> 
>>> Jun 16, 2019, 10:57 AM by michael@niedermayer.cc:
>>> 
>>>> Fixes: left shift of negative value -4
>>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
>>>> type 'int'
>>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
>>>> represented in type 'int'
>>>> Fixes:
>>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>>>> 
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>> libavcodec/apedec.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>>> index 61ebfdafd5..f1bfc634c2 100644
>>>> --- a/libavcodec/apedec.c
>>>> +++ b/libavcodec/apedec.c
>>>> @@ -859,22 +859,22 @@ static av_always_inline int
>>>> filter_3800(APEPredictor *p,
>>>> return predictionA;
>>>> }
>>>> d2 =  p->buf[delayA];
>>>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>>>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
>>>> 3);
>>>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>>>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
>>>> 8);
>>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>>>> d4 =  p->buf[delayB];
>>>> 
>>>> -    predictionA = d0 * p->coeffsA[filter][0] +
>>>> -                  d1 * p->coeffsA[filter][1] +
>>>> -                  d2 * p->coeffsA[filter][2];
>>>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>>>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
>>>> +                  d2 * (unsigned)p->coeffsA[filter][2];
>>>> 
>>>> sign = APESIGN(decoded);
>>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>>>> 
>>>> -    predictionB = d3 * p->coeffsB[filter][0] -
>>>> -                  d4 * p->coeffsB[filter][1];
>>>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>>>> +                  d4 * (unsigned)p->coeffsB[filter][1];
>>>> p->lastA[filter] = decoded + (predictionA >> 11);
>>>> sign = APESIGN(p->lastA[filter]);
>>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
>>>> int order, int shift, int len
>>>> dotprod = 0;
>>>> sign = APESIGN(buffer[i]);
>>>> for (j = 0; j < order; j++) {
>>>> -            dotprod += delay[j] * coeffs[j];
>>>> +            dotprod += delay[j] * (unsigned)coeffs[j];
>>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>>>> }
>>>> buffer[i] -= dotprod >> shift;
>>>> 
>>> 
>>> This code is DSP, overflows should be allowed in case input is corrupt.
>> 
>> Then get the C standard changed or use a different language.
>> But in C overflows on signed types absolutely are not Ok.
> 
> I disagree, overflows in DSP are normal.

It's simply incorrect code.
Yes, you will probably get lucky for a long time.
But there are ISAs that trap on overflow, and gcc can trap on overflow.
That already makes these cases a denial-of-service issue.
However it gets far worse than that.
All it takes is for code to do something like
if (x < 2048) some_array[x] = y;
(lots of code followed by DSP code doing)
x * (1 << 24)

and now the compiler can and actually might really remove the if condition and you have an exploitable buffer overflow.
People have been bitten by such issues often enough (because due to LTO compiler optimizations of this kind have potentially unlimited scope, whereas reviewer have almost not chance of catching something this subtle) that such assumptions violating the C standard should be avoided.
(besides the more immediate point that fuzzing does catch quite real issues, and leaving such well-maybe-it's-ok cases in prevents finding real issues - on its own it might be an argument to change to tools, but due to the above it is reasonable to consider the tools doing the right thing).

Patch hide | download patch | download mbox

diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
index 61ebfdafd5..f1bfc634c2 100644
--- a/libavcodec/apedec.c
+++ b/libavcodec/apedec.c
@@ -859,22 +859,22 @@  static av_always_inline int filter_3800(APEPredictor *p,
         return predictionA;
     }
     d2 =  p->buf[delayA];
-    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
-    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) << 3);
+    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
+    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) * 8);
     d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
     d4 =  p->buf[delayB];
 
-    predictionA = d0 * p->coeffsA[filter][0] +
-                  d1 * p->coeffsA[filter][1] +
-                  d2 * p->coeffsA[filter][2];
+    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
+                  d1 * (unsigned)p->coeffsA[filter][1] +
+                  d2 * (unsigned)p->coeffsA[filter][2];
 
     sign = APESIGN(decoded);
     p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
     p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
     p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
 
-    predictionB = d3 * p->coeffsB[filter][0] -
-                  d4 * p->coeffsB[filter][1];
+    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
+                  d4 * (unsigned)p->coeffsB[filter][1];
     p->lastA[filter] = decoded + (predictionA >> 11);
     sign = APESIGN(p->lastA[filter]);
     p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
@@ -902,7 +902,7 @@  static void long_filter_high_3800(int32_t *buffer, int order, int shift, int len
         dotprod = 0;
         sign = APESIGN(buffer[i]);
         for (j = 0; j < order; j++) {
-            dotprod += delay[j] * coeffs[j];
+            dotprod += delay[j] * (unsigned)coeffs[j];
             coeffs[j] += ((delay[j] >> 31) | 1) * sign;
         }
         buffer[i] -= dotprod >> shift;