Bug 19337 - segfault during X startup with randr < 1.2 drivers
Summary: segfault during X startup with randr < 1.2 drivers
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: unspecified
Hardware: All All
: high major
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL: https://bugs.launchpad.net/ubuntu/+so...
Whiteboard:
Keywords:
: 19532 19539 (view as bug list)
Depends on:
Blocks: xserver-1.6.1
  Show dependency treegraph
 
Reported: 2008-12-30 03:30 UTC by Frank de Lange
Modified: 2009-08-24 03:32 UTC (History)
8 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Call xf86CrtcConfigInit() from within SavagePreInit() (1.55 KB, patch)
2008-12-30 03:30 UTC, Frank de Lange
no flags Details | Splinter Review
Check for private index initialization before getting config ptr (15.53 KB, patch)
2009-01-14 08:17 UTC, Alex Villacís Lasso
no flags Details | Splinter Review
gdb backtrace, savage & 1.5.99.902 (2.22 KB, text/plain)
2009-02-03 11:23 UTC, Tormod Volden
no flags Details
Make XF86_CRTC_CONFIG_PTR return null on invalid index and check for null in rest of code (8.38 KB, patch)
2009-02-06 07:12 UTC, Alex Villacís Lasso
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frank de Lange 2008-12-30 03:30:33 UTC
Created attachment 21563 [details] [review]
Call xf86CrtcConfigInit() from within SavagePreInit()

Savage crashes during X startup because it refers to an uninitialized variable (see bug report on launchpad). The attached patch fixes the segfault and makes it possible to use savage with the most recent version of the server as distributed in Ubuntu Jaunty ('1.5.99').
Comment 1 Keith Packard 2009-01-13 09:49:25 UTC
Could someone test this patch?

diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
index b972974..f4b04d8 100644
--- a/hw/xfree86/modes/xf86Crtc.c
+++ b/hw/xfree86/modes/xf86Crtc.c
@@ -2940,22 +2940,29 @@ xf86_crtc_clip_video_helper(ScrnInfoPtr pScrn,
 xf86_crtc_notify_proc_ptr
 xf86_wrap_crtc_notify (ScreenPtr screen, xf86_crtc_notify_proc_ptr new)
 {
-    ScrnInfoPtr                scrn = xf86Screens[screen->myNum];
-    xf86CrtcConfigPtr  config = XF86_CRTC_CONFIG_PTR(scrn);
-    xf86_crtc_notify_proc_ptr  old;
-    
-    old = config->xf86_crtc_notify;
-    config->xf86_crtc_notify = new;
-    return old;
+    if (xf86CrtcConfigPrivateIndex != -1)
+    {
+       ScrnInfoPtr             scrn = xf86Screens[screen->myNum];
+       xf86CrtcConfigPtr       config = XF86_CRTC_CONFIG_PTR(scrn);
+       xf86_crtc_notify_proc_ptr       old;
+       
+       old = config->xf86_crtc_notify;
+       config->xf86_crtc_notify = new;
+       return old;
+    }
+    return NULL;
 }
 
 void
 xf86_unwrap_crtc_notify(ScreenPtr screen, xf86_crtc_notify_proc_ptr old)
 {
-    ScrnInfoPtr                scrn = xf86Screens[screen->myNum];
-    xf86CrtcConfigPtr  config = XF86_CRTC_CONFIG_PTR(scrn);
-
-    config->xf86_crtc_notify = old;
+    if (xf86CrtcConfigPrivateIndex != -1)
+    {
+       ScrnInfoPtr             scrn = xf86Screens[screen->myNum];
+       xf86CrtcConfigPtr       config = XF86_CRTC_CONFIG_PTR(scrn);
+    
+       config->xf86_crtc_notify = old;
+    }
 }
 
 void
Comment 2 Eric Anholt 2009-01-13 10:30:38 UTC
*** Bug 19532 has been marked as a duplicate of this bug. ***
Comment 3 Alex Deucher 2009-01-13 10:46:33 UTC
*** Bug 19539 has been marked as a duplicate of this bug. ***
Comment 4 Alex Villacís Lasso 2009-01-13 10:55:20 UTC
Several other places need the check for xf86CrtcConfigPrivateIndex != -1 , not just the two functions noted in the proposed patch. I don't have the valgrind output handy, but I will check it when I return home.
Comment 5 Alex Villacís Lasso 2009-01-14 08:17:10 UTC
Created attachment 21982 [details] [review]
Check for private index initialization before getting config ptr

The diff in comment #1 does not apply in my tree, but I get the hang of it. This new patch adds checks for initialization in many other places beyond the first diff. My goal was to remove any attempt on an invalid read, not just the one that causes the actual crash. However, could somebody please check whether this one breaks xrandr-1.2 enabled drivers?
Comment 6 Frederic Crozat 2009-01-15 01:40:24 UTC
I tested Keith patch, but it is not enough.
Comment 7 Alex Villacís Lasso 2009-01-16 08:29:06 UTC
(In reply to comment #6)
> I tested Keith patch, but it is not enough.
> 

Could you please test mine instead, and report whether it applies and whether it actually solves your problem?
Comment 8 Frederic Crozat 2009-01-16 09:41:33 UTC
Alex : I couldn't apply your patch directly on mandriva package but I adapted it and it fixed the crash.
Comment 9 Ian Romanick 2009-01-19 16:58:18 UTC
(In reply to comment #5)
> Created an attachment (id=21982) [details]
> Check for private index initialization before getting config ptr
> 
> The diff in comment #1 does not apply in my tree, but I get the hang of it.
> This new patch adds checks for initialization in many other places beyond the
> first diff. My goal was to remove any attempt on an invalid read, not just the
> one that causes the actual crash. However, could somebody please check whether
> this one breaks xrandr-1.2 enabled drivers?

It would probably make the code cleaner if XF86_CRTC_CONFIG_PTR returned NULL when xf86CrtcConfigPrivateIndex is -1.  The various -1 checks would then be replaced with NULL pointer tests.  Any future uses of XF86_CRTC_CONFIG_PTR would at least get /some/ protection:  the NULL dereference will produce an obvious crash, but the -1 array index may not.
Comment 10 Frank de Lange 2009-01-20 05:39:16 UTC
I adapted Alex' patch to the current iteration of xorg-server in Ubuntu Jaunty (attached to LP: #319210) and tested it with an unpatched savage driver with positive results (similar to using a patched savage driver with unpatched xorg-server). I do agree that the patch needs some cleaning up and that it would be better to test for NULL instead of -1.
Comment 11 Eric Anholt 2009-01-30 20:18:48 UTC
Please retest with master as of:

commit f716e3f3445d443cbc6507d27f806e9ad387120a
Author: Eric Anholt <eric@anholt.net>
Date:   Fri Jan 30 20:10:21 2009 -0800

    modes: Protect xf86_crtc_supports_gamma() from non-RandR 1.2 drivers.

server-1.6-branch should also work, and doesn't need this fix.

If you still have problems, please run X under gdb (sshed in from another machine).
Comment 12 Fabio Pedretti 2009-02-03 02:36:34 UTC
> server-1.6-branch should also work, and doesn't need this fix.

I just tried 1.5.99.902 as packaged by Ubuntu and it's still crashing in the same way than 1.5.99.901 with my i815.

> If you still have problems, please run X under gdb (sshed in from another
> machine).

Can't try that at the moment.
Comment 13 Tormod Volden 2009-02-03 11:23:29 UTC
Created attachment 22537 [details]
gdb backtrace,  savage & 1.5.99.902

Here is a gdb "backtrace full". Unfortunately with an optimized build so some variables are lost, but it should give an idea what goes wrong.
Comment 14 Eric Anholt 2009-02-04 09:06:40 UTC
Sorry, server 1.6 didn't have the merge of keithp's fix.  I've nominated it for inclusion.
Comment 15 Frank de Lange 2009-02-04 09:34:19 UTC
AFAIK KeithP's fix is not sufficient (see previous comments). A reworked version of Alex's patch would be a better solution.
Comment 16 Tormod Volden 2009-02-05 14:37:07 UTC
I have tested 1.6 RC2 with ea309e47457156b60aadbf113f04e5b6851029c8 added, and it works fine on savage.
Comment 17 Alex Villacís Lasso 2009-02-06 07:12:10 UTC
Created attachment 22646 [details] [review]
Make XF86_CRTC_CONFIG_PTR return null on invalid index and check for null in rest of code

This is the second try at a fix for the bug. This version makes XF86_CRTC_CONFIG_PTR return NULL on an invalid index. In addition, this NULL is checked in every access on this file.
Comment 18 Tormod Volden 2009-02-06 14:51:41 UTC
Sorry, I didn't notice that ea309e47457156b60aadbf113f04e5b6851029c8 was the same as Keith's patch in comment #1. I would like to test Alex's latest patch as well but it does not apply against the 1.6 head. Alex, can you please update your tree?
Comment 19 Tormod Volden 2009-02-06 16:03:10 UTC
I applied the patch by deleting the one failing hunk (the last one in the .c file). It works fine on savage, and I also tested that it does not screw up for radeon.
Comment 20 Jouni Mettala 2009-02-06 16:32:36 UTC
I had this bug too using i815. I can confirm patch Tormod applied in Ubuntu fixes it in intel-driver.

https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bug/319210
Comment 21 Fabio Pedretti 2009-03-09 03:21:50 UTC
This appear to be fixed on 1.6 (jaunty 1.6.0-0ubuntu1 package) on my i815.
Comment 22 Fabio Pedretti 2009-08-24 03:32:21 UTC
Latest patch on comment #17 was not applied, but the problem appears to be fixed now.


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.