On 3/03/11 17:07 , Ping Cheng wrote: > On Wed, Mar 2, 2011 at 10:06 PM, Peter Hutterer > <peter.hutte...@who-t.net <mailto:peter.hutte...@who-t.net>> wrote: > > There is a small time window where a device may try to send an event > even > though it is not fully setup to send events yet, causing a server crash. > > This window opens when the tool is added to the list of devices in > wcmParseOptions() and closes with the server calling > xf86ActivateDevice(). > If an event for a dependent device is processed during that time, > the tool > will be available but the device pointer is still invalid. > > Crash can be reproduced by putting a breakpoint after > wcmParseOptions() for > the eraser, then generating events with the eraser. These will cause the > tool to dereference tool->device->dev, which is uninitialized. > > Work around this with a simple "enabled" flag that is set whenever > the tool > is actually enabled. > > I didn't test anything yet. As I've shared with you before, I don't > think a local enabled flag is enough. Something common to all the tools > associated with the same port is needed since we should not access the > fd before all tools are ready. > + if (!tool->enabled) { > need to be changed to something like > + if (!common->enabled) { > And commmon->enabled is set to true oly when the last hotplugged tool is > enabled. It will be set to false as soon as one tool is disabled. > Let me know if I've lost you.
is this just a hunch or have you found some other race condition? there is a different between the code before and after this patch. before, we relied on the server to drop events if the device wasn't enabled yet (see GetPointerEvents()). since the device wasn't necessarily set up, this could lead to null-pointer dereferences if the server hadn't finished everything yet. now we control the enabled bit in the driver. Any device that is initialised by us is disabled and will bail out in commonDispatchEvent. when the device is enabled in DEVICE_ON, we also enable it internally, allowing events to complete commonDispatchEvent. There is still a tiny window that is in the server between EnableDevice() and CheckMotion() and I've got a patch to block signals for those few instructions. Mind you, I don't know if an event during this window would crash the server, I think it'd just disappear but I need to test this tomorrow. Either or, that is something that cannot be addressed in the driver at all. As I said in the email, testing of this is hard, you could be running this code for month and never hit the race condition. The only way to make sure it's gone is to look at the code. Cheers, Peter ------------------------------------------------------------------------------ Free Software Download: Index, Search & Analyze Logs and other IT data in Real-Time with Splunk. Collect, index and harness all the fast moving IT data generated by your applications, servers and devices whether physical, virtual or in the cloud. Deliver compliance at lower cost and gain new business insights. http://p.sf.net/sfu/splunk-dev2dev _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
