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

[wm4]

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 (which in turn is only defined if DEBUG is
defined, or if it was specified in CFLAGS, apparently). So this doesn't
fix/improve the code. It only makes the analyzer happy. There is no
point in this. Either the "normal" code is still buggy, then it should
be fixed, or it's not - then the code doesn't need to be changed.

Having subtly different correctness between code compiled in debug and
release mode works against what you want to achieve with these tools.
The only purpose of these changes seems to be to make the tools happy,
while actually running different code for normal builds, which might
lead to errors in that code being hidden.