Bug 45623

Summary: hw/xfree86/modes/xf86EdidModes.c fails to build: array subscript is above array bounds
Product: xorg Reporter: Torsten Kaiser <x11>
Component: Server/GeneralAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: ajax, hramrach, jeremyhu, peter.hutterer, walch.martin
Version: unspecifiedKeywords: patch
Hardware: All   
OS: All   
Whiteboard: 2012BRB_Reviewed
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 44202    
Attachments:
Description Flags
Fix access to EstIIIModes[]
none
Fix above array bounds error
none
Patch for the second issue: the skipped modes none

Description Torsten Kaiser 2012-02-04 08:55:54 UTC
Commit 99e22b86c5f1a3653f3caaf01368a777d2b208d0 from Adam Jackson introduced a build failure for me.
Gcc (4.5.3) complains that the access to EstIIIModes from DDCModesFromEstIII() goes beyound the end of that array.
And I think gcc is right.

DDCModesFromEstIII() loops i until 5 and j until 1, so the maximum index will be  (5*8)+(7-1) == 46, but EstIIIModes[] only contains 44 elements. So if the last bits of est are set, then DDCModesFromEstIII() will read garbage from beyond the end of the array. I suspect these bits are reserved in the standard, because there are no modes defined for them, but that does not prevent a monitor vendor from shipping a broken EDID.

So I would suggest filling the mode array with 4 additional dummy modes.

Hmmh: for (j = 7; j > 0; j--)
Shouldn't this be j >= 0? Otherwise the lowest bit will never be checked. (Modes "1152, 864, 75", "1280, 1024, 85", "1400, 1050, 75", ...)
Comment 1 Chí-Thanh Christopher Nguyễn 2012-02-07 13:31:20 UTC
A patch was posted here http://lists.x.org/archives/xorg-devel/2011-November/027176.html but it seems that nobody replied.
Comment 2 Torsten Kaiser 2012-02-08 13:26:02 UTC
Created attachment 56770 [details] [review]
Fix access to EstIIIModes[]

I would prefer this patch.
It solves my compile error and the resulting X server works for me. (I'm using it to post this comment...)
Comment 3 Matt Whitlock 2012-02-11 22:53:10 UTC
(In reply to comment #2)
> Created attachment 56770 [details] [review] [review]
> Fix access to EstIIIModes[]
> 
> I would prefer this patch.
> It solves my compile error and the resulting X server works for me. (I'm using it to post this comment...)

This patch is cleaner and corrects the off-by-one error, whereas the patch given in comment #1 does not.
Comment 4 Jeremy Huddleston Sequoia 2012-06-12 03:56:21 UTC
Is this still an issue for you?  If so, please send your patch to xorg-devel 
for review.
Comment 5 Torsten Kaiser 2012-06-12 10:22:23 UTC
Yes, the issue still exists and it is for example popping up in Gentoo:
https://bugs.gentoo.org/show_bug.cgi?id=405139
https://bugs.gentoo.org/show_bug.cgi?id=420127

The Gentoo-Fix for 1.11 is currently to automatically downgrade from -O3 to -O2, for 1.12 they added my patch.

I will post it to the mailing list.
Comment 6 Torsten Kaiser 2012-06-26 11:52:41 UTC
Patch send to mailing list 14 days ago: http://lists.freedesktop.org/archives/xorg-devel/2012-June/031660.html

No reaction, but I hope that means nobody has (serious) problems with it.
Comment 7 Jeremy Huddleston Sequoia 2012-07-04 18:17:04 UTC
Please followup on the list.  I've been on honeymoon for the past 2 weeks and have quite a backlog to churn through.  Hopefully someone can get to it before I do.
Comment 8 Torsten Kaiser 2012-07-05 22:57:40 UTC
Created attachment 63875 [details] [review]
Fix above array bounds error

Split the two issues into separate patches, suggesting from Peter Hutterer on the xorg-devel ml.
Comment 9 Torsten Kaiser 2012-07-05 22:58:47 UTC
Created attachment 63876 [details] [review]
Patch for the second issue: the skipped modes
Comment 10 Adam Jackson 2012-07-06 08:47:56 UTC
Comment on attachment 63875 [details] [review]
Fix above array bounds error

Review of attachment 63875 [details] [review]:
-----------------------------------------------------------------

I'd slightly prefer to do this by bounds-checking m itself, but this is fine too.

Reviewed-by: Adam Jackson <ajax@redhat.com>
Comment 11 Adam Jackson 2012-07-06 08:48:28 UTC
Comment on attachment 63876 [details] [review]
Patch for the second issue: the skipped modes

Review of attachment 63876 [details] [review]:
-----------------------------------------------------------------

Reviewed-by: Adam Jackson <ajax@redhat.com>
Comment 12 Torsten Kaiser 2012-07-07 01:11:45 UTC
Patches have been committed to the xserver git tree, closing as RESOLVED/FIXED

Thanks everyone!
Comment 13 Peter Hutterer 2012-07-09 01:14:20 UTC
commit f27fcb81c4a30cec899628e4bb3e300edbcebe4b and
commit 738e55ebbdf516a45b95761b815bed4e697dc726

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.