[PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect

[Mario Kleiner]

On 03/05/2018 10:56 PM, Alex Deucher wrote:
> On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer <michel at daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer at amd.com>
>>
>> Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
>> PointPriv->spriteFuncs doesn't point to the same struct in the latter as
>> in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
>> the wrong struct. Then in the next server generation,
>> info->Set/MoveCursor would end up pointing to
>> drmmode_sprite_set/move_cursor, resulting in an infinite loop if one of
>> them was called.
>>
>> To avoid this, only change the Set/MoveCursor hooks if their values
>> match our expectations, otherwise leave them as is. This is kind of a
>> hack, but the alternative would be invasive and thus risky changes to
>> the way we're wrapping CloseScreen, and it's not even clear that can
>> work without changing xserver code.
>>
>> Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
>>                        each screen")
>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> 
> Acked-by: Alex Deucher <alexander.deucher at amd.com>
> 

Nope, not quite, unfortunately. Tested against x-server master, mesa 
master, ati-ddx master, with sddm login manager. With a freshly started 
server, now on a dual-x-screen setup, instead of an infinite loop, i get 
a server crash as soon as i move the mouse cursor from X-Screen 0 to 
X-Screen 1:

(gdb) c
Continuing.

Thread 1 "X" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00000000004bcad2 in xf86CursorSetCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCurs=0x1177610, x=32, y=358) at xf86CursorRD.c:345
#2  0x000000000057a758 in miPointerUpdateSprite (pDev=0x11912e0) at 
mipointer.c:453
#3  0x000000000057aada in miPointerDisplayCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCursor=0x1177610) at mipointer.c:206
#4  0x00000000004cb1b6 in CursorDisplayCursor (pDev=<optimized out>, 
pScreen=0xf9a510, pCursor=0x1177610) at cursor.c:168
#5  0x000000000050f78b in AnimCurDisplayCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCursor=0x1177610) at animcur.c:196
#6  0x00000000004473b8 in ChangeToCursor (pDev=0x11912e0, 
cursor=0x1177610) at events.c:936
#7  0x000000000044b20a in CheckMotion (ev=ev at entry=0x7ffdad7a7060, 
pDev=pDev at entry=0x11912e0) at events.c:3081
#8  0x000000000051ee6b in ProcessDeviceEvent 
(ev=ev at entry=0x7ffdad7a7060, device=device at entry=0x11912e0) at 
exevents.c:1716
#9  0x000000000051f5fb in ProcessOtherEvent (ev=0x7ffdad7a7060, 
device=0x11912e0) at exevents.c:1873
#10 0x00000000005403a2 in ProcessPointerEvent (ev=0x7ffdad7a7060, 
mouse=0x11912e0) at xkbAccessX.c:756
#11 0x0000000000571712 in mieqProcessDeviceEvent (dev=0x1368e10, 
event=0x7ffdad7a7cc0, screen=0xf9a510) at mieq.c:496
#12 0x000000000057184a in mieqProcessInputEvents () at mieq.c:551
#13 0x000000000047a8a9 in ProcessInputEvents () at xf86Events.c:151
#14 0x000000000043e2a7 in Dispatch () at dispatch.c:417
#15 0x0000000000442568 in dix_main (argc=11, argv=0x7ffdad7a8ad8, 
envp=<optimized out>) at main.c:276
#16 0x00007f20fdf60830 in __libc_start_main (main=0x42c520 <main>, 
argc=11, argv=0x7ffdad7a8ad8, init=<optimized out>, fini=<optimized 
out>, rtld_fini=<optimized out>, stack_end=0x7ffdad7a8ac8) at 
../csu/libc-start.c:291

-mario


>> ---
>>   src/radeon_kms.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/radeon_kms.c b/src/radeon_kms.c
>> index 85390e306..790d4be16 100644
>> --- a/src/radeon_kms.c
>> +++ b/src/radeon_kms.c
>> @@ -2017,10 +2017,12 @@ static Bool RADEONCursorInit_KMS(ScreenPtr pScreen)
>>              return FALSE;
>>          }
>>
>> -       info->SetCursor = PointPriv->spriteFuncs->SetCursor;
>> -       info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
>> -       PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
>> -       PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
>> +       if (PointPriv->spriteFuncs->SetCursor != drmmode_sprite_set_cursor) {
>> +           info->SetCursor = PointPriv->spriteFuncs->SetCursor;
>> +           info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
>> +           PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
>> +           PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
>> +       }
>>       }
>>
>>       if (xf86ReturnOptValBool(info->Options, OPTION_SW_CURSOR, FALSE))
>> @@ -2184,8 +2186,10 @@ static Bool RADEONCloseScreen_KMS(ScreenPtr pScreen)
>>          miPointerScreenPtr PointPriv =
>>              dixLookupPrivate(&pScreen->devPrivates, miPointerScreenKey);
>>
>> -       PointPriv->spriteFuncs->SetCursor = info->SetCursor;
>> -       PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
>> +       if (PointPriv->spriteFuncs->SetCursor == drmmode_sprite_set_cursor) {
>> +           PointPriv->spriteFuncs->SetCursor = info->SetCursor;
>> +           PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
>> +       }
>>       }
>>
>>       pScreen->BlockHandler = info->BlockHandler;
>> --
>> 2.16.2
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx