Re: QUEUE_WaitBits race condition

[Ulrich Weigand]

Alexandre Julliard wrote:
> Ove Kaaven <[EMAIL PROTECTED]> writes:
> 
> > OK, this code may look all fine and dandy, even for sendmessages. But what
> > happens when an optimizing C compiler like gcc gets run on it? Well, it
> > figures that the changeBits are tested at [2], so when it gets to [3], it
> > sees no change, and optimizes [3] out... so, instant race condition.
> 
> It does not make any difference whether the compiler optimizes it out
> or not, you still have the same race. And adding volatile doesn't
> help, the bits can always be changed after they are tested (volatile
> is almost always the wrong fix for a race condition anyway). 

But in this case, I think Ove is basically right: you have only one 
race between QUEUE_SetWakeBit and QUEUE_WaitBits. 

QUEUE_SetWakeBit does basically:

    queue->changeBits |= bit;
    if (queue->wakeMask & bit)
        server_call( REQ_WAKE_QUEUE );

and QUEUE_WaitBits does:

    queue->wakeMask = bits | QS_SENDMESSAGE;
    if (!(queue->changeBits & bits))
        WaitForSingleObject( queue->server_queue, timeout );

To avoid the race, we must guarantee that if QUEUE_WaitBits
runs into WaitForSingleObject, then QUEUE_SetWakeBits must
either have already run into REQ_WAKE_QUEUE or will run into
REQ_WAKE_QUEUE (this is of course also a race, but of no 
importance, as the queue remembers its 'signalled' state).

So, the only important thing is that QUEUE_SetWakeBits
sets the changeBits *before* it tests the wakeMask, and
that QUEUE_WaitBits sets the wakeMask *before* it tests
the changeBits.

However, I don't think that just making the 'queue' volatile
fixes the problem completely, as the accesses to the queue
need also be atomic (the ... |= bits for example).

I think we should just protect access to the wakeMask
by the queue critical section.  Then we need not care about
volatile access or not, we can even remove the second check ...

What do you think about this patch, for example?  (Untested ...)


--- wine-cvs/windows/queue.c    Mon Nov  6 22:24:21 2000
+++ wine-uw/windows/queue.c     Wed Jan  3 21:20:00 2001
@@ -628,6 +628,8 @@
     TRACE_(msg)("queue = %04x (wm=%04x), bit = %04x\n", 
                        queue->self, queue->wakeMask, bit );
 
+    EnterCriticalSection( &queue->cSection );
+
     if (bit & QS_MOUSE) pMouseQueue = queue;
     if (bit & QS_KEY) pKbdQueue = queue;
     queue->changeBits |= bit;
@@ -655,6 +657,8 @@
             SERVER_END_REQ;
         }
     }
+
+    LeaveCriticalSection( &queue->cSection );
 }
 
 
@@ -693,27 +697,29 @@
     
     for (;;)
     {
+        EnterCriticalSection( &queue->cSection );
+
         if (queue->changeBits & bits)
         {
             /* One of the bits is set; we can return */
             queue->wakeMask = 0;
+
+            LeaveCriticalSection( &queue->cSection );
             QUEUE_Unlock( queue );
             return 1;
         }
         if (queue->wakeBits & QS_SENDMESSAGE)
         {
             /* Process the sent message immediately */
-
            queue->wakeMask = 0;
+
+            LeaveCriticalSection( &queue->cSection );
             QUEUE_ReceiveMessage( queue );
            continue;                           /* nested sm crux */
         }
 
         queue->wakeMask = bits | QS_SENDMESSAGE;
-        if(queue->changeBits & bits)
-        {
-            continue;
-        }
+        LeaveCriticalSection( &queue->cSection );
        
        TRACE_(msg)("%04x) wakeMask is %04x, waiting\n", queue->self, queue->wakeMask);
 

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  [EMAIL PROTECTED]