[FFmpeg-devel] [PATCH] avcodec/mjpegdec: Fixes runtime error: signed integer overflow: -24543 * 2031616 cannot be represented in type 'int'

[Michael Niedermayer]

On Wed, Mar 29, 2017 at 11:47:31AM +0200, wm4 wrote:
> On Mon, 27 Mar 2017 00:41:05 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Sun, Mar 26, 2017 at 05:30:05PM -0400, Ronald S. Bultje wrote:
> > > Hi,
> > > 
> > > On Sun, Mar 26, 2017 at 3:31 PM, Michael Niedermayer <michael at niedermayer.cc  
> > > > wrote:  
> > >   
> > > > On Sun, Mar 26, 2017 at 07:41:58PM +0200, wm4 wrote:  
> > > > > On Sun, 26 Mar 2017 19:16:26 +0200
> > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > >  
> > > > > > On Sun, Mar 26, 2017 at 06:51:11PM +0200, wm4 wrote:  
> > > > > > > On Sun, 26 Mar 2017 18:11:01 +0200
> > > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > > >  
> > > > > > > > Fixes: 943/clusterfuzz-testcase-5114865297391616
> > > > > > > >
> > > > > > > > Found-by: continuous fuzzing process  
> > > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg  
> > > > > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > > > > ---
> > > > > > > >  libavcodec/mjpegdec.c | 3 ++-
> > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > > > > > > index f26e8a3f9a..e08b045fe7 100644
> > > > > > > > --- a/libavcodec/mjpegdec.c
> > > > > > > > +++ b/libavcodec/mjpegdec.c
> > > > > > > > @@ -757,7 +757,8 @@ static int decode_block_progressive(MJpegDecodeContext  
> > > > *s, int16_t *block,  
> > > > > > > >                                      uint16_t *quant_matrix,
> > > > > > > >                                      int ss, int se, int Al, int  
> > > > *EOBRUN)  
> > > > > > > >  {
> > > > > > > > -    int code, i, j, level, val, run;
> > > > > > > > +    int code, i, j, val, run;
> > > > > > > > +    SUINT level;
> > > > > > > >
> > > > > > > >      if (*EOBRUN) {
> > > > > > > >          (*EOBRUN)--;  
> > > > > > >
> > > > > > > Please make the type either signed or unsigned. Making it both
> > > > > > > (depending on the debug level) just to make the fuzzer happy (or
> > > > > > > something more complicated than that?) isn't a good idea. You  
> > > > probably  
> > > > > > > want to make it always unsigned?  
> > > > > >
> > > > > > No, i want to make it SUINT
> > > > > >
> > > > > > If it is always unsigned then its not possible to detect overflows
> > > > > > without explicitly checking for overflows.
> > > > > > If it is SUINT then ubsan can be used to detect overflows, this is
> > > > > > usefull to test files showing artifacts but no decode errors.
> > > > > >  
> > > > >
> > > > > The point of these tools (static analyzers, sanitizers, fuzzers) is to
> > > > > improve the correctness of the code.  
> > > >  
> > > > > SUINT is still defined to "int" if
> > > > > CHECKED is not defined  
> > > >
> > > > no
> > > >
> > > > internal.h:
> > > > #ifdef CHECKED
> > > > #define SUINT   int
> > > > #define SUINT32 int32_t
> > > > #else
> > > > #define SUINT   unsigned
> > > > #define SUINT32 uint32_t
> > > > #endif
> > > >
> > > > I belive the rest of your mail assumes this condition is backward to
> > > > how it is
> > > >
> > > > SUINT is not there to make any tools happy its there to allow finding
> > > > overflows in debug more while having valid c code in normal builds  
> > > 
> > >   
> > 
> > > Why don't we want to detect overflows in debug mode?  
> > 
> > like wm4 you read "#if A" as "#if not A", all your mail and questions
> > seem based on reading the condition for SUINT flipped around
> > 
> > in DEBUG mode CHECKED is enabled, SUINT is int and overflows are
> > undefined behaviour which can be detected easily with ubsan.
> > 
> > This allows us to debug samples producing artifacts but no decode
> > errors due to overflows.
> > 
> > 
> > in normal mode CHECKED is disabled, SUINT is unsigned and overflows
> > are defined behavior. There must be no undefined behavior in releases
> > 
> > maybe you belive everyone is using debug mode and the fuzzers run in
> > debug mode. Maybe this is why everyone belives the condition is
> > backward
> 
> How would we know this? Maybe I've been assuming too much in order to
> try to make sense out of it.
> 
> > I might be wrong but unless you manually pass -DDEBUG you dont use
> > debug mode, adding -DDEBUG is how our debug mode fate client tests that
> > mode
> 

> But why do you want to detect overflows in debug mode, if in release

To debug.
Like i want compiler warnings to "debug"
or coverity warnings to find and correct bugs. == "debug"

If a undamaged file triggers an overflow thats very likely a bug and
a easy to fix one if one knows about the overflow.
If one doesnt know of that overflow it can be very hard to find


> mode they're using unsigned arithmetic, which has defined overflows?
>
> Either the code is correct with unsigned - then it can use unsigned in
> both release and debug mode. Or it isn't - then there's another problem
> that is painted over (?) in release mode.
>
> Either way, it makes no sense to me.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri
-------------- 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/20170407/61d893a2/attachment.sig>