[FFmpeg-devel] [PATCH 2/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int'

[wm4]

On Sat, 11 Mar 2017 01:26:33 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote:
> > On Fri, 10 Mar 2017 15:24:52 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > Fixes: 755/clusterfuzz-testcase-5369072516595712
> > > 
> > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int'
> > > 
> > > 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/h264_direct.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c
> > > index cbb84665b3..66e54479d1 100644
> > > --- a/libavcodec/h264_direct.c
> > > +++ b/libavcodec/h264_direct.c
> > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl,
> > >                              int poc, int poc1, int i)
> > >  {
> > >      int poc0 = sl->ref_list[0][i].poc;
> > > -    int td = av_clip_int8(poc1 - poc0);
> > > +    int64_t pocdiff = poc1 - (int64_t)poc0;
> > > +    int td = av_clip_int8(pocdiff);
> > > +
> > > +    if (pocdiff != (int)pocdiff)
> > > +        avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n");
> > > +
> > >      if (td == 0 || sl->ref_list[0][i].parent->long_ref) {
> > >          return 256;
> > >      } else {  
> > 
> > Hard to image that these poc values aren't bounded by something else,
> > but I don't know.  
> 
> > 
> > Also the previous patch didn't have this request_sample call, which
> > inflates this whole thing by 5 lines of code.  
> 
> yes thats why i suggested it initially.
> SUINT allows overflow detection simply by  #define CHECKED 1
> and running under ubsan
> 
> otherwise an excplicit check is needed to detect such occurances

You can either
1. ignore the error in some way that doesn't cause problems
2. ignore the error in some way that doesn't cause problems in debug
   mode
3. make the error explicit and log it

Your first patch did 2 (which I find questionable, btw.), your current
patch does 3 - why not do 1? That solution seems safest and has the
smallest footprint.