[FFmpeg-devel] [PATCH] Checked get_bits.h functions to prevent overread

[Michael Niedermayer]

Hi

On Fri, Sep 09, 2011 at 01:05:54AM +0200, Laurent Aimar wrote:
> 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 :)

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.

[...]
> @@ -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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110909/354ff3f8/attachment.asc>