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

[Michael Niedermayer]

On Thu, Jan 14, 2016 at 12:30:32PM -0500, Ronald S. Bultje wrote:
> 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?

iam not too happy about it either

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160115/30f0f1ee/attachment.sig>