reentrancy of mieqProcessInputEvents()?

Andy Ritger aritger at nvidia.com
Thu Jun 14 16:13:53 PDT 2012


Thanks, Peter and Keith.  I'll send out a patch to fix this up.

Thanks,
- Andy


On Tue, Jun 12, 2012 at 05:48:30PM -0700, Peter Hutterer wrote:
> On Tue, Jun 12, 2012 at 05:11:49PM -0700, Andy Ritger wrote:
> > Is mi/mieq.c:mieqProcessInputEvents() expected to be reentrant?  Or should
> > mieqProcessInputEvents() never be called in that situation?
> > 
> > More specifically, is there a reason that the 'event' local variable in
> > mieqProcessInputEvents() is declared static?
> > 
> >     static InternalEvent event;
> 
> I don't think so, this doesn't look right. looking at the log this was added
> in xorg-server-1.5.99.1-136-ga939368 to avoid calloc but you're certainly
> right, it breaks reentrancy. Following the history of that, it's not even
> needed anymore, this came from a time when we had to callloc a list of
> devices - now we can have just have one InternalEvent on the stack.
> 
> see more below though.
> 
> > Consider the following sequence:
> > 
> > (a) User hits ctrl-alt-+ to do an old-school mode switch, and the keyboard
> >     event is put on the event queue.
> > 
> > (b) mieqProcessInputEvents() pops the keyboard event from the event queue,
> >     storing the event data in a static local variable, and then passing a
> >     pointer to that variable into mieqProcessDeviceEvent():
> > 
> >     mieqProcessInputEvents(void)
> >     {
> >         [...]
> >         static InternalEvent event;
> >         [...]
> >         while (miEventQueue.head != miEventQueue.tail) {
> >             e = &miEventQueue.events[miEventQueue.head];
> > 
> >             event = *e->events;
> >             [...]
> >             mieqProcessDeviceEvent(dev, &event, screen);
> >         [...]
> >     }
> > 
> > (c) The event pointer gets passed through several functions until we
> >     hit XkbHandleActions():
> > 
> > #34 0x0000000000585596 in XkbHandleActions (dev=0x1176370, kbd=0x1176370, event=0x85cf00) at xkbActions.c:1234
> > #35 0x00000000005865d8 in XkbProcessKeyboardEvent (event=0x85cf00, keybd=0x1176370) at xkbPrKeyEv.c:146
> > #36 0x000000000057ad28 in AccessXFilterPressEvent (event=0x85cf00, keybd=0x1176370) at xkbAccessX.c:569
> > #37 0x000000000058678e in ProcessKeyboardEvent (ev=0x85cf00, keybd=0x1176370) at xkbPrKeyEv.c:181
> > #38 0x00000000005aec39 in mieqProcessDeviceEvent (dev=0x1176370, event=0x85cf00, screen=0xaa49e0) at mieq.c:557
> > #39 0x00000000005aee75 in mieqProcessInputEvents () at mieq.c:624
> > #40 0x000000000048adb9 in ProcessInputEvents () at xf86Events.c:164
> > #41 0x0000000000430835 in Dispatch () at dispatch.c:353
> > #42 0x0000000000421a45 in main (argc=1, argv=0x7fff049880d8, envp=0x7fff049880e8) at main.c:288
> > 
> > (d) XkbHandleActions() will look at the event type to decide it is a
> >     keyEvent type, query the key action type, perform some operation based
> >     on the key action type, then pass the event into FixKeyState():
> > 
> >     void
> >     XkbHandleActions(DeviceIntPtr dev, DeviceIntPtr kbd, DeviceEvent *event)
> >     {
> >         [...]
> >         keyEvent = ((event->type == ET_KeyPress) || (event->type == ET_KeyRelease));
> >         [...]
> >         act = XkbGetKeyAction(xkbi, &xkbi->state, key);
> >         [...]
> >         switch (act.type) {
> >             [...]
> >             case XkbSA_XFree86Private:
> >                 filter = _XkbNextFreeFilter(xkbi);
> >                 sendEvent = _XkbFilterXF86Private(xkbi, filter, key, &act);
> >                 break;
> >             }
> >         }
> >         [...]
> >         if (keyEvent) {
> >             FixKeyState(event, dev);
> >         }
> >         [...]
> >     }
> > 
> > (e) FixKeyState() does this:
> > 
> >     if (event->type == ET_KeyPress)
> >         set_key_down(keybd, key, KEY_PROCESSED);
> >     else if (event->type == ET_KeyRelease)
> >         set_key_up(keybd, key, KEY_PROCESSED);
> >     else
> >         FatalError("Impossible keyboard event");
> > 
> > (that will be important later...)
> > 
> > (f) Back in XkbHandleActions(), we called _XkbFilterXF86Private which
> >     passes the key event to the XFree86 layer, which decodes the key event as
> >     "+vmode", and we call through the pScrnInfo->SwitchMode wrap chain:
> > 
> > #6  0x00000000004df525 in xf86CursorSwitchMode (index=0, mode=0x1c54eb0, flags=0) at xf86Cursor.c:253
> > #7  0x0000000000496ef1 in CMapSwitchMode (index=0, mode=0x1c54eb0, flags=0) at xf86cmap.c:492
> > #8  0x0000000000485fb1 in xf86SwitchMode (pScreen=0x1c40c00, mode=0x1c54eb0) at xf86Cursor.c:232
> > #9  0x00000000004863f8 in xf86ZoomViewport (pScreen=0x1c40c00, zoom=1) at xf86Cursor.c:332
> > #10 0x000000000048ae8e in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, arg=0x0) at xf86Events.c:191
> > #11 0x00000000004eb36c in XkbDDXPrivate (dev=0x22ac580, key=86 'V', act=0x7ffff8e60770) at xkbPrivate.c:32
> > #12 0x0000000000584746 in _XkbFilterXF86Private (xkbi=0x22ad470, filter=0x2265400, keycode=86,
> >     pAction=0x7ffff8e60770) at xkbActions.c:959
> > #13 0x0000000000585596 in XkbHandleActions (dev=0x22ac580, kbd=0x22ac580, event=0x85cf00)
> >     at xkbActions.c:1234
> > 
> > (g) Someone in the pScrnInfo->SwitchMode wrap chain calls RRTellChanged()
> >     (in my case, it is the NVIDIA X driver, but I think many
> >     drivers could be exposed to this through xf86SetSingleMode() ->
> >     xf86RandR12TellChanged()) which calls UpdateCurrentTime().
> > 
> > (h) UpdateCurrentTime() conditionally calls ProcessInputEvents():
> > 
> >     void
> >     UpdateCurrentTime(void)
> >     {
> >         [...]
> >         if (*checkForInput[0] != *checkForInput[1])
> >             ProcessInputEvents();
> >         [...]
> >     }
> > 
> >     ---
> > 
> >     void
> >     ProcessInputEvents(void)
> >     {
> >         int x, y;
> > 
> >         mieqProcessInputEvents();
> > 
> >         /* FIXME: This is a problem if we have multiple pointers */
> >         miPointerGetPosition(inputInfo.pointer, &x, &y);
> > 
> >         xf86SetViewport(xf86Info.currentScreen, x, y);
> >     }
> > 
> > (i) checkForInput[0] and [1] point to miEventQueue.{head,tail}; and tail
> >     potentially got updated earlier in this sequence:
> > 
> > 0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at mieq.c:322
> > 322         miEventQueue.tail = (oldtail + 1) % miEventQueue.nevents;
> > #0  0x00000000005ae0aa in mieqEnqueue (pDev=0x2309c00, e=0x26e2e20) at mieq.c:322
> > #1  0x00000000005baae7 in miPointerMove (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at mipointer.c:703
> > #2  0x00000000005b9f0c in miPointerWarpCursor (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at mipointer.c:371
> > #3  0x00000000004875e1 in xf86WarpCursor (pDev=0x2309c00, pScreen=0x21dc7f0, x=1600, y=600) at xf86Cursor.c:476
> > #4  0x0000000000486f65 in xf86SwitchMode (pScreen=0x21dc7f0, mode=0x21fac80) at xf86Cursor.c:283
> > #5  0x000000000048712b in xf86ZoomViewport (pScreen=0x21dc7f0, zoom=1) at xf86Cursor.c:333
> > #6  0x000000000048bba2 in xf86ProcessActionEvent (action=ACTION_NEXT_MODE, arg=0x0) at xf86Events.c:191
> > #7  0x00000000004ebed8 in XkbDDXPrivate (dev=0x26bbc00, key=86 'V', act=0x7fff8e43bc70) at xkbPrivate.c:32
> > #8  0x00000000005845ae in _XkbFilterXF86Private (xkbi=0x26bcaf0, filter=0x233b650, keycode=86, pAction=0x7fff8e43bc70)
> >     at xkbActions.c:958
> > #9  0x0000000000585119 in XkbHandleActions (dev=0x26bbc00, kbd=0x26bbc00, event=0x85c500) at xkbActions.c:1194
> > 
> >     So, UpdateCurrentTime() may call ProcessInputEvents(), which will reenter
> >     mieqProcessInputEvents().
> > 
> > (j) mieqProcessInputEvents() will overwrite the contents of the static
> >     'event' variable, such that when we finally get back up to
> >     XkbHandleActions() and it calls FixKeyState(), the contents of
> >     'event' are no longer consistent with the value of keyEvent and we
> >     hit the FatalError() case.
> > 
> > Making the 'event' variable in mieqProcessInputEvents() non-static
> > avoids the FatalError() in FixKeyState(), but is it problematic to
> > have the re-entered instance of mieqProcessInputEvents() process events
> > from miEventQueue before the first instance of mieqProcessInputEvents()
> > has completed processing of its current event?
> 
> good analysis, thanks.  afaict we need a mutex in
> mieqProcessInputEvents(), it's certainly not designed to be reentrant and
> it's pointless anyway - if you're already processing events, why would you
> need to do so again?
> 
> most of the calls to ProcessInputEvents() from the server are intended as
> "process input now instead of going to sleep".
> 
> Cheers,
>   Peter


More information about the xorg-devel mailing list