[FFmpeg-devel] [PATCH] Fix uninitialized reads on malformed ogg files.

[Reimar Döffinger]

On Thu, Mar 08, 2012 at 12:27:35PM -0800, Dale Curtis wrote:
> On Thu, Mar 8, 2012 at 12:04 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de>wrote:
> 
> > On Thu, Mar 08, 2012 at 05:51:59AM +0100, Michael Niedermayer wrote:
> > > On Wed, Mar 07, 2012 at 02:26:58PM -0800, dalecurtis at chromium.org wrote:
> > > > From: Dale Curtis <dalecurtis at chromium.org>
> > > >
> > > > The ogg decoder wasn't padding the input buffer with the appropriate
> > > > FF_INPUT_BUFFER_PADDING_SIZE bytes. Which led to uninitialized reads in
> > > > various pieces of parsing code when they thought they had more data
> > than
> > > > they actually did.
> > >
> > > patch looks good to me
> > > reimar ?
> >
> > None of the code I am aware of is supposed to require padding, so
> > I'd really like to hear what code that is. Bugs tend to come in bunches,
> > so I'd expect that code to be buggy in more ways.
> > I also have some doubts that that add can never cause integer overflows.
> >
> 
> The adds are so I could do the memset w/o using
> FFMIN(FF_INPUT_BUFFER_PADDING_SIZE, os->bufsize - os->bufpos). If you don't
> think the buffers need padding, I'm happy to switch the patch around.
> 
> Specifically the code we found issue with was downstream in
> oggparsetheora.c. If there aren't enough bits left for header parsing, the
> values will load uninitialized data:

Ok, I guess it is strictly seen necessary then.
However that code should at least be sanity-checking the buffer
size to be reasonable (e.g. the header is never less than around
27 bytes, so we shouldn't even start to try anything smaller
than that).
That would probably already fix the case you were seeing,
even if it is still not fully correct.