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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170703/e28cc034/attachment.sig>