[FFmpeg-devel] [PATCH 1/5] avcodec/cinepak: move some checks prior to frame allocation

[Michael Niedermayer]

On Wed, Apr 18, 2018 at 09:07:20PM +0200, Tomas Härdin wrote:
> tis 2018-04-17 klockan 14:24 +0200 skrev Michael Niedermayer:
> > On Tue, Apr 17, 2018 at 10:32:16AM +0200, Tomas Härdin wrote:
> > > tis 2018-04-17 klockan 02:13 +0200 skrev Michael Niedermayer:
> > > > Speeds up decoding from 8 to 3 seconds for 6302/clusterfuzz-
> > > > testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-
> > > > 5626371985375232
> > > > Fixes: Timeout
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/os
> > > > s-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > 
> > > > ---
> > > >  libavcodec/cinepak.c | 27 ++++++++++++++++++++++-----
> > > >  1 file changed, 22 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
> > > > index 89e940ae0d..ba0589582f 100644
> > > > --- a/libavcodec/cinepak.c
> > > > +++ b/libavcodec/cinepak.c
> > > > @@ -315,14 +315,11 @@ static int cinepak_decode_strip
> > > > (CinepakContext *s,
> > > >      return AVERROR_INVALIDDATA;
> > > >  }
> > > >  
> > > > -static int cinepak_decode (CinepakContext *s)
> > > > +static int cinepak_predecode_check (CinepakContext *s)
> > > >  {
> > > > -    const uint8_t  *eod = (s->data + s->size);
> > > > -    int           i, result, strip_size, frame_flags,
> > > > num_strips;
> > > > -    int           y0 = 0;
> > > > +    int           num_strips;
> > > >      int           encoded_buf_size;
> > > >  
> > > > -    frame_flags = s->data[0];
> > > >      num_strips  = AV_RB16 (&s->data[8]);
> > > >      encoded_buf_size = AV_RB24(&s->data[1]);
> > > >  
> > > > @@ -353,6 +350,21 @@ static int cinepak_decode (CinepakContext
> > > > *s)
> > > >              s->sega_film_skip_bytes = 0;
> > > >      }
> > > >  
> > > > +    if (s->size < 10 + s->sega_film_skip_bytes + num_strips *
> > > > 12)
> > > > +        return AVERROR_INVALIDDATA;
> > > 
> > > Looks like an extra check, not just moving existing checks as the
> > > commit message implies
> > 
> > its from this:
> >     for (i=0; i < num_strips; i++) {
> >         if ((s->data + 12) > eod)
> >             return AVERROR_INVALIDDATA;
> >     ...
> > 
> > But i can split this in a separte patch if you prefer ?
> 
> Nah, I think this discussion being archived should be enough

will apply

thx

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20180419/15f30d5b/attachment.sig>