Discussion:
[RFC] overread-safe get_bits API
(too old to reply)
Ronald S. Bultje
2011-12-12 15:35:44 UTC
Permalink
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
Kostya Shishkov
2011-12-12 15:45:38 UTC
Permalink
Post by Ronald S. Bultje
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
Ronald S. Bultje
2011-12-15 04:28:15 UTC
Permalink
Hi,

On Mon, Dec 12, 2011 at 7:45 AM, Kostya Shishkov
On Mon, Dec 12, 2011 at 07:35:44AM -0800, Ronald S. Bultje wrote:> ---
a/configure
+++ b/configure
--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)
Somewhat funny, that was the original name I gave the option but other
options in get_bits.h were called *_BITSTREAM_READER, so I decided to stay
with that theme. I'll change it back if you prefer.
@@ -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
Well, yes and no. I agree I can do it before (which I will), but I can't do
it after since the compiler barfs because we're comparing incompatible
types (const uint32_t * to uint32_t *).
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()
I thought it was cute. :-). It really works.

Ronald
Laurent Aimar
2011-12-12 15:54:06 UTC
Permalink
Post by Ronald S. Bultje
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
Måns Rullgård
2011-12-12 16:16:13 UTC
Permalink
Post by Laurent Aimar
Post by Ronald S. Bultje
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
Laurent Aimar
2011-12-12 16:24:56 UTC
Permalink
Post by Laurent Aimar
Post by Ronald S. Bultje
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
Ronald S. Bultje
2011-12-12 16:35:16 UTC
Permalink
Hi,
Post by Laurent Aimar
I think that with your patch, get_bits_count() will never be negative and this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we should
document that as undefined behaviour, IMO.

Ronald
Laurent Aimar
2011-12-12 16:50:12 UTC
Permalink
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be negative and this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we should
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left() to
return < 0 without creating any issue if the user of the get bits function
ensure that the 'overread' does not exceed the mandatory padding there is
at the end of each buffer.

Now, it can of course be decided to make get_bits_left() returning < 0
a non valid use case, but it's a change from what I think was previously
understood (at least that's my impression reading various decoder codes).
It will probably need a lot of code review before the change can be done
safely.

Regards,
--
fenrir
Ronald S. Bultje
2011-12-12 16:55:18 UTC
Permalink
Hi,
Post by Laurent Aimar
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be
negative and
Post by Laurent Aimar
Post by Laurent Aimar
this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we should
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left() to
return < 0 without creating any issue if the user of the get bits function
ensure that the 'overread' does not exceed the mandatory padding there is
at the end of each buffer.
Now, it can of course be decided to make get_bits_left() returning < 0
a non valid use case, but it's a change from what I think was previously
understood (at least that's my impression reading various decoder codes).
It will probably need a lot of code review before the change can be done
safely.
I agree, but I do feel this change has lowest effect on performance while
still covering all of get_bits.

Want to help review?

Ronald
Alex Converse
2011-12-12 18:50:29 UTC
Permalink
Post by Laurent Aimar
Hi,
Post by Laurent Aimar
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be
negative and
Post by Laurent Aimar
Post by Laurent Aimar
this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we
should
Post by Laurent Aimar
Post by Laurent Aimar
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left() to
return < 0 without creating any issue if the user of the get bits
function
Post by Laurent Aimar
ensure that the 'overread' does not exceed the mandatory padding there is
at the end of each buffer.
Now, it can of course be decided to make get_bits_left() returning < 0
a non valid use case, but it's a change from what I think was previously
understood (at least that's my impression reading various decoder codes).
It will probably need a lot of code review before the change can be done
safely.
I agree, but I do feel this change has lowest effect on performance while
still covering all of get_bits.
Want to help review?
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);
If we make this FFMIN(s->size_in_bits + 1, s->index + n) can't we get the
best of both worlds for just one additional add?
+#endif
}

--Alex
Ronald S. Bultje
2011-12-12 20:27:54 UTC
Permalink
Hi,
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be
negative and
Post by Laurent Aimar
Post by Laurent Aimar
this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we
should
Post by Laurent Aimar
Post by Laurent Aimar
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left() to
return < 0 without creating any issue if the user of the get bits
function
Post by Laurent Aimar
ensure that the 'overread' does not exceed the mandatory padding there
is
Post by Laurent Aimar
at the end of each buffer.
Now, it can of course be decided to make get_bits_left() returning < 0
a non valid use case, but it's a change from what I think was previously
understood (at least that's my impression reading various decoder
codes).
Post by Laurent Aimar
It will probably need a lot of code review before the change can be done
safely.
I agree, but I do feel this change has lowest effect on performance while
still covering all of get_bits.
Want to help review?
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);
If we make this FFMIN(s->size_in_bits + 1, s->index + n) can't we get the
best of both worlds for just one additional add?
Yeah I think that's a good idea, better yet we can add
s->size_in_bits_plus1 to the context and use that to prevent the extra
instruction. Maybe we can even get rid of s->size_in_bits, but that's not
so important.

