[FFmpeg-devel] [PATCH] avcodec/fft_template: Fix multiple runtime error: signed integer overflow: -1943918714 - 1935113003 cannot be represented in type 'int'

[wm4]

On Sat, 10 Jun 2017 02:50:11 +0300
Ivan Kalvachev <ikalvachev at gmail.com> wrote:

> On 6/9/17, wm4 <nfxjfg at googlemail.com> wrote:
> > On Fri, 9 Jun 2017 00:10:49 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >  
> >> On Sat, May 27, 2017 at 12:24:16PM +0200, wm4 wrote:  
> >> > On Sat, 27 May 2017 03:56:42 +0200
> >> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >> >  
> >> > > On Fri, May 26, 2017 at 07:06:44PM -0400, Ronald S. Bultje wrote:  
> >> > > > Hi,
> >> > > >
> >> > > > On Fri, May 26, 2017 at 6:34 PM, Michael Niedermayer
> >> > > > <michael at niedermayer.cc  
> >> > > > > wrote:  
> >> > > >  
> >> > > > > On Fri, May 26, 2017 at 06:07:35PM -0400, Ronald S. Bultje wrote:
> >> > > > >  
> >> > > > > > Hi,
> >> > > > > >
> >> > > > > > On Fri, May 26, 2017 at 5:50 PM, Michael Niedermayer  
> >> > > > > <michael at niedermayer.cc  
> >> > > > > > > wrote:  
> >> > > > > >  
> >> > > > > > > On Fri, May 26, 2017 at 11:18:12PM +0200, Hendrik Leppkes
> >> > > > > > > wrote:  
> >> > > > > > > > On Fri, May 26, 2017 at 11:11 PM, Michael Niedermayer
> >> > > > > > > > <michael at niedermayer.cc> wrote:  
> >> > > > > > > > > On Fri, May 26, 2017 at 03:20:14PM +0100, Rostislav
> >> > > > > > > > > Pehlivanov  
> >> > > > > wrote:  
> >> > > > > > > > >> On 26 May 2017 at 12:21, wm4 <nfxjfg at googlemail.com>
> >> > > > > > > > >> wrote:
> >> > > > > > > > >>  
> >> > > > > > > > >> > On Thu, 25 May 2017 16:10:49 +0200
> >> > > > > > > > >> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >> > > > > > > > >> >  
> >> > > > > > > > >> > > Fixes:
> >> > > > > > > > >> > > 1735/clusterfuzz-testcase-minimized-5350472347025408
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > Found-by: continuous fuzzing process  
> >> > > > > > > https://github.com/google/oss-  
> >> > > > > > > > >> > fuzz/tree/master/projects/ffmpeg  
> >> > > > > > > > >> > > Signed-off-by: Michael Niedermayer
> >> > > > > > > > >> > > <michael at niedermayer.cc>
> >> > > > > > > > >> > > ---
> >> > > > > > > > >> > >  libavcodec/fft_template.c | 50
> >> > > > > > > > >> > > +++++++++++++++++++++++-------  
> >> > > > > > > > >> > -----------------  
> >> > > > > > > > >> > >  1 file changed, 25 insertions(+), 25 deletions(-)
> >> > > > > > > > >> > >
> >> > > > > > > > >> > > diff --git a/libavcodec/fft_template.c  
> >> > > > > b/libavcodec/fft_template.c  
> >> > > > > > > > >> > > index 480557f49f..e3a37e5d69 100644
> >> > > > > > > > >> > > --- a/libavcodec/fft_template.c
> >> > > > > > > > >> > > +++ b/libavcodec/fft_template.c
> >> > > > > > > > >> > > @@ -249,7 +249,7 @@ static void fft_calc_c(FFTContext
> >> > > > > > > > >> > > *s,  
> >> > > > > > > FFTComplex *z)  
> >> > > > > > > > >> > {  
> >> > > > > > > > >> > >
> >> > > > > > > > >> > >      int nbits, i, n, num_transforms, offset, step;
> >> > > > > > > > >> > >      int n4, n2, n34;
> >> > > > > > > > >> > > -    FFTSample tmp1, tmp2, tmp3, tmp4, tmp5, tmp6,
> >> > > > > > > > >> > > tmp7, tmp8;
> >> > > > > > > > >> > > +    SUINT tmp1, tmp2, tmp3, tmp4, tmp5, tmp6, tmp7,
> >> > > > > > > > >> > > tmp8;  
> >> > > > > > > > >> >
> >> > > > > > > > >> > I want this SUINT thing gone, not have more of it.
> >> > > > > > > > >> > _______________________________________________
> >> > > > > > > > >> > ffmpeg-devel mailing list
> >> > > > > > > > >> > ffmpeg-devel at ffmpeg.org
> >> > > > > > > > >> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> > > > > > > > >> >  
> >> > > > > > > > >>
> >> > > > > > > > >> I agree, especially here.  
> >> > > > > > > > >  
> >> > > > > > > > >> Overflows should be left to happen in transforms if the
> >> > > > > > > > >> input is  
> >> > > > > > > corrupt.  
> >> > > > > > > > >
> >> > > > > > > > > signed int overflow is not allowed in C, if that is what
> >> > > > > > > > > you meant.
> >> > > > > > > > >
> >> > > > > > > > >  
> >> > > > > > > >
> >> > > > > > > > Its "undefined", which means what the result will be is not
> >> > > > > > > > defined -
> >> > > > > > > > which in such a DSP function is irrelevant, if all it causes
> >> > > > > > > > is
> >> > > > > > > > corrupt output ... from corrupt input.  
> >> > > > > > >
> >> > > > > > > no, this is not correct.
> >> > > > > > > undefined behavior does not mean the effect will be limited to
> >> > > > > > > the output.
> >> > > > > > > It could cause the process to hard lockup, trigger an
> >> > > > > > > exception or
> >> > > > > > > set a flag causing errors in later unrelated code.  
> >> > > > > >
> >> > > > > >  
> >> > > > >  
> >> > > > > > We've discussed this before, if you believe this to be
> >> > > > > > exploitable, why
> >> > > > > > allow it in our repository at all? I know of no other project
> >> > > > > > that even
> >> > > > > > remotely allows such ridiculous things. Please limit exploitable
> >> > > > > > code to
> >> > > > > > your personal tree, ffmpeg is not a rootkit.  
> >> > > > >
> >> > > > > please calm down, you make all kinds of statments and implications
> >> > > > > in
> >> > > > > the text above which are not true.
> >> > > > >
> >> > > > > This specific code in git triggers undefined behavior, the patch
> >> > > > > fixes
> >> > > > > this undefined behavior.
> >> > > > > If that is exploitable (which i neither claim it is nor that it
> >> > > > > isnt)
> >> > > > > its a issue that exists before the patch but not afterwards.  
> >> > > >
> >> > > >  
> >> > >  
> >> > > > *unsigned* removes the exploit. SUINT keeps it, and is therefore
> >> > > > part of a
> >> > > > rootkit.  
> >> > >
> >> > > SUINT is defined to unsigned, if unsigned removes the issue
> >> > > so does SUINT.  
> >> >
> >> > Then why is it called SUINT and not UINT.  
> >>
> >> Signed value in
> >> Unsigned
> >> INTeger type
> >>
> >> This concept is needed for fixing undefined operations efficiently.
> >> The type can always be replaced by unsigned and thats what is being
> >> done now.
> >> But it makes the code hard to understand and maintain because these
> >> values are not positive integers but signed integers. Which for
> >> C standard compliance need to be stored in a unsigned type.
> >>
> >> Both SUINT and unsigned should produce identical binaries but unsigned  
> >
> > Is that so? Then please add a FATE check that guarantees this.
> >  
> >> is semantically wrong.  
> >
> > If it's wrong then it shouldn't be used.  
> 
> I think you should read (again) these articles:
> http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

I know what undefined behavior is.