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

[wm4]

On Fri, 7 Apr 2017 02:17:37 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> 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

Then it's not a bug by definition, and you can ignore it.