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

wm4 nfxjfg at googlemail.com
Wed Mar 29 12:47:31 EEST 2017


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


More information about the ffmpeg-devel mailing list