When using Xv to display an I420 image, a buffer overflow is triggered. http://lists.x.org/archives/xorg-driver-geode/2010-August/000938.html The problem is that gp_color_bitmap_to_screen_blt() does not consider pitch of the input image when it is doing the copy. It always assumes 2 bytes per pixel, as determined by the leading (but erroneous?) gp_set_bpp(16) call. For YUYV images there is no problem (2 bytes per pixel are copied). For I420 images, the image planes that this function has to copy are 1 byte per pixel, so the function ends up copying twice the amount of data, and reading beyond the end of the buffer. Fixing the function to consider image pitch (a parameter already available in the function) fixes the bug, but introduces a regression where another of my videos causes the geode driver to hang (while waiting for the blt engine to stop being busy). It is possibly related to the fact that this 2nd video is low-resolution (160x120). Analysis can be found in the thread linked to above. Version details are not too relevant as I've tracked down the buggy code in geode git HEAD, but anyway: OLPC XO-1 running OLPC OS 10.1.2 beta (based on Fedora 11) X server 1.6.4 RC1 xf86-video-geode git HEAD as of today Xorg.0.log: http://dev.laptop.org/~dsd/20100803/Xorg.0.log xorg.conf: http://dev.laptop.org/~dsd/20100803/xorg.conf Video that I'm using to trigger the crash relatively easily: http://proxy-48.dailymotion.com/video/791/905/21509197%3aogg_theora_vorbis%3a255.ogg?auth=1280882838-c862f9f6c8fd3de117a4e04b3676246c&cache=0 Low resolution video that causes a hang when the obvious fix is applied: http://dev.laptop.org/~dsd/20100803/video.ogv
Created attachment 37578 [details] [review] fix Fixed it. The set_bpp call is valid. We're BLTing an image from system memory to video memory. We aren't displaying it on the screen; the screen output BPP is irrelevant. As we're copying without transforming the data in any way, the BLT output BPP needs to be the same as the input image. Therefore the bug is that LXCopyFromSys currently expects 16bpp input data, but is called with 8bpp planes in the LXCopyPlanar path. This patch fixes it by deriving the bpp from the input parameters.
On Wed, Aug 04, 2010 at 07:44:10PM -0700, bugzilla-daemon@freedesktop.org wrote: > Fixed it. > > The set_bpp call is valid. We're BLTing an image from system memory to video > memory. We aren't displaying it on the screen; the screen output BPP is > irrelevant. As we're copying without transforming the data in any way, the BLT > output BPP needs to be the same as the input image. > > Therefore the bug is that LXCopyFromSys currently expects 16bpp input data, but > is called with 8bpp planes in the LXCopyPlanar path. This patch fixes it by > deriving the bpp from the input parameters. Um, stride in bytes / width in pixels isn't always bpp, given various alignment constraints and the need for padding. You'll just have to pass a bpp/format in explicitly.
Unless I'm mistaken, this is fixed by commit e9effca821c1d604aeffeb3d3e7a49539485117d in Geode 2.11.9 that was just released?
Created attachment 38497 [details] [review] fix Daniel Stone's concern additional patch to address Daniel's concern above
Close this bug now that it has been fixed. Thanks, Frank
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.