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

[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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181120/f77e43d2/attachment.sig>