Discussion:
[PATCH] Checked get_bits.h functions to prevent overread
(too old to reply)
Laurent Aimar
2011-09-08 23:05:54 UTC
Permalink
Hi,

After trying some fuzzing on libavcodec, it seems that a lot of decoders
does not check (or not enough) for buffer overread which can lead for some
to a segfault.

I attached a patch that make get_bits.h function checked for overread by
default but let safe decoders disabling the checks at compilation time by
defining UNCHECK_BITSTREAM_READER before including get_bits.h.
If such patch would be including, I would gladly provide a patch
adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.

I haven't yet benchmark the performance loss but will do so.

One decoder breaks with this patch: mpegaudio. It seems to do weird things
with two get bit context and switching them while decoding. I will try to
have a look at it (unless someone would volunteer to explain me what it is
doing :)

Also, I haven't implemented the checks for A32_BITSTREAM_READER. But I am not
sure when (or even if) this reader is used.

Regards,
--
fenrir
Ronald S. Bultje
2011-09-08 23:12:53 UTC
Permalink
Hi,
 I attached a patch that make get_bits.h function checked for overread by
default but let safe decoders disabling the checks at compilation time by
defining UNCHECK_BITSTREAM_READER before including get_bits.h.
 If such patch would be including, I would gladly provide a patch
adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.
I wanted to do this in exactly the same way but gut buried in some
other work. +1 from me.
+#ifndef UNCHECK_BITSTREAM_READER
+# warn "Checked bistream reader unimplemented"
+#endif
biTstream.

