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

Paul B Mahol onemda at gmail.com
Sun Aug 18 13:25:55 EEST 2019


On Sun, Aug 18, 2019 at 12:19 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> 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.
>
>
No, you are very mistaken and confused.


> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list