| Message ID | 20190616095701.31384-4-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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.
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.
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".
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.
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. [...]
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).
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;
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(-)