[FFmpeg-devel] [PATCH] avcodec/rl2: set dimensions

[Lynne]

Jul 23, 2019, 12:00 AM by michael at niedermayer.cc:

> On Tue, Jul 23, 2019 at 12:13:51AM +0200, Lynne wrote:
>
>> Jul 22, 2019, 10:57 PM by michael at niedermayer.cc:
>>
>> > The dimensions are always 320x200 they are hardcoded in the demuxer.
>> > Hardcode them instead in the decoder.
>> >
>> > Fixes: Timeout (16sec -> 400ms)
>> > Fixes: 15574/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RL2_fuzzer-5158614072819712
>> >
>> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > ---
>> >  libavcodec/rl2.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/libavcodec/rl2.c b/libavcodec/rl2.c
>> > index 6662979c52..2d336a61e5 100644
>> > --- a/libavcodec/rl2.c
>> > +++ b/libavcodec/rl2.c
>> > @@ -134,10 +134,15 @@ static av_cold int rl2_decode_init(AVCodecContext *avctx)
>> >  Rl2Context *s = avctx->priv_data;
>> >  int back_size;
>> >  int i;
>> > +    int ret;
>> > 
>> >  s->avctx       = avctx;
>> >  avctx->pix_fmt = AV_PIX_FMT_PAL8;
>> > 
>> > +    ret = ff_set_dimensions(avctx, 320, 200);
>> > +    if (ret < 0)
>> > +        return ret;
>> > +
>> >  /** parse extra data */
>> >  if (!avctx->extradata || avctx->extradata_size < EXTRADATA1_SIZE) {
>> >  av_log(avctx, AV_LOG_ERROR, "invalid extradata size\n");
>> > -- 
>> > 2.22.0
>> >
>> > _______________________________________________
>> > 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 <mailto:ffmpeg-devel-request at ffmpeg.org>>  with subject "unsubscribe".
>> >
>>
>> If the size somehow gets lost between lavf and lavc then the problem is there, and should be solved by not fudging the decoder.
>>
>
> libavcodec is used by many applications which do not use libavformat.
> for all i know and what is documented in the only reference spec i have. 
> the resolution is a constant its always 320x200 for this codec. So its
> natural to have the decoder know about its own resolution. 
>

Nonetheless, while I agree with having both decoder and demuxer hardcode the resolution, I still think this is fudging.



>> Moreover, if neither the codec, nor container can change the resolution, then how does that make anything different?
>>
>
>
>> Please, stop it with those patches. It takes energy and some worry to have to open up the ML and scan it for bad patches from people who really should know better every time.
>>
>
> Please stop with these attacks, this is just making people become
> mutually more aggressive and filled with hatred.
>

Of course I'm frustrated, I've been ignored.
These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. Sure, protecting against ddos attacts is important, but not important enough to make decoders give up early and return nothing. Especially in cases where the timeout speedup is of the order of 2s to 400ms.
Yet in all of those timeout patches all you've cared about is shutting up the tool. You've never once shown any figures if this could affect decoding, because its a lot harder than just showing they fix something some tool calls a timeout and forgetting about it. You haven't even commented on this when I asked you on IRC.
You also sneak this type of patches in when there's an overflow later on during decoding, which is completely incorrect in almost all cases, for the same reason above.

If people really wanted to ddos anything using decoders they can easly do so by sending over a gzip compressed specially crafted vobis file to allocate tons of memory and spend CPU. Something every browser supports, rather than use old codecs disabled by everyone.