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

[Michael Niedermayer]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20170708/4bcdad5c/attachment.sig>