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

Submitted by Michael Niedermayer on March 26, 2017, 4:11 p.m.

Details

Message ID 20170326161101.3542-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer March 26, 2017, 4:11 p.m.
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@niedermayer.cc>
---
 libavcodec/mjpegdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

wm4 March 26, 2017, 4:51 p.m.
On Sun, 26 Mar 2017 18:11:01 +0200
Michael Niedermayer <michael@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@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?
Michael Niedermayer March 26, 2017, 5:16 p.m.
On Sun, Mar 26, 2017 at 06:51:11PM +0200, wm4 wrote:
> On Sun, 26 Mar 2017 18:11:01 +0200
> Michael Niedermayer <michael@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@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.


[...]
wm4 March 26, 2017, 5:41 p.m.
On Sun, 26 Mar 2017 19:16:26 +0200
Michael Niedermayer <michael@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@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@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.
Michael Niedermayer March 26, 2017, 7:31 p.m.
On Sun, Mar 26, 2017 at 07:41:58PM +0200, wm4 wrote:
> On Sun, 26 Mar 2017 19:16:26 +0200
> Michael Niedermayer <michael@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@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@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

[...]
Ronald S. Bultje March 26, 2017, 9:30 p.m.
Hi,

On Sun, Mar 26, 2017 at 3:31 PM, Michael Niedermayer <michael@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@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@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@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
Michael Niedermayer March 26, 2017, 10:41 p.m.
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@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@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@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@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


[...]
Ronald S. Bultje March 27, 2017, 1:18 a.m.
Hi,

On Sun, Mar 26, 2017 at 6:41 PM, Michael Niedermayer <michael@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@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@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@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@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
Carl Eugen Hoyos March 27, 2017, 7:33 a.m.
2017-03-27 3:18 GMT+02:00 Ronald S. Bultje <rsbultje@gmail.com>:
> So apparently it's OK that my system is taken over by a carefully
> crafted sample posted on bugzilla?

Please stop this!

I don't know if and on what kind of mission you are (and it won't
make a difference to me) but these kind of comments are simply
not welcome.

Carl Eugen
wm4 March 29, 2017, 9:47 a.m.
On Mon, 27 Mar 2017 00:41:05 +0200
Michael Niedermayer <michael@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@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@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@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@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
mode they're using unsigned arithmetic, which has defined overflows?

Either the code is correct with unsigned - then it can use unsigned in
both release and debug mode. Or it isn't - then there's another problem
that is painted over (?) in release mode.

Either way, it makes no sense to me.
Michael Niedermayer April 7, 2017, 12:17 a.m.
On Wed, Mar 29, 2017 at 11:47:31AM +0200, wm4 wrote:
> On Mon, 27 Mar 2017 00:41:05 +0200
> Michael Niedermayer <michael@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@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@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@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@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


> mode they're using unsigned arithmetic, which has defined overflows?
>
> Either the code is correct with unsigned - then it can use unsigned in
> both release and debug mode. Or it isn't - then there's another problem
> that is painted over (?) in release mode.
>
> Either way, it makes no sense to me.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 April 7, 2017, 6:30 a.m.
On Fri, 7 Apr 2017 02:17:37 +0200
Michael Niedermayer <michael@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 <michael@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@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@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@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@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.
Michael Niedermayer April 7, 2017, 10:22 a.m.
On Fri, Apr 07, 2017 at 08:30:50AM +0200, wm4 wrote:
> On Fri, 7 Apr 2017 02:17:37 +0200
> Michael Niedermayer <michael@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 <michael@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@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@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@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@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.

overflows have been bugs in the past
warnings have pointed to bugs in the past
ubsan has pointed to bugs in the past

The example is about a overflow bug causing wrong decoder output
ubsan would find
that is in the most generic sense, the example would fit any such
bug and such bugs were found that way in the past.

if you say these are not bugs by definition even though its
quite obvious they are bugs then whatever you use as definition must
be flawed. Or you misunderstood what i wrote.

Again the key points are
* ubsan does and did help finding bugs which otherwise are hard to find
* using unsigned prevents ubsan from working
* adding checks and warnings would slow the code down and become messy
* using signed without checks is undefined behavior which is not ok for
  use of the code as its not valid C
* using SUINT avoids the undefined behavior, avoids peppering the code
  with checks and warnings and still allows us to use ubsan to detect
  overflows when we need to

