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

[Michael Niedermayer]

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 semantically wrong.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- 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/20170609/1dc7f64d/attachment.sig>