[FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input

[Tomas Härdin]

fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > Fixes: Timeout (12sec -> 32ms)
> > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > [...]
> > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > +        return AVERROR_INVALIDDATA;
> > > 
> > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > could merge it with the check above into something like:
> > > 
> > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > 0)) {
> > >     return AVERROR_INVALIDDATA;
> > > }
> > > 
> > > The check further down could also check each strip's size, not just the
> > > first one.
> > 
> > Actually, thinking a bit more about this, I suspect it might be legal
> > to have strips that don't cover the entire frame. It would certainly be
> > good to support that, for optimizing skip frames. Not sure what old
> > decoders make of that however. A skip could potentially be encoded in
> > 22 + (width/4 + 7)/8 bytes while still being compatible.
> 
> i was asking myself the same questions before writing the patch, and
> in the "spec" there is
> "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> "Each" meaning no holes to me. If thats actually true for all real files
> that i do not know.

We should keep in mind the spec is fine with there being zero strips.
It's only if one wants to support certain decoders that there will/must
be one or more strips.

I don't have any way of testing the MacOS decoder, but the old Win3.1
decoder is easy enough to get running. Then I can also check whether
strips narrower than avctx->width are OK.

> > I'd replace s->avctx->height in the expression with the height of the
> > first strip, to not constrain things too much.
> 
> ill post a patch doing that 

I feel like a better fix would be to make the decoder not choke on the
kind of files that trigger this. With this style of check it's still
possible to insert a second strip that triggers the slowness, with a
trivially sized strip before it.

Would you mind sending me the sample?

/Tomas