Id like to apply the patch unless you or someone else objects.

thanks

[...]
Ronald S. Bultje April 7, 2017, 10:26 a.m.
Hi,

On Fri, Apr 7, 2017 at 6:22 AM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> if you say these are not bugs by definition even though its
> quite obvious they are bugs then whatever you use as definition must
> be flawed. Or you misunderstood what i wrote.
>

That's hypocritical.

Id like to apply the patch unless you or someone else objects.


I really don't like this approach. I'd like you to try to find something
more sensible that protects developer machines from bugs also.

Ronald
wm4 April 7, 2017, 10:35 a.m.
On Fri, 7 Apr 2017 12:22:07 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Fri, Apr 07, 2017 at 08:30:50AM +0200, wm4 wrote:
> > On Fri, 7 Apr 2017 02:17:37 +0200
> > Michael Niedermayer <michael@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 <michael@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@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@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@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@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.  
> 
> overflows have been bugs in the past
> warnings have pointed to bugs in the past
> ubsan has pointed to bugs in the past

What I'm saying is:

Either an overflow is a bug. Then you should not apply hacks like
making all types unsigned just to hide these bugs not show up on
sanitizers or static analyzers. And you shouldn't add another hack just
to make them show up again in debug mode. You could argue that this is
some kind of "hardening" technique (with the second hack still allowing
debugging it), but how about let's not? Just fix overflows when you
find them.

Or the overflow is not a bug. Then the type should always be unsigned,
because overflows would be relatively meaningless. Why make the source
code harder to follow and add noise in debug mode just because of a
vague idea that unknown overflows could occur and which could be bugs?
If overflows are a problem, make the type always signed, and always fix
overflows as you encounter them.

As it looks like, SUINT only hides bugs, and serves the questionable
idea that it's better to hide bugs for the sake of silencing undefined
behavior.

> 
> The example is about a overflow bug causing wrong decoder output
> ubsan would find
> that is in the most generic sense, the example would fit any such
> bug and such bugs were found that way in the past.
> 
> if you say these are not bugs by definition even though its
> quite obvious they are bugs then whatever you use as definition must
> be flawed. Or you misunderstood what i wrote.
> 
> Again the key points are
> * ubsan does and did help finding bugs which otherwise are hard to find
> * using unsigned prevents ubsan from working
> * adding checks and warnings would slow the code down and become messy
> * using signed without checks is undefined behavior which is not ok for
>   use of the code as its not valid C
> * using SUINT avoids the undefined behavior, avoids peppering the code
>   with checks and warnings and still allows us to use ubsan to detect
>   overflows when we need to
> 
> Id like to apply the patch unless you or someone else objects.
> 
> thanks
> 
> [...]
Michael Niedermayer April 7, 2017, 10:47 a.m.
On Fri, Apr 07, 2017 at 06:26:05AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Apr 7, 2017 at 6:22 AM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > if you say these are not bugs by definition even though its
> > quite obvious they are bugs then whatever you use as definition must
> > be flawed. Or you misunderstood what i wrote.
> >
> 
> That's hypocritical.

in what way ?


> 
> Id like to apply the patch unless you or someone else objects.
> 
> 
> I really don't like this approach. I'd like you to try to find something
> more sensible that protects developer machines from bugs also.

i suggested to decouple SUINT from the #ifdef DEBUG, which fixes
exactly that possibility (and noone seemed to care)



[...]
Michael Niedermayer April 7, 2017, 11:07 a.m.
On Fri, Apr 07, 2017 at 12:35:37PM +0200, wm4 wrote:
> On Fri, 7 Apr 2017 12:22:07 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Fri, Apr 07, 2017 at 08:30:50AM +0200, wm4 wrote:
> > > On Fri, 7 Apr 2017 02:17:37 +0200
> > > Michael Niedermayer <michael@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 <michael@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@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@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@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@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.  
> > 
> > overflows have been bugs in the past
> > warnings have pointed to bugs in the past
> > ubsan has pointed to bugs in the past
> 
> What I'm saying is:
> 
> Either an overflow is a bug. Then you should not apply hacks like
> making all types unsigned just to hide these bugs not show up on
> sanitizers or static analyzers. And you shouldn't add another hack just
> to make them show up again in debug mode. You could argue that this is
> some kind of "hardening" technique (with the second hack still allowing
> debugging it), but how about let's not? Just fix overflows when you
> find them.
> 
> Or the overflow is not a bug. Then the type should always be unsigned,
> because overflows would be relatively meaningless. Why make the source
> code harder to follow and add noise in debug mode just because of a
> vague idea that unknown overflows could occur and which could be bugs?
> If overflows are a problem, make the type always signed, and always fix
> overflows as you encounter them.

