Bug 57448 - xrestop kills Xorg 1.13->
xrestop kills Xorg 1.13->
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Server/General
git
x86 (IA32) Linux (All)
: medium normal
Assigned To: Xorg Project Team
Xorg Project Team
:
Depends on:
Blocks: xserver-1.14
  Show dependency treegraph
 
Reported: 2012-11-23 09:17 UTC by Knut Petersen
Modified: 2012-12-01 12:07 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
call rrprovider init where it should be called (888 bytes, patch)
2012-11-30 10:35 UTC, Dave Airlie
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Knut Petersen 2012-11-23 09:17:00 UTC
Hardware
========
AOpen i915GMm-HFS, Pentium M Dothan, 2GB RAM

Software
==========
openSuSE 12.1 with kernel 3.6.7, fresh Xorg and xrestop

Problem
=======
Starting xrestop in a terminal window immediately kills Xorg.
Keyboard and mouse dead, screen frozen.
Login via ssh is possible, absolutely nothing related in dmesg and Xorg.0.log
Restarting Xorg via ssh works fine and reanimates screen/keyboard/mouse.

Starting xrestop via ssh -X from another system works fine.

cu,
 Knut
Comment 1 Knut Petersen 2012-11-23 09:23:27 UTC
Forget about "Starting xrestop via ssh -X from another system works fine."
That does inspect the xserver on the wrong system.

cu,
 Knut
Comment 3 Dave Gilbert 2012-11-24 21:53:02 UTC
Since everything useful was optimised out, I added some debug:

....

FindAllClientResources: i=19
FindAllClientResources: resources loop: this=0x7ffe7b0cc420 next=(nil) this->value=0x7ffe7b0cc380 id=52 type=21
ResFindAllRes: value=0x7ffe7b0cc380 id=82 type=21 TypeMask=1fffffff counts=0x7ffe7bc0e950
FindAllClientResources: i=20
FindAllClientResources: resources loop: this=0x7ffe7b0cd840 next=(nil) this->value=0x7ffe7b0cd7d0 id=55 type=19
ResFindAllRes: value=0x7ffe7b0cd7d0 id=85 type=19 TypeMask=1fffffff counts=0x7ffe7bc0e950
FindAllClientResources: i=21
FindAllClientResources: resources loop: this=0x7ffe7b76a600 next=0x7ffe7b0cd610 this->value=0x7ffe7b76ae80 id=193 type=44
ResFindAllRes: value=0x7ffe7b76ae80 id=403 type=44 TypeMask=1fffffff counts=0x7ffe7bc0e950
FindAllClientResources: resources loop: this=0x7ffe7b0cd610 next=(nil) this->value=0x7ffe7b0cd5b0 id=54 type=0
ResFindAllRes: value=0x7ffe7b0cd5b0 id=84 type=0 TypeMask=1fffffff counts=0x7ffe7bc0e950
Comment 4 Dave Gilbert 2012-11-25 00:37:02 UTC
OK, a bit more;

xres.c ResFindAllRes doesn't handle the case where the type is 0 (it uses type-1 as an index into an array); it's trivial to fix that with an

if ((type & TypeMask)!=0) 

the question is should that ever happen?

I can see that dix/resource.c:AddResource is getting called (once) with a type of 0 - is that legal?
Comment 5 Dave Gilbert 2012-11-25 01:17:38 UTC
and my final one for tonight:
The case where AddResource is being called with a 0 type is internal to the server:

No locals.
#1  0x00005555555ccce2 in AddResource (id=84, type=0, value=0x5555559ddaa0) at ../../dix/resource.c:799
        client = <optimised out>
        rrec = <optimised out>
        res = <optimised out>
        head = <optimised out>
#2  0x000055555566e5ce in RRProviderCreate (pScreen=0x5555559b5a80, name=0x5555559bd550 "radeon", nameLength=6) at ../../randr/rrprovider.c:361
        provider = 0x5555559ddaa0
        pScrPriv = 0x5555559d9a50
line 361:
    if (!AddResource (provider->id, RRProviderType, (pointer) provider))
        return NULL;

