On Fri, 7 Apr 2017 02:17:37 +0200 Michael Niedermayer <mich...@niedermayer.cc> wrote:
> On Wed, Mar 29, 2017 at 11:47:31AM +0200, wm4 wrote: > > On Mon, 27 Mar 2017 00:41:05 +0200 > > 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 > > > > 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 > > To debug. > Like i want compiler warnings to "debug" > or coverity warnings to find and correct bugs. == "debug" > > If a undamaged file triggers an overflow thats very likely a bug and > a easy to fix one if one knows about the overflow. > If one doesnt know of that overflow it can be very hard to find Then it's not a bug by definition, and you can ignore it. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