Ronald
Måns Rullgård
2011-12-12 21:26:14 UTC
Permalink
Post by Alex Converse
Post by Laurent Aimar
Hi,
Post by Laurent Aimar
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be
negative and
Post by Laurent Aimar
Post by Laurent Aimar
this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we
should
Post by Laurent Aimar
Post by Laurent Aimar
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left() to
return < 0 without creating any issue if the user of the get bits
function
Post by Laurent Aimar
ensure that the 'overread' does not exceed the mandatory padding there is
at the end of each buffer.
Now, it can of course be decided to make get_bits_left() returning < 0
a non valid use case, but it's a change from what I think was previously
understood (at least that's my impression reading various decoder codes).
It will probably need a lot of code review before the change can be done
safely.
I agree, but I do feel this change has lowest effect on performance while
still covering all of get_bits.
Want to help review?
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
}
If we make this FFMIN(s->size_in_bits + 1, s->index + n) can't we get the
best of both worlds for just one additional add?
That still doesn't protect against overflow from a very large n.
You must check for n <= s->size_in_bits - s->index.
--
Måns Rullgård
***@mansr.com
Ronald S. Bultje
2011-12-12 21:58:54 UTC
Permalink
Hi,
Post by Laurent Aimar
Post by Alex Converse
Post by Laurent Aimar
Hi,
Post by Laurent Aimar
Post by Laurent Aimar
Hi,
I think that with your patch, get_bits_count() will never be
negative and
Post by Laurent Aimar
Post by Laurent Aimar
this
will creates issues with some decoders.
We should fix those decoders. get_bits_left() < 0 is a bug and we
should
Post by Laurent Aimar
Post by Laurent Aimar
document that as undefined behaviour, IMO.
In itself it is not a bug. It is prefectly fine for get_bits_left()
to
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
return < 0 without creating any issue if the user of the get bits
function
Post by Laurent Aimar
ensure that the 'overread' does not exceed the mandatory padding
there is
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
at the end of each buffer.
Now, it can of course be decided to make get_bits_left() returning <
0
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
a non valid use case, but it's a change from what I think was
previously
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
understood (at least that's my impression reading various decoder
codes).
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
It will probably need a lot of code review before the change can be
done
Post by Alex Converse
Post by Laurent Aimar
Post by Laurent Aimar
safely.
I agree, but I do feel this change has lowest effect on performance
while
Post by Alex Converse
Post by Laurent Aimar
still covering all of get_bits.
Want to help review?
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
}
If we make this FFMIN(s->size_in_bits + 1, s->index + n) can't we get the
best of both worlds for just one additional add?
That still doesn't protect against overflow from a very large n.
You must check for n <= s->size_in_bits - s->index.
Yes I'll fix that in my next round - I'm a bit slow trying to catch up on
email and stuff.

Ronald
Måns Rullgård
2011-12-12 16:12:57 UTC
Permalink
Post by Ronald S. Bultje
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
Ronald S. Bultje
2011-12-15 04:31:21 UTC
Permalink
Hi,
@@ -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.
I wanted to keep it off by default until we have a good idea on performance
impact.
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -25,6 +25,8 @@
*/
+#define UNCHECKED_BITSTREAM_READER 1
Is this the only safe decoder?
No, it's just intended as an example. I believe we have a handful of
actually safe decoders. I don't know which they are tbh.

Ronald
Ronald S. Bultje
2011-12-15 17:49:24 UTC
Permalink
From: "Ronald S. Bultje" <***@gmail.com>

Based on work (for GCI) by Aneesh Dogra <***@gmail.com>, and
inspired by patch in Chromium by Chris Evans <***@chromium.org>.

When turned on, H264/CAVLC gets ~15% (CVPCMNL1_SVA_C.264) slower for
ultra-high-bitrate files, or ~2.5% (CVFI1_SVA_C.264) for lower-bitrate
files. Other codecs are affected to a lower extend because they are
less optimized; e.g., VC-1 slows down by less than 1% (all on x86).
The patch generated 3 extra instructions (cmp, cmovae and mov) per
call to get_bits().
---
libavcodec/get_bits.h | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
index dd56308..e408e06 100644
--- a/libavcodec/get_bits.h
+++ b/libavcodec/get_bits.h
@@ -77,7 +77,7 @@ typedef struct GetBitContext {
uint32_t cache1;
int bit_count;
#endif
- int size_in_bits;
+ int size_in_bits, size_in_bits_plus1;
} GetBitContext;

