Xorg crashed with SIGSEGV in CopyKeyClass()

Bug #311254 reported by Matt Zimmerman
2
Affects Status Importance Assigned to Milestone
X.Org X server
Fix Released
Critical
xorg-server (Ubuntu)
Fix Released
High
Bryce Harrington

Bug Description

Binary package hint: xorg

This happened after I pressed a button on an ATI USB remote control to seek backward in a video in totem.

ProblemType: Bug
Architecture: amd64
DistroRelease: Ubuntu 9.04
Package: xserver-xorg 1:7.4~5ubuntu9
ProcEnviron:
 LC_COLLATE=C
 PATH: custom, user
 LANG=en_US.UTF-8
 SHELL=/bin/zsh
ProcVersion: Linux version 2.6.28-3-generic (buildd@yellow) (gcc version 4.3.3 20081210 (prerelease) (Ubuntu 4.3.2-2ubuntu8) ) #4-Ubuntu SMP Fri Dec 12 22:47:31 UTC 2008

SourcePackage: xorg
Uname: Linux 2.6.28-3-generic x86_64
xkbcomp:

Backtrace:
0: /usr/X11R6/bin/X(xorg_backtrace+0x26) [0x4ef056]
1: /usr/X11R6/bin/X(xf86SigHandler+0x41) [0x483a51]
2: /lib/libc.so.6 [0x7f3c3c67d030]
3: /usr/X11R6/bin/X(CopyKeyClass+0x75) [0x53d155]
4: /usr/X11R6/bin/X(mieqProcessInputEvents+0x2d3) [0x4cfa93]
5: /usr/X11R6/bin/X(ProcessInputEvents+0x9) [0x484669]
6: /usr/X11R6/bin/X(Dispatch+0x71) [0x44d591]
7: /usr/X11R6/bin/X(main+0x3bd) [0x4332bd]
8: /lib/libc.so.6(__libc_start_main+0xe6) [0x7f3c3c668586]
9: /usr/X11R6/bin/X [0x432749]

[lspci]
00:00.0 Host bridge [0600]: Intel Corporation Mobile PM965/GM965/GL960 Memory Controller Hub [8086:2a00] (rev 0c)
     Subsystem: Lenovo Device [17aa:20b3]
00:02.0 VGA compatible controller [0300]: Intel Corporation Mobile GM965/GL960 Integrated Graphics Controller [8086:2a02] (rev 0c)
     Subsystem: Lenovo Device [17aa:20b5]

Related branches

Revision history for this message
Matt Zimmerman (mdz) wrote :
Revision history for this message
Matt Zimmerman (mdz) wrote :
Revision history for this message
Matt Zimmerman (mdz) wrote :

Tagged as a regression, as this worked correctly in Intrepid. Note that this is reliably reproducible for me.

Bryce Harrington (bryce)
description: updated
Revision history for this message
Bryce Harrington (bryce) wrote :

Hi Matt,

Merry Christmas. There is one upstream bug report, #19222, with a very similar backtrace (but different method to reproduce); I'm wondering if it's related but I'd need to see a full backtrace to be sure. Too bad apport didn't take care of doing that, but at least it sounds like this is easy enough to reproduce?

Would you mind collecting a full backtrace in gdb for this issue? If my guess is right, we'd see master=0x0; if wrong it'd at least point us in a better direction. (ref. http://wiki.ubuntu.com/X/Backtracing)

Changed in xorg-server:
status: New → Incomplete
Revision history for this message
Bryce Harrington (bryce) wrote :

[309785 may be a dupe]

Revision history for this message
Matt Zimmerman (mdz) wrote :

Yes, it's easily reproducible.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fef0719c6f0 (LWP 7190)]
0x000000000053d155 in CopyKeyClass (device=0x1b55fa0, master=0x1ac2090)
    at /usr/include/bits/string3.h:52
52 return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
(gdb) bt full
#0 0x000000000053d155 in CopyKeyClass (device=0x1b55fa0, master=0x1ac2090)
    at /usr/include/bits/string3.h:52
        mk = (KeyClassPtr) 0x0
        dk = (KeyClassPtr) 0x1b566c0
        lastMapNotifyDevice = (DeviceIntPtr) 0x1ac2740
#1 0x00000000004cfa93 in mieqProcessInputEvents () at ../../mi/mieq.c:361
        handler = (mieqHandler) 0
        e = <value optimized out>
        type = <value optimized out>
        nevents = 1
        i = <value optimized out>
        screen = (ScreenPtr) 0x1937830
        event = (xEvent *) 0x2944fe0
        master_event = (xEvent *) 0x0
        dev = (DeviceIntPtr) 0x1b55fa0
        master = (DeviceIntPtr) 0x1ac2090
#2 0x0000000000484669 in ProcessInputEvents ()
    at ../../../../hw/xfree86/common/xf86Events.c:174
        x = 0
        y = 8232584
#3 0x000000000044d591 in Dispatch () at ../../dix/dispatch.c:363
        result = 0
        client = (ClientPtr) 0x1b337d0
        nready = -1
        start_tick = <value optimized out>
#4 0x00000000004332bd in main (argc=10, argv=0x7fff0f1bde78,
    envp=<value optimized out>) at ../../dix/main.c:383
        i = 1
        alwaysCheckForInput = {0, 1}

