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

Michael Niedermayer michael at niedermayer.cc
Fri Apr 7 14:07:23 EEST 2017


On Fri, Apr 07, 2017 at 12:35:37PM +0200, wm4 wrote:
> On Fri, 7 Apr 2017 12:22:07 +0200
> Michael Niedermayer <michael at 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 at 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 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    
> > > > > 
> > > > > 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170407/4711cef8/attachment.sig>


More information about the ffmpeg-devel mailing list