#define VLC_TYPE int16_t
@@ -165,8 +165,8 @@ for examples see get_bits, show_bits, skip_bits, get_vlc
# define SKIP_COUNTER(name, gb, num) name##_index += (num)
#else
# define SKIP_COUNTER(name, gb, num) name##_index = \
- (gb)->size_in_bits - name##_index <= (num) ? \
- (gb)->size_in_bits + 1 : name##_index + (num)
+ (gb)->size_in_bits_plus1 - name##_index < (num) ? \
+ (gb)->size_in_bits_plus1 : name##_index + (num)
#endif

# define SKIP_BITS(name, gb, num) do { \
@@ -197,8 +197,8 @@ static inline void skip_bits_long(GetBitContext *s, int n){
#if UNCHECKED_BITSTREAM_READER
s->index += n;
#else
- s->index = s->size_in_bits - s->index <= n ?
- s->size_in_bits + 1 : s->index + n;
+ s->index = s->size_in_bits_plus1 - s->index < n ?
+ s->size_in_bits_plus1 : s->index + n;
#endif
}

@@ -346,7 +346,7 @@ static inline unsigned int get_bits1(GetBitContext *s){
result <<= index & 7;
result >>= 8 - 1;
#endif
- if (UNCHECKED_BITSTREAM_READER || s->index <= s->size_in_bits)
+ if (UNCHECKED_BITSTREAM_READER || s->index < s->size_in_bits_plus1)
index++;
s->index = index;

@@ -427,6 +427,7 @@ static inline void init_get_bits(GetBitContext *s,

s->buffer = buffer;
s->size_in_bits = bit_size;
+ s->size_in_bits_plus1 = bit_size + 1;
s->buffer_end = buffer + buffer_size;
#ifdef ALT_BITSTREAM_READER
s->index = 0;
--
1.7.6
Ronald S. Bultje
2011-12-15 17:59:24 UTC
Permalink
From: "Ronald S. Bultje" <***@gmail.com>

Based on work (for GCI) by Aneesh Dogra <***@gmail.com>, and
inspired by patch in Chromium by Chris Evans <***@chromium.org>.

When turned on, H264/CAVLC gets ~15% (CVPCMNL1_SVA_C.264) slower for
ultra-high-bitrate files, or ~2.5% (CVFI1_SVA_C.264) for lower-bitrate
files. Other codecs are affected to a lesser extent because they are
less optimized; e.g., VC-1 slows down by less than 1% (all on x86).
The patch generated 3 extra instructions (cmp, cmovae and mov) per
call to get_bits().
---
configure | 2 ++
libavcodec/get_bits.h | 49 +++++++++++++++++++++++++++++++++++++++++++------
libavcodec/wmavoice.c | 2 ++
3 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index b3af2e9..056f409 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
@@ -976,6 +977,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 7980284..e408e06 100644
--- a/libavcodec/get_bits.h
+++ b/libavcodec/get_bits.h
@@ -48,6 +48,23 @@
# endif
#endif

+/*
+ * Safe 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,12 +72,12 @@ typedef struct GetBitContext {
#ifdef ALT_BITSTREAM_READER
int index;
#elif defined A32_BITSTREAM_READER
- uint32_t *buffer_ptr;
+ const uint32_t *buffer_ptr;
uint32_t cache0;
uint32_t cache1;
int bit_count;
#endif
- int size_in_bits;
+ int size_in_bits, size_in_bits_plus1;
} GetBitContext;

#define VLC_TYPE int16_t
@@ -144,7 +161,13 @@ 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 = \
+ (gb)->size_in_bits_plus1 - name##_index < (num) ? \
+ (gb)->size_in_bits_plus1 : name##_index + (num)
+#endif

# define SKIP_BITS(name, gb, num) do { \
SKIP_CACHE(name, gb, num); \
@@ -171,7 +194,12 @@ 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 = s->size_in_bits_plus1 - s->index < n ?
+ s->size_in_bits_plus1 : s->index + n;
+#endif
}

#elif defined A32_BITSTREAM_READER
@@ -182,7 +210,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 +224,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 +254,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);
+#endif
re_bit_count &= 31;
re_cache0 = av_be2ne32(re_buffer_ptr[-1]) << re_bit_count;
re_cache1 = 0;
@@ -311,7 +346,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_plus1)
+ index++;
s->index = index;

return result;
@@ -391,6 +427,7 @@ static inline void init_get_bits(GetBitContext *s,

s->buffer = buffer;
s->size_in_bits = bit_size;
+ s->size_in_bits_plus1 = bit_size + 1;
s->buffer_end = buffer + buffer_size;
#ifdef ALT_BITSTREAM_READER
s->index = 0;
diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 00e985d..8854e35 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -25,6 +25,8 @@
* @author Ronald S. Bultje <***@gmail.com>
*/

+#define UNCHECKED_BITSTREAM_READER 1
+
#include <math.h>
#include "avcodec.h"
#include "get_bits.h"
--
1.7.6
Loading...