[FFmpeg-devel] [PATCH 3/3] avcodec/aacpsdsp_template: Fixes integer overflow in ps_add_squares_c()

[Ivan Kalvachev]

On 7/9/17, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
>
>> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote:
>> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote:
>> > > On 2 July 2017 at 03:28, Michael Niedermayer <michael at niedermayer.cc>
>> wrote:
>> > >
>> > > > Fixes: runtime error: signed integer overflow: 1965219850 +
>> > > > 995792909
>> > > > cannot be represented in type 'int'
>> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920
>> > > >
>> > > > Found-by: continuous fuzzing process https://github.com/google/oss-
>> > > > fuzz/tree/master/projects/ffmpeg
>> > > > Signed-off-by
>> > > > <https://github.com/google/oss-fuzz/tree/master/projects/
>> ffmpeg%0ASigned-off-by>:
>> > > > Michael Niedermayer <michael at niedermayer.cc>
>> > > > ---
>> > > >  libavcodec/aacpsdsp_template.c | 3 ++-
>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_
>> > > > template.c
>> > > > index 9e1a95c1a1..2c0afd4512 100644
>> > > > --- a/libavcodec/aacpsdsp_template.c
>> > > > +++ b/libavcodec/aacpsdsp_template.c
>> > > > @@ -26,9 +26,10 @@
>> > > >  #include "libavutil/attributes.h"
>> > > >  #include "aacpsdsp.h"
>> > > >
>> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT
>> (*src)[2], int
>> > > > n)
>> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT
>> > > > (*src)[2], int n)
>> > > >  {
>> > > >      int i;
>> > > > +    SUINTFLOAT *dst = dst_param;
>> > > >      for (i = 0; i < n; i++)
>> > > >          dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1],
>> src[i][1]);
>> > > >  }
>> > > >
>> > > >
>> > > What's the issue with just _not_ fixing it here? It only occurs on
>> fuzzed
>> > > inputs, doesn't crash on any known platform ever, makes the code
>> uglier and
>> > > why? Because some fuzzer is super pedantic.
>> > > Why not fix the fuzzer? Why not just mark this as a false positive,
>> since
>> > > fixing it is pointless from the standpoint of security (you can't
>> exploit
>> > > overflows in transforms or functions like this), and all developers
>> hate it.
>> >
>> > Iam not sure you understand the problem.
>> > signed integer overflows are undefined behavior, undefined behavior
>> > is not allowed in C.
>> >
>> >
>> > Theres also no option to mark anything as false positive.
>> > If the tool makes a mistake, the issue needs to be reported to its
>> > authors and needs to be fixed.
>> > I belive the tool is correct, if someone thinks its not correct, the
>> > place to report this to is likely where the clang sanitizers are
>> > developed.
>> >
>> > I do understand that this is annoying but this is how C works.
>> >
>> > About "doesn't crash on any known platform ever",
>> > "you can't exploit  overflows in transforms or functions like this"
>> >
>> > undefined behavior has bitten people with unexpected results in the
>> > past. for example "if (a+b < a)" to test for overflow, but because the
>> condition
>> > can only be true in case of overflow and overflow isnt allowed in C
>> > the compiler didnt assemble this the way the human thought.
>> >
>> > In the case of ps_add_squares_c(), dst[i] has to increase if iam
>> > not mistaken. In case of overflow it can decrease but overflow is
>> > not allowed so a compiler can prune anything that implies dst[] to be
>> > negative.
>> > dst[] is used downstream in checks of greater / less and in FFMAX
>> > In this code the compiler can assume that the sign bit is clear,
>> > what happens when  the compilers optimizer has false assumtations
>> > i dont know but i know its not good.
>> >
>> > That said even if no such chain of conditions exist its still invalid
>> > code if theres undefined behavior in it
>>
>> Does anyone object to this patch ?
>> Or does anyone have a better idea on how to fix this ?
>> if not id like to apply it
>
>
> I think Rostislav's point is: why fix it, if it can only happen with
> corrupt input? The before and after situation is identical: garbage in,
> garbage out. If the compiler does funny things that makes the garbage
> slightly differently bad, is that really so devilishly bad? It's still
> garbage. Is anything improved by this?

As I have tried to explain before,
you can harden a program by compiling it
with `gcc -ftrapv` .

In such a program the overflow would trap
and in normal case would lead to termination.

Do you want your browser to die by a small bit error in a random clip?