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