On Mon, Dec 12, 2011 at 04:16:13PM +0000, Måns Rullgård wrote: > Laurent Aimar <[email protected]> writes: > > > On Mon, Dec 12, 2011 at 07:35:44AM -0800, Ronald S. Bultje wrote: > >> Hi guys, > >> > >> the idea of this patch is very simple, and has existed in Chrome in a > >> slightly > >> different form for quite a while: for each call to get_bits() and related > >> functions, check for overreads before advancing the frame pointer. > >> > >> This protects against overreads in most decoders except those that use > >> custom > >> bitreaders, like VP8 (which already does this), CABAC (which I'll do > >> later) and > >> a few more. Speed depends on how many bits are read per frame, but ranges > >> from > >> a 1-10% loss on low to ultra-high bitrate CAVLC H264 streams, to > >> unnoticeable > >> losses of <0.1% on e.g. VC1 (because the DSP is less optimized, so the > >> bitreader overhead is relatively lower). > >> > >> Design is that there's a configure option (currently default-off) to > >> enable the > >> "safe" bitstream reader, and individual decoders that do bitstream checks > >> themselves can turn it off on a per-decoder basis. > > > > I think that with your patch, get_bits_count() will never be negative and > > this > > will creates issues with some decoders. > > You may want to read the following thread > > "[PATCH] Checked get_bits.h functions to prevent overread" > > (libav-devel/ffmpeg-devel) > > to see the associated discussions/arguments. > > > > I attached the version of a similar patch that I came up with after taking > > thoses arguments into account. (I never created the configue part). > > > > Regards, > > > > -- > > fenrir > > > >>From af1e5c3a989291a1e78b1f0dbeabf76f5e8bdfc6 Mon Sep 17 00:00:00 2001 > > From: Laurent Aimar <[email protected]> > > Date: Fri, 9 Sep 2011 00:52:36 +0200 > > Subject: [PATCH] Prevent by default for overread in get_bits.h functions. > > > > It can be disabled (at compilation time so without any performance loss) > > for decoder that check for overreads by themselves by defining > > UNCHECKED_BITSTREAM_READER before including get_bits.h > > > > Checks for A32_BITSTREAM_READER are not yet implemented. > > --- > > libavcodec/get_bits.h | 34 ++++++++++++++++++++++++++++++---- > > 1 files changed, 30 insertions(+), 4 deletions(-) > > This patch a) doesn't protect against extreme overflows, and b) probably > has more performance impact. For a), I don't really see where. The only case where I see an issue is that the patch let the index do a signed overflow. That could be fixed. But for the rest of the patch, as the index is compared with the size in an unsigned way, there should be no issue.
For b), yes, but a lot of current decoder expect get_bit_count() to be < 0 when a small overread as been done. If this apsumption is broken, then the supplied patch is not really usefull until all thoses decoders are reviewed/changed. Regards, -- fenrir _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
