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

Ronald S. Bultje rsbultje at gmail.com
Mon Mar 27 00:30:05 EEST 2017


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?

I somewhat agree with wm4's concern about SUINT.

The answer is probably "we don't want to clip coefficients read from
bitstreams" (e.g. for performance reasons, or because the bitstream
provides no limits for such values, especially not pre-dequant), which
means fuzzed files can have any value coefficient and signed arithmetic can
thus cause undefined behaviour.

If that is true, then both of the following are true:
- (in release builds) signed arithmetic overflows and the resulting
undefined behaviour for artifically crafted input are not a security
concern.
- (in debug builds) signed arithmetic overflows and the resulting undefined
behaviour for artificially crafted input are a significant security
concern, so much that we have added specific code to deal with them.

However, these two are in direct violation with each other. As such, if one
is true, the other one is by definition false. Which one is right?

Ronald


More information about the ffmpeg-devel mailing list