Race condition with Timers and IO Signal handler in xorg-server

[Andrew de los Reyes]

Hi folks,

I think I've found a race condition with Timers in xorg-server. I found this
in the context of using Timers within an Xinput module. As you may know,
Xinput modules handle input on a signal handler which interrupts the main
thread of execution.

Looking at DoTimer in xorg-server's os/WaitFor.c file with some annotations
added:

static void
DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
{
    CARD32 newTime;

    // A
    *prev = timer->next;  // remote this timer from the list
    // B
    timer->next = NULL;  // clean up this timer's next pointer
    // C
    newTime = (*timer->callback)(timer, now, timer->arg);
    // D
    if (newTime)
TimerSet(timer, 0, newTime, timer->callback, timer->arg);
}

Timer callbacks occur on the main thread, but an IO handler can interrupt at
any time and schedule or cancel timers. Many timer handlers that worry about
this will block SIGIO in their callback, so for now let's assume that no
SIGIO will occur between C and D above. Think about the following sequence
of events:

Legend:

Main Thread (not indented)
        SIGIO handler (indented)

Sequence of events:

There is 1 live timer (timer #1), which is firing.
DoTimer() called.
Timer removed from list.
We are at point "B" above
        SIGIO occurs
        Timer #1 is cancelled (noop)
        Timer #1 is rescheduled - inserted into timers list
        SIGIO is complete

        Another module's SIGIO fires
        Timer #2 is scheduled and goes at end of timers list
        SIGIO is complete

(At this point, we have 2 timers in the timers list, the first of which is
inside DoTimer, and we resume the main thread)

Timer #1's next ptr set to NULL

(At this point, Timer #2 is lost from the timers list!)

I'm new to the xorg-server code base, but my naive inclination is that when
reading and writing the timers linked list, SIGIO (and other signals?)
should be blocked. In the example above, if we blocked SIGIO at point A and
unblocked it at point C, this particular race condition would be solved, it
seems. However, I suspect that there are other problematic cases in
WaitFor.c.

Also, to be fair, I haven't seen this problem occur in a real system; I only
noticed it by looking at the code.

Thanks,
-andrew
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110803/eb6992bc/attachment.html>