Revision history for this message
Matt Zimmerman (mdz) wrote : Re: [Bug 311254] Re: Xorg crashed with SIGSEGV in CopyKeyClass()

On Tue, Dec 30, 2008 at 11:11:48PM -0000, Timo Aaltonen wrote:
> *** This bug is a duplicate of bug 309785 ***
> https://bugs.launchpad.net/bugs/309785
>
> ** This bug has been marked a duplicate of bug 309785
> [Jaunty] Pressing any key in onboard crashes xserver with SIGSEGV in CopyKeyClass

Are you sure about this? In 309785, master=0x0, while in my bug, master
looks valid and mk=0x0.

--
 - mdz

Revision history for this message
Timo Aaltonen (tjaalton) wrote :

nope, unduping for now. I've raised the issue on the upstream bug which resulted in patch 154. You could try building the server without that and try reproducing the crash with it.

Revision history for this message
Matt Zimmerman (mdz) wrote :

As expected, removing the patch does avoid the crash (since this call stack cannot happen without the patch). As a side effect, my keymap on the gdm login screen is wrong (which is what I assume the patch is for).

I just want to point out that there may be a distinct bug here, and that fixing 309785 will not necessarily fix this one, even though both bugs seem to be caused by the patch.

Matt Zimmerman (mdz)
Changed in xorg-server:
importance: Undecided → High
status: Incomplete → Triaged
Revision history for this message
Bryce Harrington (bryce) wrote :

The upstream bug resulted in the following patch, which looks like it should solve the issue.

Revision history for this message
Bryce Harrington (bryce) wrote :

Should be added to our xserver git tree once it is building, and tested.

Changed in xorg-server:
status: Triaged → In Progress
Revision history for this message
In , Pcpa (pcpa) wrote :

Original bug report:

https://qa.mandriva.com/show_bug.cgi?id=46863

  Steps to reproduce:

1. plug a usb keyboard with multimedia keys
2. press a multimedia key
3. X Server crashes here

  The problem doesn't happen in git master.
But happens in a computer with only "released"
tarballs installed.

Revision history for this message
In , Keith Packard (keithp) wrote :

git bisect and a stack trace seem indicated here; I'm not seeing any trouble with media keys on this build.

Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

Created an attachment (id=21994)
0001-Xi-Force-a-CopyKeyClass-on-the-VCK-when-posting-a-k.patch

Something like this should fix the crash. I think my device at home is such a combo that posts some event through the mouse so I might be able to reproduce this tonight.

Revision history for this message
In , Pcpa (pcpa) wrote :
Download full text (3.3 KiB)

