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

[Tomas Härdin]

sön 2019-08-18 klockan 02:35 +0200 skrev 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:
> > > 
> > > 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.

I did some investigation, it is indeed ff_reget_buffer(). It copies the
frame data for some reason. The fix is simple in this case: just call
ff_get_buffer() once in cinepak_decode_init() and keep overwriting the
same frame.

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

And related to this, ff_reget_buffer() is used for a lot of these
codecs which only overwrite pixels in the old frame. flicvideo, gifdec,
msrle, roqvideodec and others probably have the same flaw.

Patched attached.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavcodec-cinepak-Avoid-copying-frame-data.patch
Type: text/x-patch
Size: 1535 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190818/12bdf637/attachment.bin>