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. > > Please comment. > > Ronald
> From de04ee508025316093b04c9058dd18e9e311c5ea Mon Sep 17 00:00:00 2001 > From: Ronald S. Bultje <[email protected]> > Date: Sat, 3 Dec 2011 16:30:43 -0800 > Subject: [PATCH] get_bits: introduce safe bitreading to prevent overreads. > > Based on work (for GCI) by Aneesh Dogra <[email protected]>, and > inspired by patch in Chromium by Chris Evans <[email protected]>. > > When turned on, this makes H264/CAVLC 1-10% slower (depending on > bitrate). Other codecs are affected in a similar manner. > --- > configure | 2 ++ > libavcodec/get_bits.h | 43 ++++++++++++++++++++++++++++++++++++++----- > libavcodec/wmavoice.c | 2 ++ > 3 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/configure b/configure > index a0dc6e0..f7311f2 100755 > --- a/configure > +++ b/configure > @@ -113,6 +113,7 @@ Configuration options: > --disable-dxva2 disable DXVA2 code > --enable-runtime-cpudetect detect cpu capabilities at runtime (bigger > binary) > --enable-hardcoded-tables use hardcoded tables instead of runtime > generation > + --disable-safe-bitstream-reader disable buffer boundary checking in > bitreaders (faster, but may crash) --this-option-name-is-too-short-and-still-can-enabled-by-persistent-people (I'd prefer --disable-safe-bitreader) > --enable-memalign-hack emulate memalign, interferes with memory debuggers > --disable-everything disable all components listed below > --disable-encoder=NAME disable encoder NAME > @@ -971,6 +972,7 @@ CONFIG_LIST=" > rdft > rtpdec > runtime_cpudetect > + safe_bitstream_reader > shared > sinewin > small > diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h > index d2ae345..07f6084 100644 > --- a/libavcodec/get_bits.h > +++ b/libavcodec/get_bits.h > @@ -48,6 +48,23 @@ > # endif > #endif > > +/* > + * Save bitstream reading: > + * optionally, the get_bits API can check to ensure that we > + * don't read past input buffer boundaries. This is protected > + * with CONFIG_SAFE_BITSTREAM_READER at the global level, and > + * then below that with UNCHECKED_BITSTREAM_READER at the per- > + * decoder level. This means that decoders that check internally > + * can "#define UNCHECKED_BITSTREAM_READER 1" to disable > + * overread checks. > + * Boundary checking causes a minor performance penalty so for > + * applications that won't want/need this, it can be disabled > + * globally using "#define CONFIG_SAFE_BITSTREAM_READER 0". > + */ > +#ifndef UNCHECKED_BITSTREAM_READER > +#define UNCHECKED_BITSTREAM_READER !CONFIG_SAFE_BITSTREAM_READER > +#endif > + > /* bit input */ > /* buffer, buffer_end and size_in_bits must be present and used by every > reader */ > typedef struct GetBitContext { > @@ -55,7 +72,7 @@ typedef struct GetBitContext { > #ifdef ALT_BITSTREAM_READER > int index; > #elif defined A32_BITSTREAM_READER > - uint32_t *buffer_ptr; > + const uint32_t *buffer_ptr; making buffer const is a separate thing IMO > uint32_t cache0; > uint32_t cache1; > int bit_count; > @@ -144,7 +161,11 @@ for examples see get_bits, show_bits, skip_bits, get_vlc > # endif > > // FIXME name? > +#if UNCHECKED_BITSTREAM_READER > # define SKIP_COUNTER(name, gb, num) name##_index += (num) > +#else > +# define SKIP_COUNTER(name, gb, num) name##_index = > FFMIN((gb)->size_in_bits, name##_index + num) > +#endif > > # define SKIP_BITS(name, gb, num) do { \ > SKIP_CACHE(name, gb, num); \ > @@ -171,7 +192,11 @@ static inline int get_bits_count(const GetBitContext *s){ > } > > static inline void skip_bits_long(GetBitContext *s, int n){ > +#if UNCHECKED_BITSTREAM_READER > s->index += n; > +#else > + s->index = FFMIN(s->size_in_bits, s->index + n); > +#endif > } > > #elif defined A32_BITSTREAM_READER > @@ -182,7 +207,7 @@ static inline void skip_bits_long(GetBitContext *s, int > n){ > int name##_bit_count = (gb)->bit_count; \ > uint32_t name##_cache0 = (gb)->cache0; \ > uint32_t name##_cache1 = (gb)->cache1; \ > - uint32_t *name##_buffer_ptr = (gb)->buffer_ptr > + const uint32_t *name##_buffer_ptr = (gb)->buffer_ptr > > # define CLOSE_READER(name, gb) do { \ > (gb)->bit_count = name##_bit_count; \ > @@ -196,7 +221,9 @@ static inline void skip_bits_long(GetBitContext *s, int > n){ > const uint32_t next = av_be2ne32(*name##_buffer_ptr); \ > name##_cache0 |= NEG_USR32(next, name##_bit_count); \ > name##_cache1 |= next << name##_bit_count; \ > - name##_buffer_ptr++; \ > + if (UNCHECKED_BITSTREAM_READER || \ > + name##_buffer_ptr < (const uint32_t *)(gb)->buffer_end) \ > + name##_buffer_ptr++; \ > name##_bit_count -= 32; \ > } \ > } while (0) > @@ -224,13 +251,18 @@ static inline void skip_bits_long(GetBitContext *s, int > n){ > # define GET_CACHE(name, gb) name##_cache0 > > static inline int get_bits_count(const GetBitContext *s) { > - return ((uint8_t*)s->buffer_ptr - s->buffer)*8 - 32 + s->bit_count; > + return ((const uint8_t*)s->buffer_ptr - s->buffer)*8 - 32 + s->bit_count; > } > > static inline void skip_bits_long(GetBitContext *s, int n){ > OPEN_READER(re, s); > re_bit_count += n; > +#if UNCHECKED_BITSTREAM_READER > re_buffer_ptr += re_bit_count>>5; > +#else > + re_buffer_ptr = FFMIN(re_buffer_ptr + (re_bit_count >> 5), > + (const uint32_t *) s->buffer_end); probably it's fine but I still have an uneasy feeling about pointers in FFMIN() > +#endif > re_bit_count &= 31; > re_cache0 = av_be2ne32(re_buffer_ptr[-1]) << re_bit_count; > re_cache1 = 0; > @@ -311,7 +343,8 @@ static inline unsigned int get_bits1(GetBitContext *s){ > result <<= index & 7; > result >>= 8 - 1; > #endif > - index++; > + if (UNCHECKED_BITSTREAM_READER || s->index < s->size_in_bits) > + index++; > s->index = index; > > return result; > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c > index 6f3a6b2..7be6445 100644 > --- a/libavcodec/wmavoice.c > +++ b/libavcodec/wmavoice.c > @@ -25,6 +25,8 @@ > * @author Ronald S. Bultje <[email protected]> > */ > > +#define UNCHECKED_BITSTREAM_READER 1 > + > #include <math.h> > #include "avcodec.h" > #include "get_bits.h" > -- good idea in general _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