(In reply to comment #2)
> Created an attachment (id=21994) [details]
> 0001-Xi-Force-a-CopyKeyClass-on-the-VCK-when-posting-a-k.patch
>
> Something like this should fix the crash. I think my device at home is such a
> combo that posts some event through the mouse so I might be able to reproduce
> this tonight.

  As I told you in irc it corrects the crash, but it will not post
the events. To trigger the problem, it appears to be required to
have a "main" keyboard without multimedia keys, and then attach
the new keyboard with multimedia keys.

  Before using this patch I started with a patch like (don't have
access to both computers now):
-%<-
diff --git a/Xi/exevents.c b/Xi/exevents.c
index 083bb2f..d245382 100644
--- a/Xi/exevents.c
+++ b/Xi/exevents.c
@@ -192,7 +192,7 @@ CopyKeyClass(DeviceIntPtr device, DeviceIntPtr master)
     static DeviceIntPtr lastMapNotifyDevice = NULL;
     KeyClassPtr mk, dk; /* master, device */
     BOOL sendNotify = FALSE;
- int i;
+ int i, size;

     if (device == master)
         return;
@@ -202,17 +202,22 @@ CopyKeyClass(DeviceIntPtr device, DeviceIntPtr master)

     if (device != dixLookupPrivate(&master->devPrivates,
                                    CoreDevicePrivateKey)) {
- memcpy(mk->modifierMap, dk->modifierMap, MAP_LENGTH);
-
- if (dk->maxKeysPerModifier)
- {
- mk->modifierKeyMap = xrealloc(mk->modifierKeyMap,
- 8 * dk->maxKeysPerModifier);
+ if (dk->maxKeysPerModifier) {
+ if (mk == NULL) {
+ mk = master->key = xmalloc(sizeof(KeyClassRec));
+ if (!mk)
+ FatalError("[Xi] no memory for class shift.\n");
+ memcpy(mk, dk, sizeof(KeyClassRec));
+ mk->modifierMap = NULL;
+ }
+ size = 8 * dk->maxKeysPerModifier;
+ if (size < MAP_LENGTH)
+ size = MAP_LENGTH;
+ mk->modifierKeyMap = xrealloc(mk->modifierKeyMap, size);
             if (!mk->modifierKeyMap)
                 FatalError("[Xi] no memory for class shift.\n");
- memcpy(mk->modifierKeyMap, dk->modifierKeyMap,
- (8 * dk->maxKeysPerModifier));
- } else
+ memcpy(mk->modifierMap, dk->modifierMap, size);
+ } else if (mk)
         {
             xfree(mk->modifierKeyMap);
             mk->modifierKeyMap = NULL;
-%<-
  But it would fail a few lines below in:
            if (!XkbCopyKeymap(dk->xkbInfo->desc, mk->xkbInfo->desc, True))
                FatalError("Couldn't pivot keymap from device to core!\n");

  But I did not try a merge of the above patch with yours, neither
the remaining of the code I wrote in an attempt to "port" git master
behavior to origin/server-1.6-branch. Example, I converted
static void
ChangeMasterDeviceClasses(DeviceIntPtr device,
                          deviceClassesChangedEvent *dcce)
{...}
to something like:
static void
ChangeMasterDeviceClasses(DeviceIntPtr device)
{
    DeviceIntPtr master = device->u.master;

    if (device->isMaster)
        return;

    if (!master) /* if device was set floating between SIGIO and now */
        return;

    master->public.devicePrivate = device->publi...

Read more...

Revision history for this message
In , Pcpa (pcpa) wrote :
Download full text (6.5 KiB)

(In reply to comment #1)
> git bisect and a stack trace seem indicated here; I'm not seeing any trouble
> with media keys on this build.

  The crash happens when plugging a usb keyboard on a computer with
a simple/standard keyboard, and using the multimedia keys in the
usb keyboard.

  I started with tag 1.5.99.3, after the revert of mpx, etc, as in this
computer, everything, but X Server was the latest released tarball.
  1.5.99.3 is "good" in the sense that it doesn't crash, but the event
is not sent to any window, but it should be "acceptable" to require
properly configuring xkb in this case :-)

% git bisect log
# bad: [251d0d8090322b2c9dc0c8b7bef001f338d19433] Update version to 1.5.99.901 (1.6 RC1)
# good: [523aae1fa6d8002e55e85aee49f113b7eb9a6df3] Bump version to 1.5.99.3 (1.6 beta3)
git bisect start 'origin/server-1.6-branch' 'xorg-server-1.5.99.3'
# good: [6c635faa6ff0474199f4f7375022efe1e8ffa8f1] XQuartz: update quoting in case X11.app is moved to a directory with a space. (cherry picked from commit cc805dc799efa37c8dcefa3db04d87e9b835ffbd) (cherry picked from commit ecc3a7b6090552c309fe8e264d527ddd666a5761)
git bisect good 6c635faa6ff0474199f4f7375022efe1e8ffa8f1
# good: [2ce48363b862db134624797bc071f8c45323a075] xfree86: don't call CheckMotion if a device hasn't been enabled. #19176
git bisect good 2ce48363b862db134624797bc071f8c45323a075
# good: [8cfb353078d9b5d03a9633304038141a60adc970] dix: Fix handling of do_not_propagate_mask window attribute.
git bisect good 8cfb353078d9b5d03a9633304038141a60adc970
# good: [69ddac23281534a06c0acb3005a09e4448594925] Apple: Don't use DRI2 (cherry picked from commit a1d35cee5907a76977ee43c49cb55c8f411c9794)
git bisect good 69ddac23281534a06c0acb3005a09e4448594925
# bad: [6be355b8e8cabeb5832ce9970a83782ea46fd4d1] dix: drop x/y back into last.valuators before updating the history (#19285)
git bisect bad 6be355b8e8cabeb5832ce9970a83782ea46fd4d1

cut & paste of the first bad commit (for easier visualization):
% git show 6be355b8e8cabeb5832ce9970a83782ea46fd4d1
commit 6be355b8e8cabeb5832ce9970a83782ea46fd4d1
Author: Peter Hutterer <email address hidden>
Date: Fri Jan 9 13:46:20 2009 +1000

    dix: drop x/y back into last.valuators before updating the history (#19285)

    positionSprite needs to scale to screen coordinates and in the process of
    doing so alters dev->last.valuators[0:1]. Drop the real coordinates back after
    finishing and before updating the motion history. This way, we don't push the
    screen coordinates into the motion history.

    X.Org Bug 19285 <http://bugs.freedesktop.org/show_bug.cgi?id=19285>
    (cherry picked from commit 56efbc0986e782da45addb05ece9f456d41d7a90)

diff --git a/dix/getevents.c b/dix/getevents.c
index 707d1da..16e23dc 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -695,6 +695,9 @@ positionSprite(DeviceIntPtr dev, int *x, int *y,
                                  dev->valuator->axes + 1, scr->height);
     }

+ /* dropy x/y (device coordinates) back into valuators for next event */
+ dev->last.valuators[0] = *x;
+ dev->last.valuators[1] = *y;
 }

 /**
@@ -980,9 +983,6 @@ GetPointerEvents(EventList *events, DeviceIntP...

Read more...

Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

> diff --git a/dix/getevents.c b/dix/getevents.c
> index 707d1da..16e23dc 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -695,6 +695,9 @@ positionSprite(DeviceIntPtr dev, int *x, int *y,
> dev->valuator->axes + 1, scr->height);
> }
>
> + /* dropy x/y (device coordinates) back into valuators for next event */
> + dev->last.valuators[0] = *x;
> + dev->last.valuators[1] = *y;
> }
>
> /**
> @@ -980,9 +983,6 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int
> type, int buttons,
> positionSprite(pDev, &x, &y, scr, &cx, &cy);
> updateHistory(pDev, first_valuator, num_valuators, ms);
>
> - /* dropy x/y (device coordinates) back into valuators for next event */
> - pDev->last.valuators[0] = x;
> - pDev->last.valuators[1] = y;
>
> /* Update the valuators with the true value sent to the client*/
> if (num_valuators >= 1 && first_valuator == 0)

I really fail to see how that patch has to do with the crash you described,
especially since GPE isn't touched at all on key events.

> Backtrace:
> 0: X(xorg_backtrace+0x3b) [0x812dbbb]
> 1: X(xf86SigHandler+0x51) [0x80c8bc1]
> 2: [0xffffe400]
> 3: X(mieqProcessInputEvents+0x327) [0x810d1e7]
> 4: X(ProcessInputEvents+0x17) [0x80c9787]
> 5: X(Dispatch+0x6e) [0x808716e]
> 6: X(main+0x3bd) [0x806c47d]
> 7: /lib/i686/libc.so.6(__libc_start_main+0xe5) [0xb7c0e5c5]
> 8: X [0x806b931]

this backtrace is unrelated to the patch above as well.

Revision history for this message
In , Pcpa (pcpa) wrote :

(In reply to comment #5)
[...]
> I really fail to see how that patch has to do with the crash you described,
> especially since GPE isn't touched at all on key events.
>
> > Backtrace:
> > 0: X(xorg_backtrace+0x3b) [0x812dbbb]
> > 1: X(xf86SigHandler+0x51) [0x80c8bc1]
> > 2: [0xffffe400]
> > 3: X(mieqProcessInputEvents+0x327) [0x810d1e7]
> > 4: X(ProcessInputEvents+0x17) [0x80c9787]
> > 5: X(Dispatch+0x6e) [0x808716e]
> > 6: X(main+0x3bd) [0x806c47d]
> > 7: /lib/i686/libc.so.6(__libc_start_main+0xe5) [0xb7c0e5c5]
> > 8: X [0x806b931]
>
> this backtrace is unrelated to the patch above as well.

  Err, you are right, me being dumb :-), I stopped in the first "bad"
commit in the binary search, cannot test it right now, but the proper
one is in the range:
% git shortlog 10c0287232eab1b93d078774f52e65efa0c03607..681cc0f38b0b96c5e41c93d6944e4fa58c950eda
Jeremy Huddleston (2):
      XQuartz: Make debugging output for invalid depths a bit more detailed
      XQuartz: Use depth=24 instead of FatalError if we can't figure out our depth

Keith Packard (1):
      Merge commit 'whot/server-1.6-branch' into server-1.6-branch

Peter Hutterer (4):
      Let the DDX decide on the XkbRulesDefaults.
      xfree86: Only use the evdev ruleset on linux.
      dix: Fix handling of do_not_propagate_mask window attribute.
      mi: force CopyKeyClass for key events. (#19048)

Paulo

Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

> mi: force CopyKeyClass for key events. (#19048)

my money is on this one.

Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

Created an attachment (id=22028)
0001-mi-force-the-paired-kbd-device-before-CopyKeyClass.patch

This fixes the bug on my box, but more testing would be appreciated.

Revision history for this message
Bryce Harrington (bryce) wrote :

I studied the patch a bit more closely and am unconvinced that it by itself is guaranteed to solve this issue. Like mdz pointed out, in this case mk is null, not master.

This patch should handle this case, although it leaves open the question of why mk was NULL to begin with. I'll forward this patch upstream to see what comment we can get on it.

Bryce Harrington (bryce)
Changed in xorg-server:
assignee: nobody → bryceharrington
Revision history for this message
In , Pcpa (pcpa) wrote :

(In reply to comment #8)
> Created an attachment (id=22028) [details]
> 0001-mi-force-the-paired-kbd-device-before-CopyKeyClass.patch
>
> This fixes the bug on my box, but more testing would be appreciated.

  Thanks. This corrects the problem, and sends the proper events,
like in git master.

  Keeping the bug open as it should be closed when this patch is
pushed to 1.6 branch.

Revision history for this message
In , Charles (charles-bovy) wrote :

I can confirm the patch works! X doesn't crash anymore using multimedia keyboard.
Maybe related, but still some keys are not working properly.
Key 'm' does generate KP_Insert
After pressing Numlock once:
Key 'm' does generate KP_0
After pressing Numlock again:
Key 'm' does generate m.
Same is valid for other keypad keys.

Bryce Harrington (bryce)
description: updated
Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

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

Revision history for this message
Launchpad Janitor (janitor) wrote :

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

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

  [ Timo Aaltonen ]
  * debian/rules: Enable dbus-support.
  * Merge current server-1.6-branch.
  * Disable patch 107 for now, to see what kind of a performance hit
    it'll be. The problem it causes is random garbage on windows
    while apps are being loaded.
    (LP: #254468)
  * Remove patches 150, 151, 152, 154, applied upstream.

  [ Bryce Harrington ]
  * 156_exevents_copykeyclass_nullptrcheck.patch: Add several NULL pointer
    checks in CopyKeyClass to prevent SEGFAULT seen when pressing button
    on an ATI USB remote control.
    (LP: #311254)

 -- Timo Aaltonen <email address hidden> Sat, 17 Jan 2009 16:17:58 +0200

Changed in xorg-server:
status: In Progress → Fix Released
Revision history for this message
Dan Andreșan (danyer) wrote :

This fixed something else too. My multimedia keyboard crashed X every time I pressed a "strange" button: Back, Forward, Volume Up, Volume Down, etc.

It used to work perfectly, but since (maybe) 2 months ago I cannot use any multimedia button, just normal 104 Generic buttons.

After todays update, which implemented the fix, the multimedia keys don't crash X, they are simply ignored. All except Power, which brings up the dialog (Shut Down the Computer).

Revision history for this message
Matt Zimmerman (mdz) wrote :

Verified, this avoids the crash I originally reported. However, the remote still doesn't work (and worked under 8.10). I've filed bug 318261 about that.

Changed in xorg-server:
status: Unknown → Invalid
Matt Zimmerman (mdz)
Changed in xorg-server:
status: Invalid → Unknown
Changed in xorg-server:
status: Unknown → In Progress
Revision history for this message
In , Peter Hutterer (peter-hutterer) wrote :

Reassigning to keith for 1.6 picking.

Changed in xorg-server:
status: In Progress → Confirmed
Revision history for this message
In , Keith Packard (keithp) wrote :

I've applied this patch to the 1.6 branch, marking it as fixed.

Changed in xorg-server:
status: Confirmed → Fix Released
Changed in xorg-server:
importance: Unknown → Critical
Changed in xorg-server:
importance: Critical → Unknown
Changed in xorg-server:
importance: Unknown → Critical
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

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