Re: [PATCH xorg-server] Fix xf86EdidModes.c: array subscript is above array bounds
On Fri, Jul 6, 2012 at 9:44 AM, Keith Packard kei...@keithp.com wrote: Torsten Kaiser just.for.l...@googlemail.com writes: My guess would be, that these bits where reserved in the spec and should never been set. These bits come from external sources, and often buggy ones at that. We Yes, because buggy devices can trigger this, I like to see it fixed and not only making gcc no longer notice this. should probably just compute the index and skip it if it's out of There was an earlier patch that did that: http://lists.x.org/archives/xorg-devel/2011-November/027176.html But I found that adding these calculations to the loop made it rather difficult to read. The solution with the dummy entries looked better to me, because its obvious what they are at the first glance. range. Sending a mode of all zeros along to the rest of the server is likely to cause chaos (and divide-by-zero errors). Thats why I added if (EstIIIModes[m].w) into the loop to skip over them, even if the device sends the strange bits to enable them. Just the current code could cause chaos, if these bits ever get enabled, because it would try to use whatever comes after EstIIIModes as mode information. The two issues where just located so near each other, that I fixed them in one go. It is nice to have separate patches anyways; one per bug. That can make back-porting changes easier to manage. Not arguing with that. Thats why I immediately redid patch(es) after your request. I just couldn't resist slipping the single = into the original patch, after I noticed the second bug. ;-) Torsten ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xorg-server] Fix xf86EdidModes.c: array subscript is above array bounds
On Fri, Jul 6, 2012 at 2:45 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, Jul 05, 2012 at 04:37:39PM -0700, Keith Packard wrote: Torsten Kaiser just.for.l...@googlemail.com writes: With this optimization level gcc notices, that the loop in function DDCModesFromEstIII() would go until i=5 and j=1 which would result in m = (5 * 8) + (7 - 1) = 46, but the array EstIIIModes[] only contains 44 elements. I'd like Adam's opinion; he wrote the original code and is the best person to verify that this does what the spec requires. On the face of it, it looks right to me though... My guess would be, that these bits where reserved in the spec and should never been set. But gcc can't know this and complains about the possible accesses. While the new code still looks at these bits, it will always ignore them, so it should be safe wrt. the spec. Except if the spec says, that if any of these bits are set, then the interpretation of the other bits should change. But then the old code would be broken in the same way. I'm not a big fan of fixing two separate bugs in one patches though, I'd prefer to split out the out-of-bounds from the skipping last mode issue. Reviewed-by: Peter Hutterer peter.hutte...@who-t.net otherwise Thanks for the review, split patches are now available at https://bugs.freedesktop.org/show_bug.cgi?id=45623 The two issues where just located so near each other, that I fixed them in one go. Torsten ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Bug 45623: tested patch available (was:Extending the merge window until monday)
Could somebody consider committing the patch from https://bugs.freedesktop.org/show_bug.cgi?id=45623 to the xserver tree? I posted it a month ago the list, but got no reaction. See: http://lists.freedesktop.org/archives/xorg-devel/2012-June/031660.html And Jeremy Huddleston suggest in the bug, that I should keep bothering the devel list... Why I think, it should be committed: It fixes two bugs, that are obious, if you look for them. And the patch should be well tested, as it is already getting shipped by Gentoo in their xorg-server-1.12.2. ( http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-x86/x11-base/xorg-server/files/xorg-server-1.12-xf86edidmodes-array-bounds.patch?view=log ) That version has been marked stable for amd64 and x86 since 24.6. and I can't find any relevant bugs in the Gentoo bugtracker. (Except that someone yet again hit the compile error with the unpatched 1.11 version: https://bugs.gentoo.org/show_bug.cgi?id=420127 ) Thanks for considering to apply my patch. Torsten ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xorg-server] Fix xf86EdidModes.c: array subscript is above array bounds
Using gcc with -O3 failes to compile the current version of hw/xfree86/modes/xf86EdidModes.c. With this optimization level gcc notices, that the loop in function DDCModesFromEstIII() would go until i=5 and j=1 which would result in m = (5 * 8) + (7 - 1) = 46, but the array EstIIIModes[] only contains 44 elements. The following patch fixes this by adding 4 dummy elements to the array so that for each 6 * 8 bits from the est_iii field a mode is defined. Additionally the patch fixes the loop for (j = 7; j 0; j--) to run until 0, otherwise the last mode of each byte will always be skipped. I had opened Bug https://bugs.freedesktop.org/show_bug.cgi?id=45623 for this and you can find the patch there too. Signed-off-by: Torsten Kaiser just.for.l...@googlemail.com --- a/hw/xfree86/modes/xf86EdidModes.c.orig 2012-02-08 22:00:45.805914457 +0100 +++ b/hw/xfree86/modes/xf86EdidModes.c 2012-02-08 22:02:49.615915120 +0100 @@ -731,6 +731,11 @@ { 1920, 1200, 85, 0 }, { 1920, 1440, 60, 0 }, { 1920, 1440, 75, 0 }, +/* fill up last byte */ +{ 0,0,0,0 }, +{ 0,0,0,0 }, +{ 0,0,0,0 }, +{ 0,0,0,0 }, }; static DisplayModePtr @@ -740,10 +745,11 @@ int i, j, m; for (i = 0; i 6; i++) { - for (j = 7; j 0; j--) { + for (j = 7; j = 0; j--) { if (est[i] (1 j)) { m = (i * 8) + (7 - j); - modes = xf86ModesAdd(modes, + if (EstIIIModes[m].w) + modes = xf86ModesAdd(modes, FindDMTMode(EstIIIModes[m].w, EstIIIModes[m].h, EstIIIModes[m].r, ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel