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

[Michael Niedermayer]

Hi

On Fri, Sep 09, 2011 at 09:03:13PM +0200, Laurent Aimar wrote:
> Hi,
> 
> On Fri, Sep 09, 2011 at 08:16:31AM +0200, Laurent Aimar wrote:
> > On Fri, Sep 09, 2011 at 02:26:53AM +0200, Michael Niedermayer wrote:
> > > On Fri, Sep 09, 2011 at 02:05:19AM +0200, Laurent Aimar wrote:
> > > [...]
> > > > > > @@ -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.

mpegaudio, to be able to switch buffers copies part of the new into
the old so it can saftely read over the end of the old and then
just switch. This intended overreading is likely to conflict with most
readers that prevent overread
The solution is simply not to use a checking reader as mpegaudio
needs to check for the switching to work already

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/249a2d28/attachment.asc>