[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 04:18:48 EEST 2017


Hi,

On Sun, Mar 26, 2017 at 6:41 PM, 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
> 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


So apparently it's OK that my system is taken over by a carefully crafted
sample posted on bugzilla?

Ronald


More information about the ffmpeg-devel mailing list