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

[Laurent Aimar]

Hi,

On Fri, Sep 09, 2011 at 01:57:38AM +0200, Michael Niedermayer wrote:
> >  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.


> > @@ -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