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

[Michael Niedermayer]

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


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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20160114/e1384dd0/attachment.sig>