Ronald
Laurent Aimar
2011-09-08 23:15:02 UTC
Permalink
Post by Ronald S. Bultje
+#ifndef UNCHECK_BITSTREAM_READER
+# warn "Checked bistream reader unimplemented"
+#endif
biTstream.
Locally fixed, thanks.
--
fenrir
Laurent Aimar
2011-09-09 00:05:19 UTC
Permalink
Hi,
Post by Laurent Aimar
One decoder breaks with this patch: mpegaudio. It seems to do weird things
with two get bit context and switching them while decoding. I will try to
have a look at it (unless someone would volunteer to explain me what it is
doing :)
well, iam not sure i remember all details but
mp3 frames are made of 2 parts the first part is just a bunch of
simply readable bitstream. The second part has to be appended to a
buffer out of which the actual decoding happens, that is possibly
from parts of past mp3 frames. This allows cbr mp3 to have somewhat
variable internal framesize.
What our code now does is it tries to avoid copying this bitstream
as a whole into the buffer (saving some memcpy cpu cycles) and thus
it has to switch the buffers
combining this with pretty good error recovery capability and
support for a few broken mp3 encoders results in the mess we have.
Ok, thanks. I will have a closer look.
Post by Laurent Aimar
@@ -311,7 +331,12 @@ static inline unsigned int get_bits1(GetBitContext *s){
result <<= index & 7;
result >>= 8 - 1;
#endif
+#ifndef UNCHECK_BITSTREAM_READER
+ if (index < s->size_in_bits)
+ index++;
+#else
index++;
+#endif
i think this will break error detection of some files with some
decoders because they detect it by an overread having happened
also it might lead to infinite loops or other unexpected things
as some decoders depend on them
hitting zero padding after the buffer which is guranteed to lead to
vlc decoding failures for them as they have no valid all 0 vlc code
I see. A simple way could be to simply add 8 * PADDING_SIZE to the check.
I will add that locally.
--
fenrir
Laurent Aimar
2011-09-09 06:16:31 UTC
Permalink
Hi,
[...]
Post by Laurent Aimar
Post by Laurent Aimar
@@ -311,7 +331,12 @@ static inline unsigned int get_bits1(GetBitContext *s){
result <<= index & 7;
result >>= 8 - 1;
#endif
+#ifndef UNCHECK_BITSTREAM_READER
+ if (index < s->size_in_bits)
+ index++;
+#else
index++;
+#endif
i think this will break error detection of some files with some
decoders because they detect it by an overread having happened
also it might lead to infinite loops or other unexpected things
as some decoders depend on them
hitting zero padding after the buffer which is guranteed to lead to
vlc decoding failures for them as they have no valid all 0 vlc code
I see. A simple way could be to simply add 8 * PADDING_SIZE to the check.
I will add that locally.
Iam not sure this is enough
the problematic case iam thinking of is a decoder that works with
slices, so the guranteed 0 padding would be farther away if the
current slice is not the last. mpeg1/2 should be examples of this
case
maybe just returning 0 after size+something would work better
I wanted to avoid the check while loading the cache, but you're right,
it's probably the simplest solution to avoid the issue you mentionned.
I will provide a patch to do that instead.
--
fenrir
Laurent Aimar
2011-09-09 19:03:13 UTC
Permalink
Hi,
Post by Laurent Aimar
[...]
Post by Laurent Aimar
Post by Laurent Aimar
@@ -311,7 +331,12 @@ static inline unsigned int get_bits1(GetBitContext *s){
result <<= index & 7;
result >>= 8 - 1;
#endif
+#ifndef UNCHECK_BITSTREAM_READER
+ if (index < s->size_in_bits)
+ index++;
+#else
index++;
+#endif
i think this will break error detection of some files with some
decoders because they detect it by an overread having happened
also it might lead to infinite loops or other unexpected things
as some decoders depend on them
hitting zero padding after the buffer which is guranteed to lead to
vlc decoding failures for them as they have no valid all 0 vlc code
I see. A simple way could be to simply add 8 * PADDING_SIZE to the check.
I will add that locally.
Iam not sure this is enough
the problematic case iam thinking of is a decoder that works with
slices, so the guranteed 0 padding would be farther away if the
current slice is not the last. mpeg1/2 should be examples of this
case
maybe just returning 0 after size+something would work better
I wanted to avoid the check while loading the cache, but you're right,
it's probably the simplest solution to avoid the issue you mentionned.
I will provide a patch to do that instead.
New patch attached and it's a bit simpler.

Now out of bound index is checked when reading the data and the value 0 is
returned in such cases. get_bits_count() will then return the real number
of bits read.

I still have an issue with mpegaudio though. I will try to fix it first and
then I will do some benchmarks.

Regards,
--
fenrir
Alex Converse
2011-09-09 19:09:48 UTC
Permalink
Hi,
[...]
Post by Laurent Aimar
@@ -311,7 +331,12 @@ static inline unsigned int get_bits1(GetBitContext *s){
     result <<= index & 7;
     result >>= 8 - 1;
 #endif
+#ifndef UNCHECK_BITSTREAM_READER
+    if (index < s->size_in_bits)
+        index++;
+#else
     index++;
+#endif
i think this will break error detection of some files with some
decoders because they detect it by an overread having happened
also it might lead to infinite loops or other unexpected things
as some decoders depend on them
hitting zero padding after the buffer which is guranteed to lead to
vlc decoding failures for them as they have no valid all 0 vlc code
 I see. A simple way could be to simply add 8 * PADDING_SIZE to the check.
I will add that locally.
Iam not sure this is enough
the problematic case iam thinking of is a decoder that works with
slices, so the guranteed 0 padding would be farther away if the
current slice is not the last. mpeg1/2 should be examples of this
case
maybe just returning 0 after size+something would work better
 I wanted to avoid the check while loading the cache, but you're right,
it's probably the simplest solution to avoid the issue you mentionned.
 I will provide a patch to do that instead.
 New patch attached and it's a bit simpler.
 Now out of bound index is checked when reading the data and the value 0 is
returned in such cases. get_bits_count() will then return the real number
of bits read.
 I still have an issue with mpegaudio though. I will try to fix it first and
then I will do some benchmarks.
You also broke with the following fate tests:

make: *** [fate-cljr] Error 1
make: *** [fate-creatureshock-avs] Error 1
make: *** [fate-ea-tqi-adpcm] Error 1
make: *** [fate-indeo2] Error 1
make: *** [fate-lossless-tta] Error 136
make: *** [fate-motionpixels] Error 1
make: *** [fate-wc3movie-xan] Error 139
make: *** [fate-ac3-5.1] Error 1
make: *** [fate-mp3-float-conf-compl] Error 1
make: *** [fate-mp3-float-conf-si] Error 1
make: *** [fate-vsynth1-ffv1] Error 1
make: *** [fate-vsynth2-ffv1] Error 1
Justin Ruggles
2011-09-09 19:33:39 UTC
Permalink
Post by Alex Converse
Post by Laurent Aimar
Hi,
Post by Laurent Aimar
[...]
Post by Laurent Aimar
Post by Laurent Aimar
@@ -311,7 +331,12 @@ static inline unsigned int get_bits1(GetBitContext *s){
result <<= index & 7;
result >>= 8 - 1;
#endif
+#ifndef UNCHECK_BITSTREAM_READER
+ if (index < s->size_in_bits)
+ index++;
+#else
index++;
+#endif
i think this will break error detection of some files with some
decoders because they detect it by an overread having happened
also it might lead to infinite loops or other unexpected things
as some decoders depend on them
hitting zero padding after the buffer which is guranteed to lead to
vlc decoding failures for them as they have no valid all 0 vlc code
I see. A simple way could be to simply add 8 * PADDING_SIZE to the check.
I will add that locally.
Iam not sure this is enough
the problematic case iam thinking of is a decoder that works with
slices, so the guranteed 0 padding would be farther away if the
current slice is not the last. mpeg1/2 should be examples of this
case
maybe just returning 0 after size+something would work better
I wanted to avoid the check while loading the cache, but you're right,
it's probably the simplest solution to avoid the issue you mentionned.
I will provide a patch to do that instead.
New patch attached and it's a bit simpler.
Now out of bound index is checked when reading the data and the value 0 is
returned in such cases. get_bits_count() will then return the real number
of bits read.
I still have an issue with mpegaudio though. I will try to fix it first and
then I will do some benchmarks.
make: *** [fate-cljr] Error 1
make: *** [fate-creatureshock-avs] Error 1
make: *** [fate-ea-tqi-adpcm] Error 1
make: *** [fate-indeo2] Error 1
make: *** [fate-lossless-tta] Error 136
make: *** [fate-motionpixels] Error 1
make: *** [fate-wc3movie-xan] Error 139
make: *** [fate-ac3-5.1] Error 1
damn. I thought I fixed all the ac3 fate samples, but apparently I
missed this one. It has an incomplete frame. The decoder still doesn't
read past the end of the buffer, just past the end of the bitstream reader.
Post by Alex Converse
make: *** [fate-mp3-float-conf-compl] Error 1
make: *** [fate-mp3-float-conf-si] Error 1
make: *** [fate-vsynth1-ffv1] Error 1
make: *** [fate-vsynth2-ffv1] Error 1
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel
Alex Converse
2011-09-09 00:38:37 UTC
Permalink
@@ -172,10 +184,18 @@ static inline int get_bits_count(const GetBitContext *s){
static inline void skip_bits_long(GetBitContext *s, int n){
s->index += n;
+#ifndef UNCHECK_BITSTREAM_READER
+ if (s->index > s->size_in_bits)
+ s->index = s->size_in_bits;
+#endif
}
#elif defined A32_BITSTREAM_READER
+#ifndef UNCHECK_BITSTREAM_READER
+# warn "Checked bistream reader unimplemented"
+#endif
+
# define MIN_CACHE_BITS 32
# define OPEN_READER(name, gb) \
Please use #warning instead of #warn, clang-2.9 and gcc-4.4 both
happily accept #warning but barf on #warn

I agree with mn that some decoders check if get_bits_left() has gone negative.

At least mpeg4 (possibly others) skips backwards at times and I'm not
sure if that should be taken into consideration wrt position clipping.
Diego Biurrun
2011-09-09 07:55:47 UTC
Permalink
Post by Alex Converse
@@ -172,10 +184,18 @@ static inline int get_bits_count(const GetBitContext *s){
static inline void skip_bits_long(GetBitContext *s, int n){
s->index += n;
+#ifndef UNCHECK_BITSTREAM_READER
+ if (s->index > s->size_in_bits)
+ s->index = s->size_in_bits;
+#endif
}
#elif defined A32_BITSTREAM_READER
+#ifndef UNCHECK_BITSTREAM_READER
+# warn "Checked bistream reader unimplemented"
+#endif
+
# define MIN_CACHE_BITS 32
# define OPEN_READER(name, gb) \
Please use #warning instead of #warn, clang-2.9 and gcc-4.4 both
happily accept #warning but barf on #warn
IIRC neither is standard C, so likely both should be avoided.

Diego
Laurent Aimar
2011-09-09 09:00:40 UTC
Permalink
Post by Diego Biurrun
Post by Alex Converse
@@ -172,10 +184,18 @@ static inline int get_bits_count(const GetBitContext *s){
static inline void skip_bits_long(GetBitContext *s, int n){
s->index += n;
+#ifndef UNCHECK_BITSTREAM_READER
+ if (s->index > s->size_in_bits)
+ s->index = s->size_in_bits;
+#endif
}
#elif defined A32_BITSTREAM_READER
+#ifndef UNCHECK_BITSTREAM_READER
+# warn "Checked bistream reader unimplemented"
+#endif
+
# define MIN_CACHE_BITS 32
# define OPEN_READER(name, gb) \
Please use #warning instead of #warn, clang-2.9 and gcc-4.4 both
happily accept #warning but barf on #warn
IIRC neither is standard C, so likely both should be avoided.
Ok, I will remove it.
--
fenrir
Alex Converse
2011-09-09 01:06:10 UTC
Permalink
Hi,
 After trying some fuzzing on libavcodec, it seems that a lot of decoders
does not check (or not enough) for buffer overread which can lead for some
to a segfault.
 I attached a patch that make get_bits.h function checked for overread by
default but let safe decoders disabling the checks at compilation time by
defining UNCHECK_BITSTREAM_READER before including get_bits.h.
 If such patch would be including, I would gladly provide a patch
adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.
I haven't yet benchmark the performance loss but will do so.
 One decoder breaks with this patch: mpegaudio. It seems to do weird things
with two get bit context and switching them while decoding. I will try to
have a look at it (unless someone would volunteer to explain me what it is
doing :)
Also, I haven't implemented the checks for A32_BITSTREAM_READER. But I am not
sure when (or even if) this reader is used.
Regards,
I've put this in a separate post because this is probably the more
contentious part...

I think this should be opt-in instead of opt-out. i.e. the bad
decoders (even if that is most of them) should have the ugly boiler
plate, not the good ones.

And in previous discussions the prevailing opinion has been that on
decoders for more modern formats, that make a good attempt to respect
buffer sizes, not to opt them into something like this even if they
are not perfect and instead the individual flaws have been fixed.

This is a very expensive form of error resilience and there are a lot
of use cases where people just don't care. They will tolerate the SEGV
on the occasional bad file if it means they can decode a good with
reasonable speed.

i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.

If this isn't the case anymore I'm wondering what has changed?

Is there a proposed initial opt-out list?
Ronald S. Bultje
2011-09-09 01:27:20 UTC
Permalink
Hi,
Post by Alex Converse
This is a very expensive form of error resilience and there are a lot
of use cases where people just don't care. They will tolerate the SEGV
on the occasional bad file if it means they can decode a good with
reasonable speed.
We can add a configure flag to always set (or unset) this macro for
people who like the risk. Adding it to avconfig.h has the same effect
as adding it to each codec.

Ronald
Måns Rullgård
2011-09-09 10:04:59 UTC
Permalink
Post by Alex Converse
Hi,
 After trying some fuzzing on libavcodec, it seems that a lot of decoders
does not check (or not enough) for buffer overread which can lead for some
to a segfault.
 I attached a patch that make get_bits.h function checked for overread by
default but let safe decoders disabling the checks at compilation time by
defining UNCHECK_BITSTREAM_READER before including get_bits.h.
 If such patch would be including, I would gladly provide a patch
adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.
I haven't yet benchmark the performance loss but will do so.
 One decoder breaks with this patch: mpegaudio. It seems to do weird things
with two get bit context and switching them while decoding. I will try to
have a look at it (unless someone would volunteer to explain me what it is
doing :)
Also, I haven't implemented the checks for A32_BITSTREAM_READER. But I am not
sure when (or even if) this reader is used.
Regards,
I've put this in a separate post because this is probably the more
contentious part...
I think this should be opt-in instead of opt-out. i.e. the bad
decoders (even if that is most of them) should have the ugly boiler
plate, not the good ones.
And in previous discussions the prevailing opinion has been that on
decoders for more modern formats, that make a good attempt to respect
buffer sizes, not to opt them into something like this even if they
are not perfect and instead the individual flaws have been fixed.
This is a very expensive form of error resilience and there are a lot
of use cases where people just don't care. They will tolerate the SEGV
on the occasional bad file if it means they can decode a good with
reasonable speed.
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree. This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
--
Måns Rullgård
***@mansr.com
Janne Grunau
2011-09-09 13:09:41 UTC
Permalink
Post by Måns Rullgård
Post by Alex Converse
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree. This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
could we please wait for benchmarks before making bold statements. I start
with one, currently on 3G without higher resolution samples.

./avconv -benchmark -v 0 -i ~/Downloads/cathedral-beta2-400extra-crop-avc.mp4 -f null /dev/null
Stream #0.2(eng): Video: h264 (Main), yuv420p, 640x352, 401 kb/s, 23.98 fps
Stream #0.3(eng): Audio: aac, 44100 Hz, stereo, s16, 95 kb/s

current master: bench: utime=20.569s maxrss=9628kB
witt Laurent's patch: bench: utime=20.835s maxrss=9552kB

fasted out of three runs.

Slowdown of 1.3% far away from annihilating performance.

Janne
Alex Converse
2011-09-09 15:29:26 UTC
Permalink
Post by Janne Grunau
Post by Alex Converse
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree.  This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
could we please wait for benchmarks before making bold statements. I start
with one, currently on 3G without higher resolution samples.
./avconv -benchmark -v 0 -i ~/Downloads/cathedral-beta2-400extra-crop-avc.mp4 -f null /dev/null
Stream #0.2(eng): Video: h264 (Main), yuv420p, 640x352, 401 kb/s, 23.98 fps
Stream #0.3(eng): Audio: aac, 44100 Hz, stereo, s16, 95 kb/s
current master:       bench: utime=20.569s maxrss=9628kB
witt Laurent's patch: bench: utime=20.835s maxrss=9552kB
fasted out of three runs.
Slowdown of 1.3% far away from annihilating performance.
If a proposal saved 1.3% overall we'd agree to let it contort the code
in all sorts of hideous ways.
Jason Garrett-Glaser
2011-09-09 17:47:01 UTC
Permalink
Post by Alex Converse
Post by Janne Grunau
Post by Alex Converse
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree.  This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
could we please wait for benchmarks before making bold statements. I start
with one, currently on 3G without higher resolution samples.
./avconv -benchmark -v 0 -i ~/Downloads/cathedral-beta2-400extra-crop-avc.mp4 -f null /dev/null
Stream #0.2(eng): Video: h264 (Main), yuv420p, 640x352, 401 kb/s, 23.98 fps
Stream #0.3(eng): Audio: aac, 44100 Hz, stereo, s16, 95 kb/s
current master:       bench: utime=20.569s maxrss=9628kB
witt Laurent's patch: bench: utime=20.835s maxrss=9552kB
fasted out of three runs.
Slowdown of 1.3% far away from annihilating performance.
If a proposal saved 1.3% overall we'd agree to let it contort the code
in all sorts of hideous ways.
If this patch is committed, I promise to speed up H.264 by 1.3%.

Jason
Alex Converse
2011-09-09 18:31:54 UTC
Permalink
Post by Jason Garrett-Glaser
Post by Alex Converse
Post by Janne Grunau
Post by Alex Converse
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree.  This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
could we please wait for benchmarks before making bold statements. I start
with one, currently on 3G without higher resolution samples.
./avconv -benchmark -v 0 -i ~/Downloads/cathedral-beta2-400extra-crop-avc.mp4 -f null /dev/null
Stream #0.2(eng): Video: h264 (Main), yuv420p, 640x352, 401 kb/s, 23.98 fps
Stream #0.3(eng): Audio: aac, 44100 Hz, stereo, s16, 95 kb/s
current master:       bench: utime=20.569s maxrss=9628kB
witt Laurent's patch: bench: utime=20.835s maxrss=9552kB
fasted out of three runs.
Slowdown of 1.3% far away from annihilating performance.
If a proposal saved 1.3% overall we'd agree to let it contort the code
in all sorts of hideous ways.
If this patch is committed, I promise to speed up H.264 by 1.3%.
okay with me I guess

(Which does NOT mean the patch itself is OK, fate still needs to be fixed)

--Alex
Måns Rullgård
2011-09-09 17:17:55 UTC
Permalink
Post by Janne Grunau
Post by Måns Rullgård
Post by Alex Converse
i.e. Please turn this feature on for the Indeos and the Sorensons and
the like, but let's fix the individual bugs in the H.264s and VP8s.
Turning this on for them is overkill.
Agree. This will annihilate performance, so if we're going to do this,
we might as well stop altogether.
could we please wait for benchmarks before making bold statements. I start
with one, currently on 3G without higher resolution samples.
./avconv -benchmark -v 0 -i ~/Downloads/cathedral-beta2-400extra-crop-avc.mp4 -f null /dev/null
Stream #0.2(eng): Video: h264 (Main), yuv420p, 640x352, 401 kb/s, 23.98 fps
Stream #0.3(eng): Audio: aac, 44100 Hz, stereo, s16, 95 kb/s
current master: bench: utime=20.569s maxrss=9628kB
witt Laurent's patch: bench: utime=20.835s maxrss=9552kB
fasted out of three runs.
Does that clip use CABAC? CABAC doesn't even use the get_bits interface
for the heavy lifting.
--
Måns Rullgård
***@mansr.com
Kostya Shishkov
2011-09-09 05:32:19 UTC
Permalink
Post by Laurent Aimar
Hi,
After trying some fuzzing on libavcodec, it seems that a lot of decoders
does not check (or not enough) for buffer overread which can lead for some
to a segfault.
I attached a patch that make get_bits.h function checked for overread by
default but let safe decoders disabling the checks at compilation time by
defining UNCHECK_BITSTREAM_READER before including get_bits.h.
If such patch would be including, I would gladly provide a patch
adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.
I haven't yet benchmark the performance loss but will do so.
One decoder breaks with this patch: mpegaudio. It seems to do weird things
with two get bit context and switching them while decoding. I will try to
have a look at it (unless someone would volunteer to explain me what it is
doing :)
It's easy: MP3 is actually VBR disguised as CBR, so frame data (except for the
header) actually forms an independent bitstream and frame header provides an
offset in it from which point data should be read (so actual frame data may
start in payload from previous frame). Lavc MP3 decoder reads the data in
previous frame (saved data) with one context and then switches to the current
frame data when needed IIRC.
Continue reading on narkive:
Loading...