On Thu, Mar 8, 2012 at 1:40 PM, Reimar Döffinger <[email protected]>wrote:
> 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 > > <[email protected]>wrote: > > > > > On Thu, Mar 08, 2012 at 05:51:59AM +0100, Michael Niedermayer wrote: > > > > On Wed, Mar 07, 2012 at 02:26:58PM -0800, [email protected]: > > > > > From: Dale Curtis <[email protected]> > > > > > > > > > > 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. > _______________________________________________ > ffmpeg-devel mailing list > [email protected] > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > That also fixes the issue and was the fix I discussed with Ronald prior to uploading this patch. The consensus was it's a little messy and seemed inconsistent with the padded buffer approach used elsewhere in FFmpeg. Here's what that code looks like: // Ensure we have enough bits left to read the rest of the header. if (get_bits_left(&gb) < 149 || (thp->version >= 0x030200 && get_bits_left(&gb) < 149 + 102) || (thp->version >= 0x030400 && get_bits_left(&gb) < 149 + 102 + 100) || (thp->version >= 0x304000 && get_bits_left(&gb) < 149 + 102 + 100 + 2)) return -1; - dale
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
