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

[Michael Niedermayer]

On Sun, Jun 16, 2019 at 08:31:37PM +0200, Lynne wrote:
> 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.

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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190616/125148b7/attachment.sig>