The problem is what we have is a useless fuzzed file triggering this
the fuzzed file shows that the overflow can be triggered it tells us
nothing about if its a real bug

can it be triggered also by a file we want to support?
if so its a real bug

in some cases this can be awnsered by reading some specification
still in reality what really matters if such files exist even if we
have a spec and even if it does say what the range is.
Some specs are non public, some lack detailed limits on intermediates
of computations.

We can spend a long time to determine if something is needed by a
real file and thus a bug or not.
This is what i generally do but often theres just no final clear yes
or no. Theres a probably yes / probably no / maybe
and for that SUINT comes in handy
we fix the undefined behavior but if a user submits a file that doesnt
decode correctly we can still flip a switch and use ubsan to see if
it involves any overflows

This is simply minimizing the time needed to debug such issues
* spend more and more time to make really really sure nothing valid
  can trigger an overflow, no unexpected odd files exist, ...
* vs just run files reported by users under ubsan

You could also say iam trying to use our userbases time to help
double check if what i think is likely not a bug really is not a bug

[...]
wm4 April 7, 2017, 11:30 a.m.
On Fri, 7 Apr 2017 13:07:23 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Fri, Apr 07, 2017 at 12:35:37PM +0200, wm4 wrote:
> > On Fri, 7 Apr 2017 12:22:07 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > On Fri, Apr 07, 2017 at 08:30:50AM +0200, wm4 wrote:  
> > > > On Fri, 7 Apr 2017 02:17:37 +0200
> > > > Michael Niedermayer <michael@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 <michael@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@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@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@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@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.    
> > > 
> > > overflows have been bugs in the past
> > > warnings have pointed to bugs in the past
> > > ubsan has pointed to bugs in the past  
> > 
> > What I'm saying is:
> > 
> > Either an overflow is a bug. Then you should not apply hacks like
> > making all types unsigned just to hide these bugs not show up on
> > sanitizers or static analyzers. And you shouldn't add another hack just
> > to make them show up again in debug mode. You could argue that this is
> > some kind of "hardening" technique (with the second hack still allowing
> > debugging it), but how about let's not? Just fix overflows when you
> > find them.
> > 
> > Or the overflow is not a bug. Then the type should always be unsigned,
> > because overflows would be relatively meaningless. Why make the source
> > code harder to follow and add noise in debug mode just because of a
> > vague idea that unknown overflows could occur and which could be bugs?
> > If overflows are a problem, make the type always signed, and always fix
> > overflows as you encounter them.  
> 
> The problem is what we have is a useless fuzzed file triggering this
> the fuzzed file shows that the overflow can be triggered it tells us
> nothing about if its a real bug

Look at the code and determine if it could be a bug or not?

Changing a type to unsigned is a nice way to get rid of undefined
behavior, but do it only if the result of an overflow does not matter.

> can it be triggered also by a file we want to support?
> if so its a real bug

And how does SUINT help you make the decision?

> in some cases this can be awnsered by reading some specification
> still in reality what really matters if such files exist even if we
> have a spec and even if it does say what the range is.
> Some specs are non public, some lack detailed limits on intermediates
> of computations.
> 
> We can spend a long time to determine if something is needed by a
> real file and thus a bug or not.
> This is what i generally do but often theres just no final clear yes
> or no. Theres a probably yes / probably no / maybe
> and for that SUINT comes in handy
> we fix the undefined behavior but if a user submits a file that doesnt
> decode correctly we can still flip a switch and use ubsan to see if
> it involves any overflows

Well there are 3 reasons why you'd change signed to unsigned:
1. unsigned overflow actually gives the intended result
2. avoiding undefined behavior when the result does not matter
3. shutting up a fuzzer without fixing the actual issue