RRProviderType set in RRProviderInit
#3  0x000055555562ab04 in xf86RandR12CreateObjects12 (pScreen=0x5555559b5a80) at ../../../../hw/xfree86/modes/xf86RandR12.c:1572
        pScrn = 0x5555559b7c80
        config = 0x5555559ba040
        c = <optimised out>
        o = <optimised out>
#4  xf86RandR12Init12 (pScreen=0x5555559b5a80) at ../../../../hw/xfree86/modes/xf86RandR12.c:1929
        pScrn = <optimised out>
        rp = 0x5555559d9a50
        randrp = 0x7ffff6343ac0 <_IO_stdfile_2_lock>
        i = <optimised out>
#5  xf86RandR12Init (pScreen=0x5555559b5a80) at ../../../../hw/xfree86/modes/xf86RandR12.c:897
        rp = 0x38
        randrp = <optimised out>
#6  0x000055555561e596 in xf86CrtcScreenInit (screen=0x5555559b5a80) at ../../../../hw/xfree86/modes/xf86Crtc.c:778
        scrn = <optimised out>
        config = 0x5555559ba040
        c = <optimised out>
#7  0x00007ffff4c15625 in ?? () from /usr/lib/xorg/modules/drivers/radeon_drv.so
No symbol table info available.
#8  0x00005555555a9d25 in AddScreen (pfnInit=0x7ffff4c15150, argc=1, argv=0x7fffffffe678) at ../../dix/dispatch.c:3830
        i = 0
        pScreen = 0x5555559b5a80
        ret = <optimised out>
#9  0x00005555555eb4c3 in InitOutput (pScreenInfo=0x5555555e40c0 <xf86SetDGAMode>, argc=1, argv=0x7fffffffe678) at ../../../../hw/xfree86/common/xf86Init.c:913
        i = <optimised out>
        j = <optimised out>
        k = <optimised out>
        scr_index = <optimised out>
        modulelist = <optimised out>
        optionlist = 0x5555559a4040
        screenpix24 = <optimised out>
        pix24 = <optimised out>
        pix24From = <optimised out>
        pix24Fail = 0


as far as I can tell 'RRProviderType' is never initialised, because as far as I can tell RRProviderInit in randr/RRProviderInit is never called, and neither can I see where it's supposed to be called.

So I think there are three fixes that are needed here:

   1) Xext/xres.c ResFindAllRes
change

   counts[(type & TypeMask) -1]++
to
   if ((type & TypeMask)!=0) counts[(type & TypeMask) - 1]++;

   2) If it's not legal to have a 0-type'd resource, then dix/resource.c:AddResource  should check for it and reject it.

   3) fix either randr/rrprovider.c or whatever should be calling RRProviderInit to call it before hand.
Comment 6 Dave Airlie 2012-11-30 10:35:07 UTC
Created attachment 70823 [details] [review]
call rrprovider init where it should be called

this implements option 3, no idae where this hunk got lost.
Comment 7 Vasily Khoruzhick 2012-11-30 10:44:57 UTC
(In reply to comment #6)
> Created attachment 70823 [details] [review] [review]
> call rrprovider init where it should be called
> 
> this implements option 3, no idae where this hunk got lost.

This patch fixes crash for me, thanks!
Comment 8 Knut Petersen 2012-11-30 18:02:36 UTC
(In reply to comment #6)
> Created attachment 70823 [details] [review] [review]
> call rrprovider init where it should be called
> 
> this implements option 3, no idae where this hunk got lost.

Works here, thanks!

cu,
 Knut
Comment 9 Knut Petersen 2012-11-30 18:04:09 UTC
Dave did it ...
Comment 10 Alan Coopersmith 2012-11-30 20:51:50 UTC
Fix pushed to git master for 1.14:
http://lists.x.org/archives/xorg-devel/2012-November/034609.html
Comment 11 Dave Gilbert 2012-12-01 12:07:15 UTC
Thanks Dave!

Do you think it would be worth adding the check in to cover (1) and a printf for (2).
Someone else is bound to screw up and try registering a type 0 resource somewhere ?

Dave