Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
sön 2019-08-18 klockan 19:04 +0200 skrev Michael Niedermayer: > On Sun, Aug 18, 2019 at 02:49:39PM +0200, Tomas Härdin wrote: > > sön 2019-08-18 klockan 14:18 +0200 skrev Michael Niedermayer: > > > On Sun, Aug 18, 2019 at 01:40:01PM +0200, Tomas Härdin wrote: > > > > sön 2019-08-18 klockan 12:19 +0200 skrev 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 > > > > > > > > > > > > > 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. > > > > > > > > Copying is still slower than not copying. Enabling refcounting fixes > > > > the timeout issue here, and will likely silence a whole bunch of false > > > > positives for this class of files. > > > > > > it makes probably sense to enable ref counting but we should > > > immedeatly in the next or a previous commit make the fuzzer read the > > > frames > > > from the decoder. Thats what basically every user app would do. > > > Otherwise we would have a bunch of issues closed and then reopened > > > later. > > > > Why should we care how much work the user does? We're fuzzing lavc > > here, not user programs. Certain use cases are decode-only (ffprobe > > -show_frames for example) > > thats a valid point of view > > The user though has few options if she gets many frames, she can just > continue processing or stop, she doesnt know how (in)valid the stream is This is way too abstract. Who is this user, specifically? If we were talking about ffmpeg, handbrake, peertube or something else then we could actually help them if they're using the API incorrectly. Worrying about hypotheticals is a waste of time. You can't test them. > OTOH libavcodec knows the codec bitstream, and can check various things > > so just moving the responsibility to the user app would move it where > its much harder to fix well > That is currently, we could export all kinds of metadata about the > amount of errors. Then a user app could decide what to do. > It would become duplicated code between user apps though... No one outside of forensics or satellite TV is going to care about that, and if they do they can pay for it, and we can maintain tests for it. It's not hard for a user to add checks like "if resolution > 3840 × 2160 then reject the file", which makes a lot of these fuzzy "anti- DoS" patches pointless busywork. It just causes developer fatigue. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Sun, Aug 18, 2019 at 02:49:39PM +0200, Tomas Härdin wrote: > sön 2019-08-18 klockan 14:18 +0200 skrev Michael Niedermayer: > > On Sun, Aug 18, 2019 at 01:40:01PM +0200, Tomas Härdin wrote: > > > sön 2019-08-18 klockan 12:19 +0200 skrev 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 > > > > > > > > > > > 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. > > > > > > Copying is still slower than not copying. Enabling refcounting fixes > > > the timeout issue here, and will likely silence a whole bunch of false > > > positives for this class of files. > > > > it makes probably sense to enable ref counting but we should > > immedeatly in the next or a previous commit make the fuzzer read the frames > > from the decoder. Thats what basically every user app would do. > > Otherwise we would have a bunch of issues closed and then reopened > > later. > > Why should we care how much work the user does? We're fuzzing lavc > here, not user programs. Certain use cases are decode-only (ffprobe > -show_frames for example) thats a valid point of view The user though has few options if she gets many frames, she can just continue processing or stop, she doesnt know how (in)valid the stream is OTOH libavcodec knows the codec bitstream, and can check various things so just moving the responsibility to the user app would move it where its much harder to fix well That is currently, we could export all kinds of metadata about the amount of errors. Then a user app could decide what to do. It would become duplicated code between user apps though... > > > an alternative viewpoint to this would be to set the refcounting flag > > from the input so the fuzzer itself has control over it and we test > > both codepathes. This would improve coverage > > Not a bad idea. But as this example shows, not refcounting has > performance implications. The fuzzer should be more lenient with > timeout in that case, imo. this is possible, yes Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
sön 2019-08-18 klockan 14:18 +0200 skrev Michael Niedermayer: > On Sun, Aug 18, 2019 at 01:40:01PM +0200, Tomas Härdin wrote: > > sön 2019-08-18 klockan 12:19 +0200 skrev 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 > > > > > > > > > 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. > > > > Copying is still slower than not copying. Enabling refcounting fixes > > the timeout issue here, and will likely silence a whole bunch of false > > positives for this class of files. > > it makes probably sense to enable ref counting but we should > immedeatly in the next or a previous commit make the fuzzer read the frames > from the decoder. Thats what basically every user app would do. > Otherwise we would have a bunch of issues closed and then reopened > later. Why should we care how much work the user does? We're fuzzing lavc here, not user programs. Certain use cases are decode-only (ffprobe -show_frames for example) > an alternative viewpoint to this would be to set the refcounting flag > from the input so the fuzzer itself has control over it and we test > both codepathes. This would improve coverage Not a bad idea. But as this example shows, not refcounting has performance implications. The fuzzer should be more lenient with timeout in that case, imo. > > It would be beneficial to have a consistent way of signalling that a > > frame didn't change, since a bunch of codecs have that property. > > Currently it's a mix of discontinuous timestamps, longer frame > > durations and repeating identical frames. > > yes, i strongly agree. > > > > so feeding really crazy resolutions into a decoder requires some > > > small but proportional amout of input bytes. > > > double the width and the minimum input bytes double sort of. > > > > Lavc is already very lenient with what it accepts. How do you detect > > the difference between "this packet is too small to decode to an entire > > frame" from "this packet is too small but we could still get a few MBs > > out of it"? > > In reality this is actually not hard because a frame that is smaller > than the minimum valid size is generally for many codecs so small > it really wont contain anything usefull to decode. > And we have discard_damaged_percentage where the user can tune it too. > Patches using discard_damaged_percentage are sometimes objected too > though so it is not consistently used. I remain skeptical of this, since it makes the parsing more complicated and slower, and increases mental load. There's a sliding scale of strictness here, and if I try to implement even something basic as "the length in the header should match the length of the actual data" then FATE breaks. We can of course change FATE to accept that changed strictness. I get the feeling whenver we put in checks like this it's only a matter of time before the fuzzers find out some other way to trip up the decoders. This is why I advocate for maximum strictness unless there's a good reason to not be strict. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Sun, Aug 18, 2019 at 01:40:01PM +0200, Tomas Härdin wrote: > sön 2019-08-18 klockan 12:19 +0200 skrev 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 > > > > > > 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: > > > > > > > > > > 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 > > Sorry, I'm a bit rusty on lavc internals. > > > > > 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. > > Copying is still slower than not copying. Enabling refcounting fixes > the timeout issue here, and will likely silence a whole bunch of false > positives for this class of files. it makes probably sense to enable ref counting but we should immedeatly in the next or a previous commit make the fuzzer read the frames from the decoder. Thats what basically every user app would do. Otherwise we would have a bunch of issues closed and then reopened later. an alternative viewpoint to this would be to set the refcounting flag from the input so the fuzzer itself has control over it and we test both codepathes. This would improve coverage > > It would be beneficial to have a consistent way of signalling that a > frame didn't change, since a bunch of codecs have that property. > Currently it's a mix of discontinuous timestamps, longer frame > durations and repeating identical frames. yes, i strongly agree. > > > so feeding really crazy resolutions into a decoder requires some > > small but proportional amout of input bytes. > > double the width and the minimum input bytes double sort of. > > Lavc is already very lenient with what it accepts. How do you detect > the difference between "this packet is too small to decode to an entire > frame" from "this packet is too small but we could still get a few MBs > out of it"? In reality this is actually not hard because a frame that is smaller than the minimum valid size is generally for many codecs so small it really wont contain anything usefull to decode. And we have discard_damaged_percentage where the user can tune it too. Patches using discard_damaged_percentage are sometimes objected too though so it is not consistently used. Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
sön 2019-08-18 klockan 12:19 +0200 skrev 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 > > > > 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: > > > > > > > > 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 Sorry, I'm a bit rusty on lavc internals. > > > 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. Copying is still slower than not copying. Enabling refcounting fixes the timeout issue here, and will likely silence a whole bunch of false positives for this class of files. It would be beneficial to have a consistent way of signalling that a frame didn't change, since a bunch of codecs have that property. Currently it's a mix of discontinuous timestamps, longer frame durations and repeating identical frames. > so feeding really crazy resolutions into a decoder requires some > small but proportional amout of input bytes. > double the width and the minimum input bytes double sort of. Lavc is already very lenient with what it accepts. How do you detect the difference between "this packet is too small to decode to an entire frame" from "this packet is too small but we could still get a few MBs out of it"? FTR I've been in favor of rejecting broken files for a while now, and I'm in favor of something like checking that the number of bytes in all strips add up to some minimum. This can be combined with a pre-parsing step that also rejects the input if any chunk has incorrect size. > codecs OTOH which allow coding a whole frame in 10bytes input > independant of the resolution behave worse in this way > as that can produce orders of magnitude more load per bandwidth > the attacker needs. Users are ultimately responsible for limiting how much resources their programs can use. As you say, getting high amplification isn't hard. Not even ffprobe is safe from this. I've been in talks with the peertube people about this very topic not too long ago. Stuff like this is why I've been harping on about parsers and verifying things, and being very specific about what we accept. Unfortunately FFculture seems to be against this. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Sun, Aug 18, 2019 at 12:19 PM Michael Niedermayer 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 > > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 > 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 signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Sun, Aug 18, 2019 at 02:35:45AM +0200, Tomas Härdin wrote: > 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.. i dont know about av1 specifically without checking the specs but with most of the mainstream codecs. areas not covered by slices are
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Sun, Aug 18, 2019 at 11:44 AM Michael Niedermayer 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. > > 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. > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > During times of universal deceit, telling the truth becomes a > revolutionary act. -- George Orwell > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 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. Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB During times of universal deceit, telling the truth becomes a revolutionary act. -- George Orwell signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 From c85f23ca4c42f9ce27f512be897214b8689c1c94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= Date: Sun, 18 Aug 2019 10:30:59 +0200 Subject: [PATCH] libavcodec/cinepak: Avoid copying frame data We can just keep overwriting the same frame. This speeds up the decoder, especially for very large frames, since skip MBs are turned into nops. clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 32577 ms -> 42 ms --- libavcodec/cinepak.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index aeb15de0ed..e3e6ecfdf0 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -424,6 +424,7 @@ static int cinepak_decode (CinepakContext *s) static av_cold int cinepak_decode_init(AVCodecContext *avctx) { CinepakContext *s = avctx->priv_data; +int ret; s->avctx = avctx; s->width = (avctx->width + 3) & ~3; @@ -444,6 +445,9 @@ static av_cold int cinepak_decode_init(AVCodecContext *avctx) if (!s->frame) return AVERROR(ENOMEM); +if ((ret = ff_get_buffer(avctx, s->frame, 0)) < 0) +return ret; + return 0; } @@ -473,9 +477,6 @@ static int cinepak_decode_frame(AVCodecContext *avctx, return ret; } -if ((ret = ff_reget_buffer(avctx, s->frame)) < 0) -return ret; - if (s->palette_video) { int size; const uint8_t *pal = av_packet_get_side_data(avpkt, AV_PKT_DATA_PALETTE, ); -- 2.20.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 ? check the first slice as in: @@ -359,7 +359,16 @@ static int cinepak_predecode_check (CinepakContext *s) if (num_strips) { const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes; int strip_size = AV_RB24 (data + 1); -if (strip_size < 12 || strip_size > encoded_buf_size) + +if (!(s->strips[0].y1 = AV_RB16 ([4]))) +s->strips[0].y2 = (s->strips[0].y1 = 0) + AV_RB16 ([8]); +else +s->strips[0].y2 = AV_RB16 ([8]); + +if (strip_size < 12 || strip_size > encoded_buf_size || +s->strips[0].y2 <= s->strips[0].y1 || +((s->avctx->width * (int64_t)(s->strips[0].y2 - s->strips[0].y1)) / 16 + 7)/8 > s->size +) return AVERROR_INVALIDDATA; } just raise the "threshold" in tools/target_dec_fuzzer.c for cinepak? something else ? This is not really a technical question, its a question what is preferred. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The misfortune of the wise is better than the prosperity of the fool. -- Epicurus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
On Fri, Aug 16, 2019 at 02:57:42PM +0200, Tomas Härdin wrote: > 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? will send it privatly [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer > > > --- > > > libavcodec/cinepak.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > > index aeb15de0ed..62eb794332 100644 > > > --- a/libavcodec/cinepak.c > > > +++ b/libavcodec/cinepak.c > > > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > > > (CinepakContext *s) > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > > > return AVERROR_INVALIDDATA; > > > > > > +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. > > 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 thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer > > --- > > libavcodec/cinepak.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > index aeb15de0ed..62eb794332 100644 > > --- a/libavcodec/cinepak.c > > +++ b/libavcodec/cinepak.c > > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > > (CinepakContext *s) > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > > return AVERROR_INVALIDDATA; > > > > +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'd replace s->avctx->height in the expression with the height of the first strip, to not constrain things too much. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
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 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/cinepak.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > index aeb15de0ed..62eb794332 100644 > --- a/libavcodec/cinepak.c > +++ b/libavcodec/cinepak.c > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > (CinepakContext *s) > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > return AVERROR_INVALIDDATA; > > +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. Finally, I don't think we should accept files with num_strips > MAX_STRIPS in cinepak_decode(). We should ask for samples of them instead. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input
Fixes: Timeout (12sec -> 32ms) Fixes: 16078/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/cinepak.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index aeb15de0ed..62eb794332 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -356,6 +356,9 @@ static int cinepak_predecode_check (CinepakContext *s) if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) return AVERROR_INVALIDDATA; +if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) +return AVERROR_INVALIDDATA; + if (num_strips) { const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes; int strip_size = AV_RB24 (data + 1); -- 2.22.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".