[FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()

[Tomas Härdin]

tis 2018-11-20 klockan 00:04 +0100 skrev Michael Niedermayer:
> On Mon, Nov 19, 2018 at 09:37:28AM +0100, Tomas Härdin wrote:
> > mån 2018-11-19 klockan 02:02 +0100 skrev Michael Niedermayer:
> > > On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> > > > lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > > > > Fixes: 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> > > > > 
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > 
> > > > > ---
> > > > >  libavcodec/truemotion2.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > > > index 58a577f53c..c583ff4032 100644
> > > > > --- a/libavcodec/truemotion2.c
> > > > > +++ b/libavcodec/truemotion2.c
> > > > > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int stride, int *last, unsigned *C
> > > > >      }
> > > > >  }
> > > > >  
> > > > > -static inline void tm2_low_chroma(int *data, int stride, int *clast, int *CD, int *deltas, int bx)
> > > > > +static inline void tm2_low_chroma(int *data, int stride, int *clast, unsigned *CD, int *deltas, int bx)
> > > > >  {
> > > > >      int t;
> > > > >      int l;
> > > > > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int stride, int *clast, int *CD, in
> > > > >          prev = clast[-3];
> > > > >      else
> > > > >          prev = 0;
> > > > > -    t        = (CD[0] + CD[1]) >> 1;
> > > > > -    l        = (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > > > > +    t        = (int)(CD[0] + CD[1]) >> 1;
> > > > 
> > > > I presume the old code would overflow for sums exceeding INT_MAX. 
> > > 
> > > There were overflows in the sense of undefined behavior, yes
> > > 
> > > 
> > > > Why
> > > > then is there a cast to int before the shift and not after?
> > > 
> > > because shifts of signed and unsigned values produce different results
> > > 
> > > maybe the unsigned type confused you. CD is signed in a semantic sense its
> > > stored in a unsigned type because otherwise this code has undefined behavior.
> > > It would be better to use a type with different name but that had lead to
> > > long arguments that went nowhere in the past. So i tend to use plain
> > > unsigned now for these kind of cases as it seems that is what most people
> > > prefer.
> > 
> > So signed overflow is intended? Huh, weird format.
> 
> it occurs in the fuzzed sample used here. i do not know if it occurs in
> normal samples, i would suspect it does not.

I see. Some kind of spec might be nice, but I suspect the spec as it is
is named Kostya

/Tomas