Hi, On Sun, Mar 26, 2017 at 6:41 PM, Michael Niedermayer <mich...@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 > <mich...@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 <mich...@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 <mich...@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 <mich...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
