Bug 19337

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

*** Bug 19532 has been marked as a duplicate of this bug. ***

*** Bug 19539 has been marked as a duplicate of this bug. ***

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.

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?

I tested Keith patch, but it is not enough.

(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?

Alex : I couldn't apply your patch directly on mandriva package but I adapted it and it fixed the crash.

(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.

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.

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).

> 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.

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.

Sorry, server 1.6 didn't have the merge of keithp's fix.  I've nominated it for inclusion.

AFAIK KeithP's fix is not sufficient (see previous comments). A reworked version of Alex's patch would be a better solution.

I have tested 1.6 RC2 with ea309e47457156b60aadbf113f04e5b6851029c8 added, and it works fine on savage.

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.

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?

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.

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

This appear to be fixed on 1.6 (jaunty 1.6.0-0ubuntu1 package) on my i815.

Latest patch on comment #17 was not applied, but the problem appears to be fixed now.