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
Re: [PATCH xorg-server] Fix xf86EdidModes.c: array subscript is above array bounds
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 should probably just compute the index and skip it if it's out of range. Sending a mode of all zeros along to the rest of the server is likely to cause chaos (and divide-by-zero errors). 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. -- keith.pack...@intel.com pgpLYMC0dEhyx.pgp Description: PGP signature ___ 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 7/5/12 7:37 PM, Keith Packard wrote: Torsten Kaiserjust.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... I've added my r-b to the patches on https://bugs.freedesktop.org/45623 - ajax ___ 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
Adam Jackson a...@redhat.com writes: I've added my r-b to the patches on https://bugs.freedesktop.org/45623 Merged. 9e4b8b7..7c9d8cb master - master (for future reference, applying patches from bugzilla is a huge pain. Please use git send-email to send patches to the list if at all possible, applying those is a couple of keystrokes each...) -- keith.pack...@intel.com pgppEZ1RgtRcu.pgp Description: PGP signature ___ 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
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... -- keith.pack...@intel.com pgppmgmhOGWr2.pgp Description: PGP signature ___ 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 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... 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 Cheers, Peter ___ 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
