Bug 45623 - hw/xfree86/modes/xf86EdidModes.c fails to build: array subscript is above array bounds
Summary: hw/xfree86/modes/xf86EdidModes.c fails to build: array subscript is above arr...
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard: 2012BRB_Reviewed
Keywords: patch
Depends on:
Blocks: xserver-1.13
  Show dependency treegraph
 
Reported: 2012-02-04 08:55 UTC by Torsten Kaiser
Modified: 2012-07-09 01:14 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix access to EstIIIModes[] (815 bytes, patch)
2012-02-08 13:26 UTC, Torsten Kaiser
no flags Details | Splinter Review
Fix above array bounds error (1.17 KB, patch)
2012-07-05 22:57 UTC, Torsten Kaiser
no flags Details | Splinter Review
Patch for the second issue: the skipped modes (672 bytes, patch)
2012-07-05 22:58 UTC, Torsten Kaiser
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.