[jaunty] segfault during X startup with randr < 1.2 drivers

Bug #319210 reported by knarf on 2009-01-20
100
This bug affects 5 people
Affects Status Importance Assigned to Milestone
X.Org X server
Fix Released
High
xorg-server (Ubuntu)
High
Bryce Harrington

Bug Description

X server crashes during X startup with non-rand 1.2 drivers. At least the following drivers are known to segfault:
- savage (see also bug #311544)
- r128 (see also bug #308485)
- intel (only on i81x hardware, see also bug #310075)

The discussion around this bug can be followed on the fd.o bugzilla posting.

The patch attached to this LP bug is a reworked version of Alex' patch which applies to the current version of xorg-xserver in Jaunty (xorg-server-1.5.99.901). This should enable other affected drivers to be used again. Applying this patch negates the need of applying the patch posted in LP: 311544.

Related branches

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 an attachment (id=21982)
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.

This bug is related to lp: #311544 and http://bugs.freedesktop.org/show_bug.cgi?id=19337 in that it is the root cause of the savage driver (and other non-rand 1.2 drivers) crash during X startup. The discussion around this bug can be followed on the fd.o bugzilla posting.

The patch attached to this LP bug is a reworked version of Alex' patch which applies to the current version of xorg-xserver in Jaunty (xorg-server-1.5.99.901). This should enable other affected drivers to be used again. Applying this patch negates the need of applying the patch posted in LP: 311544.

Oibaf (oibaf) on 2009-01-20
description: updated
Oibaf (oibaf) on 2009-01-20
description: updated
description: updated
Oibaf (oibaf) on 2009-01-20
Changed in xorg-server:
status: New → Confirmed
Changed in xorg-server:
status: Unknown → Confirmed
Bryce Harrington (bryce) on 2009-01-23
Changed in xorg-server:
assignee: nobody → bryceharrington
importance: Undecided → High
status: Confirmed → Triaged

Please retest with master as of:

commit f716e3f3445d443cbc6507d27f806e9ad387120a
Author: Eric Anholt <email address hidden>
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 an attachment (id=22537)
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.

Tormod Volden (tormodvolden) wrote :

The upstream commit ea309e47457156b60aadbf113f04e5b6851029c8 (proposed for 1.6 branch) promises to fix this issue. I have Jaunty xserver-xorg-core packages with this patch in my PPA https://launchpad.net/~tormodvolden/+archive/ppa and can confirm that it works fine on (unpatched) savage.

Created an attachment (id=22646)
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

Jouni Mettala (jouni-mettala) wrote :

Your package fixes intel too.

Tormod Volden (tormodvolden) wrote :

I just updated my PPA package with the latest patch of Alex in the upstream report instead. Works fine also.

Erik Andrén (erik-andren) wrote :

I can confirm that Tormods PPA patches resolved this issue for me with a savage graphics card.

Ralph Green (severian) wrote :

Howdy,
  My i815 based system could not start X because of this error. I found a simple solution for now. Edit the /etc/X11/xorg.conf file and add a line that says:
         Option "NoAccel" "true"
 in the Device section. There are some references on a RedHat board about this making X very slow. It is performing OK for my needs. I am using this as a test box for jaunty and a Miro box after jaunty goes prime time. It plays videos fine now.

Bryce Harrington (bryce) wrote :

We'll be pulling in 1.6 after the alpha-5 freeze is lifted, so we'll get the fix at that point.

Changed in xorg-server:
status: Triaged → In Progress
knarf (launchpad-ubuntu-f) wrote :

I doubt that the i815 acceleration-related problem is caused by the same bug so it would not surprise me if that problem remains after 1.6 has been pulled.

Johan Kiviniemi (ion) wrote :

The change in commit ea309e47457156b60aadbf113f04e5b6851029c8 fixed savage for me, too.

Bryce Harrington (bryce) wrote :

xserver 1.6 is in our git tree, and includes this fix. Should be uploaded some time this week.

@Ralph: Your issue sounds like a separate bug and won't be looked at as part of this bug report, so you should report it separately if you want it looked into.

Changed in xorg-server:
status: In Progress → Fix Committed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package xorg-server - 2:1.6.0-0ubuntu1

---------------
xorg-server (2:1.6.0-0ubuntu1) jaunty; urgency=low

  [ Bryce Harrington ]
  * New upstream release
    - Fixes segfault during X startup for drivers with RANDR < 1.2
      (LP: #319210)
    - Fixes EDID for monitors that incorrectly report aspect ratio instead
      of resolution (LP: #311485)
    - Fixes issue where X stops responding to mouse clicks after some time
      if using Xinerama. (LP: #296167)
  * Add 162_null_crtc_in_rotation.patch: Fixes crash when two displays on
    separate cards are attached. X doesn't work with multiple cards yet,
    but crashing is not an appropriate way to handle such a situation.
    (LP: #139990)

  [ Timo Aaltonen ]
  * 159_xinerama_focus.patch,
    161_force_paired_kbd_device.patch:
    - Dropped, applied upstream

 -- Bryce Harrington <email address hidden> Fri, 06 Mar 2009 14:44:31 -0800

Changed in xorg-server:
status: Fix Committed → Fix Released

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.

Changed in xorg-server:
status: Confirmed → Fix Released
Changed in xorg-server:
importance: Unknown → High
Changed in xorg-server:
importance: High → Unknown
knarf (launchpad-ubuntu-f) wrote :

Why does bug watch updater still bother with this bug? I'd say close it as 'fixed' until someone reopens it on a newer release.

Changed in xorg-server:
importance: Unknown → High
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.