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

[Michael Niedermayer]

On Sun, Aug 18, 2019 at 12:00:45PM +0200, Paul B Mahol wrote:
> On Sun, Aug 18, 2019 at 11:44 AM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Sun, Aug 18, 2019 at 10:47:26AM +0200, Tomas Härdin wrote:
> > > 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.
> >
> > not calling any form of *get_buffer per frame breaks decoding into
> > user supplied buffers.
> >
> > If you check the documentation of the get_buffer2 callback
> >
> > " This callback is called at the beginning of each frame to get data
> > buffer(s) for it."
> >
> > That would not be possible if its just called once in init
> >
> > and yes i too wish there was a magic fix but i think most things that
> > look like magic fixes have a fatal flaw. But maybe iam missing something
> > in fact i hope that iam missing something and that there is a magic fix
> >
> 
> Magic fix is enabling reference counted frames in fuzzer.

That is covered by the part below which you maybe did not read


> 
> 
> >
> > PS: if you think of changing the API, i dont think its the API.

> > I mean every user application will read the frames it receives, so
> > even if inside the decoder we just return the same frame with 2 pixels
> > different the user doesnt know this and has to read the whole frame.
> > The problem is moved around but its still there.

This above

Its a very specific feature of the fuzzer currently that it does not
read the returned frame. But basically every real application will 
read it, it would be rather pointless to decode if its not read.

Thanks

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20190818/da5343f5/attachment.sig>