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

[Ronald S. Bultje]

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?

Ronald