None of these fix themselves when changing the type back to signed,
except for operations which needed to be signed in the first place. I'm
not sure how changing back to signed is supposed to help anyone with
actual overflows, since these would have been buggy in the first place.
Sure, it helps you better to spot a bug you could have spotted easier
with the original code when all was still signed.

But with 1. and 2. you'd have confidence in the change to unsigned. We
don't add arbitrary ifdefs and typedefs to other code either, just
because it'd help us to easily "revert" a change. And for 3. checking
the overflow explicitly would have been better.

So, you can fix the overflows by actually fixing the overflows, instead
of switching the type to unsigned and hoping it doesn't break anything.

And even if what you said about "flipping a switch" is true, you could
just edit the source code at the questionable location, which in fact
would be quicker than changing SUINT declared in a central location or
rebuilding the entire FFmpeg to change the debug level.

Maybe I'm indeed missing something here.

> 
> This is simply minimizing the time needed to debug such issues
> * spend more and more time to make really really sure nothing valid
>   can trigger an overflow, no unexpected odd files exist, ...
> * vs just run files reported by users under ubsan
> 
> You could also say iam trying to use our userbases time to help
> double check if what i think is likely not a bug really is not a bug

I don't see how any of this helps users. They don't run fuzzers or
debug tools, and they are not going to try to change the definition of
SUINT.

If something is _really_ questionable we still could print an error
message if a calculation overflows. That would _actually_ help finding
the issue. (But please, let's not do this with every overflow check.)
Ronald S. Bultje April 7, 2017, 11:46 a.m.
Hi,

On Fri, Apr 7, 2017 at 6:47 AM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Apr 07, 2017 at 06:26:05AM -0400, Ronald S. Bultje wrote:
> > On Fri, Apr 7, 2017 at 6:22 AM, Michael Niedermayer
> <michael@niedermayer.cc>
> > > Id like to apply the patch unless you or someone else objects.
> >
> > I really don't like this approach. I'd like you to try to find something
> > more sensible that protects developer machines from bugs also.
>
> i suggested to decouple SUINT from the #ifdef DEBUG, which fixes
> exactly that possibility (and noone seemed to care)


You asked - in this email - to apply this patch as-is. The patch does not
decouple #ifdef DEBUG from SUINT.

Now the question is whether SUINT has any raison d'etre in our main
codebase. If it's under #if 0 or otherwise "dead code", any Diego'ification
would immediately get rid of it. So answer me this question: if the code is
under #if 0, why shouldn't it just exist locally on your hard disk only?

(Yes, I have actual (old) debug branches for the vp9 decoder locally.)

Ronald
Michael Niedermayer April 7, 2017, 2:41 p.m.
On Fri, Apr 07, 2017 at 07:46:01AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Apr 7, 2017 at 6:47 AM, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Apr 07, 2017 at 06:26:05AM -0400, Ronald S. Bultje wrote:
> > > On Fri, Apr 7, 2017 at 6:22 AM, Michael Niedermayer
> > <michael@niedermayer.cc>
> > > > Id like to apply the patch unless you or someone else objects.
> > >
> > > I really don't like this approach. I'd like you to try to find something
> > > more sensible that protects developer machines from bugs also.
> >
> > i suggested to decouple SUINT from the #ifdef DEBUG, which fixes
> > exactly that possibility (and noone seemed to care)
> 
> 
> You asked - in this email - to apply this patch as-is. The patch does not
> decouple #ifdef DEBUG from SUINT.

I suggested it previously to decouple them.
This patch is in no way related to the coupling, with or without the
coupling the patch wouldnt change


> 
> Now the question is whether SUINT has any raison d'etre in our main
> codebase. If it's under #if 0 or otherwise "dead code", any Diego'ification
> would immediately get rid of it. So answer me this question: if the code is
> under #if 0, why shouldn't it just exist locally on your hard disk only?

as has been said many times SUINT is essential to test for one class
of bugs with ubsan.
If its in a private branch, only i would test with it, only i would
maintain it until i lost interrest and it bit rots, if that happens
we lose the ability to test for this class of bugs

do people want this ?


>
> (Yes, I have actual (old) debug branches for the vp9 decoder locally.)

i have all kinds of branches too, but this isnt specific to a single
codec or a single developer

Do you maintain some branch that contains type changes
accorss the codebase that you know you will need repeatly in the
future ?


[...]

Patch hide | download patch | download mbox

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)--;