[PATCH 1/2] os: Fix buggy integer comparison.

[Alan Coopersmith]

On 03/ 6/11 11:39 AM, Cyril Brulebois wrote:
> -		while (timers && (int) (timers->expires - now) <= 0)
> +		while (timers && (timers->expires <= now))

That's restoring the original X11R6 code and undoing the bugfixes from
X11R6.6/XFree86 days to handle GetTimeInMillis wraparound.

http://www.mail-archive.com/xpert@xfree86.org/msg05383.html has some
information, and even more can be found in this bug reports from the old
closed X.Org database for this problem:

To: xbugs at x.org, patch at xfree86.org
Subject: Xserver: rollover condition in XdmcpWakeupHandler()
VERSION:
  X.Org: R6.6
  XFree86: Current 4.2 CVS tree as of 18-Feb-2002
CLIENT MACHINE and OPERATING SYSTEM:
  HP-UX
DISPLAY TYPE:
  N/A
WINDOW MANAGER:
  N/A
COMPILER:
  ANSI cc on HP-UX
  Others?
AREA:
  Xserver

SYNOPSIS:
  It looks like there is a rollover condition in
  xc/programs/Xserver/os/xdmcp.c:XdmcpWakeupHandler()
  which can cause XDM sessions to be terminated.
DESCRIPTION:
  The XDMCP code calculates its timeout time incorrectly
  when the time wraps from 0xffffffff to 0x00000000.
  The routine XdmcpWakeupHandler() currently has the following
  comparison in its 'else if' condition:
    else if (timeOutTime && GetTimeInMillis() >= timeOutTime)
    {
        if (state == XDM_RUN_SESSION)
        {
            state = XDM_KEEPALIVE;
            send_packet();
        }
        else
            timeout();
    }
  A problem with this test occurs when we wrap back to 0 every 49+
  days.  When this occurs, Xservers with active XDMCP sessions
  will send XDM_KEEPALIVE messages to the XDMCP host continually
  for ~3 minutes.  If the host computer cannot reply to all of the
  keep-alive messages in time, the Xservers seem to assume that the
  host is dead, and the server terminates any XDM sessions.
  Whether or not the session is closed will depend on the network
  load, etc.
  If the test in the 'else if' branch is changed from:
    else if (timeOutTime && GetTimeInMillis() >= timeOutTime)

  to:
    else if (timeOutTime && (int)(GetTimeInMillis() - timeOutTime) >= 0)
  the problem is avoided.  As a result, the XDM_KEEPALIVE
  sessions aren't sent continuously.

REPEAT BY:
You could wait the 49+ days for the rollover to occur.  Or
the following sample program indicates the differences
in the two comparisons used in the 'else if' branch.
This might not reconstruct the problem exactly, but it
should indicate whether or not a compiler generates
code that will hit the problem.
    #include <stdio.h>
    #include <stdlib.h>
    unsigned int timeOutTime, GetTimeInMillis, GetTimeInSec;
    int millis;
    main()
    {
        int test1, test2,  timeOut;
        int n;  int days;

        /* Start with time of day close to the value where
         * (time_of_day_in_sec*1000) overflows an unsigned int.
         */

        GetTimeInSec = 49*24*3600 + 16*3600;  /* 49 days, 16 hours*/
        GetTimeInMillis = 0;
        millis = 0;
        timeOut = 2; /* Always set time out to 2 millisec */
        timeOutTime = GetTimeInMillis + timeOut;
        n = 0;
        for (;;)
        {
            test1=0;
            test2=0;
            if (timeOutTime && GetTimeInMillis >= timeOutTime) {
                /* timeOutTime should always be > GetTimeInMillis */
                test1 = 1;
            }
            if (timeOutTime && (int)(GetTimeInMillis-timeOutTime) >= 0 ) {
                /* timeOutTime should always be > GetTimeInMillis */
                test2 = 1;
            }
            if(test1!=0 || test2!=0) {
                printf('test1 = 0, test2 = 0
', test1, test2);
                if(test1!=0) {
                    printf('Test1 failed when time = 0 sec and 0 millisec
',
                            GetTimeInSec, millis);
                }
                if(test2!=0) {
                    printf('Test2 failed when time = 0 sec and 0 millisec
',
                            GetTimeInSec, millis);
                }
                printf('GetTimeInSec*1000 = 0x0
', GetTimeInSec*1000);
                printf('GetTimeInMillis = 0x0
', GetTimeInMillis);
                printf('timeOutTime = 0x0
', timeOutTime);
                exit(1);
            }
            millis = millis +1;
            if(millis == 1000) {
                GetTimeInSec = GetTimeInSec + 1;
                millis = 0;
            }
            GetTimeInMillis = GetTimeInSec*1000 + millis;
            timeOutTime = GetTimeInMillis + timeOut;
            n++;
            if(n==0x100000) {
                n=0;
                printf('0 0
', GetTimeInMillis, timeOutTime);
            }
        }
    }
  Under HP-UX, the following output occurs:
    ffd68400 ffd68402
    ffe68400 ffe68402
    fff68400 fff68402
    test1 = 1, test2 = 0
    Test1 failed when time = 4294967 sec and 295 millisec
    GetTimeInSec*1000 = 0xfffffed8
    GetTimeInMillis = 0xffffffff
    timeOutTime = 0x1
  This indicates that the two calculations differ every 4294967.295 sec.
  This should be verified on other OSes and other compilers to
  see how general a problem this is.

SAMPLE FIX:
The following diff is against the xdmcp.c file found in
the XFree86 4.2.0 release:
$ diff -c xdmcp.c.v4.2 xdmcp.c.fix
*** xdmcp.c.v4.2        Tue Feb 19 01:21:03 2002
--- xdmcp.c.fix Tue Feb 19 01:21:46 2002
***************
*** 726,732 ****
        if (XFD_ANYSET(&AllClients) && state == XDM_RUN_SESSION)
            timeOutTime = GetTimeInMillis() +  keepaliveDormancy * 1000;
      }
!     else if (timeOutTime && GetTimeInMillis() >= timeOutTime)
      {
        if (state == XDM_RUN_SESSION)
        {
--- 726,732 ----
        if (XFD_ANYSET(&AllClients) && state == XDM_RUN_SESSION)
            timeOutTime = GetTimeInMillis() +  keepaliveDormancy * 1000;
      }
!     else if (timeOutTime && (int)(GetTimeInMillis() - timeOutTime) >= 0)
      {
        if (state == XDM_RUN_SESSION)
        {
Please feel free to contact me with any questions.
-paul

-- 
	-Alan Coopersmith-        alan.coopersmith at oracle.com
	 Oracle Solaris Platform Engineering: X Window System