[FFmpeg-devel] [PATCH] swscale/yuv2rgb: Increase YUV2RGB table headroom

[Ronald S. Bultje]

Hi,

On Thu, Jan 14, 2016 at 12:06 PM, Michael Niedermayer <
michael at niedermayer.cc> wrote:

> On Thu, Jan 14, 2016 at 11:42:47AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Thu, Jan 14, 2016 at 11:34 AM, Michael Niedermayer <
> > michael at niedermayer.cc> wrote:
> >
> > > On Thu, Jan 14, 2016 at 10:09:13AM -0500, Ronald S. Bultje wrote:
> > > > Hi,
> > > >
> > > > On Wed, Jan 13, 2016 at 9:50 PM, Michael Niedermayer <
> michaelni at gmx.at>
> > > > wrote:
> > > >
> > > > > From: Michael Niedermayer <michael at niedermayer.cc>
> > > > >
> > > > > This makes SWS more robust
> > > > > Fixes:
> > > > >
> > >
> 07650a772d98aa63b0fed6370dc89037/asan_heap-oob_27ddeaf_2657_2c81ff264dee5d9712cb3251fb9c3bbb.264
> > > > > Fixes: out of array read
> > > > >
> > > > > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libswscale/swscale_internal.h |    2 +-
> > > > >  libswscale/yuv2rgb.c          |   88
> > > > > ++++++++++++++++++++---------------------
> > > > >  2 files changed, 45 insertions(+), 45 deletions(-)
> > > >
> > > >
> > > > So ... I'm kind of confused. You have a 264 file that triggers a OOB
> in
> > > > swscale, probably through automated testing of ffmpeg.exe -i
> file.264 -f
> > > > null -. Can you elaborate on what happens exactly? I guess what I'm
> > > trying
> > > > to get at is, what's the input format (I'm going to assume it's
> yuv420),
> > > > what's the output (maybe rgb24?), why is it converting like that (is
> the
> > > > 264 changing pixfmt from frame to frame?), are the coefficients
> defaults
> > > > for a particular colorspace/range (e.g. bt601/mpeg) or custom, what
> _are_
> > > > the coefficients, what coefficients can swscale use in these
> functions,
> > > and
> > > > what are the theoretical bounds of the index in each array based on
> these
> > > > coefficients?
> > > >
> > > > In other words, why do we need a headroom of 256/512/384/326/896/838
> in
> > > > each of these tables? How do we verify that it is correct? I remember
> > > Jason
> > > > tried to speed up av_clip_uint8() at some point by making it a
> table, and
> > > > we had to revert the use of that in many places (e.g. idcts) because
> we
> > > > cannot give a theoretical limit on input values, i.e. the table would
> > > have
> > > > to be infinitely long. I'm trying to figure out if that's the case
> here
> > > > also.
> > >
> > > input was IIRC 10bit YUV (probably with out of range values)
> >
> >
> > Ah! This gets very interesting. OK, so then it all makes a lot of sense.
> So
> > ... Is this valid input? I mean, the h264 decoder clips the output in
> each
> > stage (prediction, idct, loopfilter) to fit within the range, no? Or is
> > this something like PCM or whatever where we don't clip? Should we?
> >
> > I'm not saying we shouldn't allow clipping of input values in swscale
> also,
> > but perhaps we could have a flag that says that the input is already
> > clipped (similar to swresample).
>
> possibly, but that might be hard to get right
> one path for too big values is uinitialized memory, our error
> concealment code doesnt support some corner cases so it doesnt always
> clean all unset set stuff up, memory is also IIRC not cleared after
> alloc
>
> my reasoning for fixing it in swscale was that users will expect
> sws not to crash from out of range input samples


Uninitialized data in output frames seems like a pretty big deal to me
regardless of bugs in swscale, no?

Ronald