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

Paul B Mahol onemda at gmail.com
Sun Jun 16 18:12:45 EEST 2019


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.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list