Re: [libvirt] Potential segfault in udev driver

On 01/25/2010 02:33 PM, Matthias Bolte wrote: > > 2010/1/25 Dave Allan<dallan(a)redhat.com>: >> >> On 01/25/2010 06:37 AM, Daniel P. Berrange wrote: >>> >>> On Sun, Jan 24, 2010 at 11:07:59PM +0100, Matthias Bolte wrote: >>>> >>>> udevDeviceMonitorStartup registers udevEventHandleCallback as event >>>> handle, but doesn't store the returned watch id to remove it later on. >>>> Also it's not clear to me whether the event handle should be register >>>> for the whole lifetime of the udev driver instance or just for the >>>> udevEnumerateDevices call. >>> >>> The handler should be active for the lifetime of libvirtd, since the >>> udev driver has to detect hotplug/unplug events over time. >>> >>>> >>>> If for example the call to udevSetupSystemDev [1] fails >>>> udevDeviceMonitorShutdown is called to cleanup, but >>>> udevEventHandleCallback is still registered and may be called when >>>> driverState is NULL again, resulting in a segfault in >>>> udevEventHandleCallback. >>>> >>>> So to solve this the udevEventHandleCallback event handle must be >>>> removed at the appropriate place. >>> >>> Yes, sounds like its needs to be removed in the failure path there >> >> Matthias, >> >> Indeed, that's correct--can you submit a patch? >> >> Dave >> > > Yes, im going to do that. > > One last question. The udev driver stores the udev monitor handle in > the private data pointer of the gloval driver state variable. > >     driverState->privateData = udev_monitor; > > To solve the event handle problem we need to store the watch id > returned by virEventAddHandle somewhere. We could either add a new > private data struct to hold the udev_monitor pointer and the watch id, > or store the watch id variable globally side by side with the driver > state variable. I prefer the first option, because it's cleaner and > the DRV_STATE_UDEV_MONITOR define allows for changing the storage > location of the udev_monitor pointer easily. Yes, I agree--a struct is the right approach.  Thanks for doing this! Dave