[PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput

[Jeremy Huddleston Sequoia]

> On Sep 19, 2016, at 08:30, Keith Packard <keithp at keithp.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu at apple.com> writes:
> 
>> ==================
>> WARNING: ThreadSanitizer: data race (pid=4943)
>>  Read of size 4 at 0x00010c4e3854 by thread T8:
>>    #0 WaitForSomething WaitFor.c:237 (X11.bin+0x00010049216c)
>>    #1 Dispatch dispatch.c:413 (X11.bin+0x000100352ed9)
>>    #2 dix_main main.c:287 (X11.bin+0x00010036e894)
>>    #3 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63)
>> 
>>  Previous write of size 4 at 0x00010c4e3854 by thread T12 (mutexes: write M856, write M1976):
>>    #0 mieqEnqueue mieq.c:263 (X11.bin+0x000100448d14)
>>    #1 DarwinSendDDXEvent darwinEvents.c:641 (X11.bin+0x000100033613)
>>    #2 DarwinProcessFDAdditionQueue_thread darwinEvents.c:338 (X11.bin+0x000100032039)
> 
> I'm not sure I want to resolve this "bug" -- the event queue has been
> designed to be safe in a lockless threaded environment as it was originally
> designed to directly map a kernel address which would be modified at
> interrupt time. Is there an actual issue with the design?

Yeah, I made the change mainly to shutup the analyzer while I was looking for other races.  I decided to propose it in case we want to be strict here.

I don't see an absolute need to do this.  The code looks perfectly safe to me without this change because the data is only read and compared.  There could be issues with reads being nonatomic and getting back some meaningless value, but the outcome of that failure would just be an extra unneeded call into ProcessInputEvents(), which is perfectly safe.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4465 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160919/1b0757af/attachment-0001.bin>