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

[Lynne]

Jun 16, 2019, 4:12 PM by onemda at gmail.com:

> On 6/16/19, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>
>>
>>
>> On 16.06.2019, at 12:30, Lynne <dev at lynne.ee> wrote:
>>
>>> Jun 16, 2019, 10:57 AM by michael at 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 at 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.