"Ronald S. Bultje" <[email protected]> writes:
> 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.
> ---
We need more benchmarks so we know just what the impact of this is.
> 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)
> --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
If the idea is to have this on by default, you need an
"enable safe_bitstream_reader" line somewhere before the flags are parsed.
> 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:
"Safe"
> + * 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;
Unrelated?
> 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
> }
These need to ensure the addition doesn't overflow for a ridiculous
value of n.
skip_bits_long() is rare enough that perhaps it's OK to always check
it. It is also more likely to overflow.
> #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;
> }
Unrelated?
> 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);
> +#endif
This is incorrect. If the addition here overflows, you've lost already.
> 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;
This one looks correct.
> 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
Is this the only safe decoder?
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
