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

[Michael Niedermayer]

On Tue, Jul 23, 2019 at 04:42:32AM +0200, Lynne wrote:
> 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.

What did you expect ? IIRC you have asked for whole classes of security
issues to be not fixed.

Something like that would require a vote and majority of developers.



> These patches affect decoding of real world broken files in favor of fixing specially crafted fuzzed files. 

Iam happy to look into such cases. Can you provide me with such
"real world broken files"?
Its not intended to worsen the output from such 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 you know of issues in a patch or commit you should report this
during patch review or as soon as you find out about the issue
as a reply to that patch or commit or as mail to the author.

So the author or any other developer can fix it or the patch is on hold
until its fixed.

Complaining out of context about unspecified issues. Or well IRC  (i dont
know what you refer to here at all either)
Is just guranteed to lead to frustration because that wont and cant lead to
issues being fixed

PS: Again, i strongly suggest we work together to find acceptable solutions
to these issues. Because that would lead to good solutions, and everyone
being non frustrated. When one demands "dont fix it" that is what leads
to a fight and frustration and a blocked patch so really both sides are
unhappy and frustrated


Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- 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/20190724/b4e58642/attachment.sig>