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

[Tomas Härdin]

lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer:
> On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote:
> > fre 2019-08-16 klockan 14:57 +0200 skrev 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've been playing around with the Windows 3.1 cinepak decoder, and it
> > seems one indeed has to code every MB even if they don't change. I
> > suspect the reason is because that decoder uses double buffering and
> > they wanted to avoid doing more memcpy()s than absolutely necessary. If
> > one tries to play tricks like coding strips that are only 4x4 pixels to
> > indicate a frame without changes then parts of the video will be
> > replaced by garbage. So demanding number_of_bits >= number_of_mbs is
> > certainly in line with that decoder.
> > 
> > I feel I should point out that being conservative here is at odds with
> > the general "best effort" approach taken in this project. These toy
> > codecs are useful as illustrative examples of this contradiction. I'm
> > sure there are many more examples of files that can cause ffmpeg to do
> > a lot more work than expected, for almost every codec. I know afl-fuzz
> > is likely to find out that it can make the ZMBV decoder do a lot of
> > work for a small input file, because I've seen it do that with gzip.
> > 
> > The user base for cinepak is of course miniscule, so I doubt anyone's
> > going to complain that their broken CVID files don't work any more. I
> > certainly don't care since cinepakenc only puts out valid files. 
> > But
> > again, for other formats we're just going to have to tell users to put
> > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is
> > vulnerable to DoS-y things.
> 
> yes
> 
> the question ATM is just what to do here about this codec ?
> apply the patch ?
> change it ?

Well for a start, the file is 65535 x 209 pixels, 3166 frames. I
wouldn't call decoding that @ 263 fps particularly slow

Second, it's not the decoder which is slow. If I comment out the
"*got_frame = 1;" then the test also runs fast. I'm not sure what
happens elsewhere with the decoded buffer, but I suspect there's a
bunch of useless malloc()/memset()ing going on. Maybe the decoder is
using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure.

As I said on IRC, this class of problems will exist for every codec.
Cinepak is easy to decode, even at these resolutions. Just imagine what
will happens when someone feeds in a 65535x209 av1 stream..

/Tomas