xquartz dereferencing a NULL pointer (patch 2)

[Jeremy Huddleston]

On Nov 5, 2008, at 07:34, George Peter Staplin wrote:

>
> On Nov 5, 2008, at 12:33 AM, Jeremy Huddleston wrote:
>
>> The locks inside mieqProcessInputEvents() should do nothing.   
>> mieqProcessInputEvents is the only thread that modifies  
>> miEventQueue.head, and we don't care if miEventQueue.tail changes  
>> out from under us so long as miEventQueue.tail is changed AFTER the  
>> data has been pushed to it (which might actually be the problem).
>>
>
> Your patch seems to fix the problem too.  Are we relying on certain  
> operations being atomic?

That's a good point.  Let's take a look at this in a bit more detail,  
so hopefully I can convince you that we don't actually care about the  
atomicity of this opperation.

mieqProcessInputEvents does a check to see if head != tail in order to  
determine if there are more events to process.  If "tail = newtail"  
and "head != tail" are atomic, this should work fine.  I argue that we  
don't care about "head != tail" itself being atomic, but we do care  
that the value of tail copied into register be valid.  We don't care  
if it's "old" or "new".  If it's "old", then the last event won't be  
processed until the next time mieqProcessInputEvents is called (not  
really a problem).  If it's "new" then we know the event is fully in  
the queue and ready to be processed.  So miEventQueue.tail needs to be  
valid.  Without optimization "miEventQueue.tail = newtail" should be a  
single store-word or copy-word type instruction.  With optimization,  
that instruction would possibly be ignored (for the case when  
(isMotion && isMotion == miEventQueue.lastMotion && oldtail !=  
miEventQueue.head) [see below as this also has a different problem I  
just realized]) or would be atomic.  I guess we run the risk that it  
is optimized as:

newtail = (oldtail + 1) % QUEUE_SIZE;
miEventQueue.tail = newtail;

becoming

miEventQueue.tail++;
miEventQueue.tail |= QUEUE_SIZE - 1;

with the intermediate value actually stored in memory for some  
reason... in that case, the != would still succeed and  
mieqProcessInputEvents would behave correctly EXCEPT if the new  
miEventQueue.tail was now QUEUE_SIZE in which case it would process  
events forever until the other thread got context back to finish the  
mod/store...

You're starting to convince me a little that we need a lock with this  
argument, but it really relies on a compiler being utterly stupid...  
this next case, however, is more persuasive...

So for the case when (isMotion && isMotion == miEventQueue.lastMotion  
&& oldtail != miEventQueue.head), we essentially modify the last item  
already in the queue...  but this is an event that the server thread  
could already be processing  (I think this code might be what caused  
that "must be reentrant with ProcessInputEvents" comment that I  
dismissed earlier).  This is essentially an optimization (combining a  
delta-x1 and delta-x2 mouse motion into a single delta-(x1 + x2) mouse  
motion).  At first glance, we could do something like this:

diff --git a/mi/mieq.c b/mi/mieq.c
index 062dede..ac2eedb 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -167,6 +168,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
      if (isMotion && isMotion == miEventQueue.lastMotion &&
          oldtail != miEventQueue.head) {
  	oldtail = (oldtail - 1) % QUEUE_SIZE;
+
+	miEventQueue.tail = oldtail;
      }
      else {
  	static int stuck = 0;

but that only prevents the mieqProcessInputEvents thread from  
processing the event if it hasn't already done the check.  Since we  
don't have a mutex, we need to assume a context switch could happen  
while the server thread is already in the middle of processing this  
event... I see two possibilities for handling this:

1) Lock inside mieqProcessInputEvents
2) Disable this optimization when we have multiple threads playing  
with mieqEventQueue and just push a second MotionNotify event.

Thoughts?


>
>
> --George
>
>> Try this:
>>
>> diff --git a/mi/mieq.c b/mi/mieq.c
>> index e93d24f..7902608 100644
>> --- a/mi/mieq.c
>> +++ b/mi/mieq.c
>> @@ -113,7 +113,8 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
>> #ifdef XQUARTZ
>>    pthread_mutex_lock(&miEventQueueMutex);
>> #endif
>> -    unsigned int           oldtail = miEventQueue.tail, newtail;
>> +    unsigned int           oldtail = miEventQueue.tail
>> +    unsigned int           newtail = oldtail;
>>    int                    isMotion = 0;
>>    deviceValuator         *v = (deviceValuator *) e;
>>    EventPtr               laste = &miEventQueue.events[(oldtail -  
>> 1) %
>> @@ -173,7 +174,6 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
>> #endif
>>            return;
>>        }
>> -	miEventQueue.tail = newtail;
>>    }
>>
>>    memcpy(&(miEventQueue.events[oldtail].event[0]), e,  
>> sizeof(xEvent));
>> @@ -192,6 +192,7 @@ mieqEnqueue(DeviceIntPtr pDev, xEvent *e)
>>    miEventQueue.events[oldtail].pDev = pDev;
>>
>>    miEventQueue.lastMotion = isMotion;
>> +	miEventQueue.tail = newtail;
>> #ifdef XQUARTZ
>>    pthread_mutex_unlock(&miEventQueueMutex);
>> #endif
>>
>>
>> --Jeremy
>>
>>
>> On Nov 4, 2008, at 22:33, George Peter Staplin wrote:
>>
>>>>
>>>
>>> I have yet another patch.  I think this is better.  The long while  
>>> loop made me miss the forest from the trees.
>>>
>>> Now we lock again at the end of the loop, and unlock on exit of  
>>> the function.  It still works fine in the test cases with this in  
>>> place.
>>>
>>> I'm not sure it's 100% perfect.  I think recursion could introduce  
>>> problems, but it's interesting how it makes the fault go away.   
>>> Perhaps we need a recursive mutex?
>>>
>>> <xserver_Nov_4_mieq_fix-2.patch>
>>>
>>>
>>> /* suspected race area*/
>>> while (miEventQueue.head != miEventQueue.tail) {
>>> /* suspected race area */
>>>    ...
>>>      memcpy(&e, &miEventQueue.events[miEventQueue.head],  
>>> sizeof(EventRec));
>>>      miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
>>>
>>> --George
>>>
>>>
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3040 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg/attachments/20081105/615dbe85/attachment.bin>