[libvirt] Potential segfault in udev driver

Dave Allan dallan at redhat.com
Mon Jan 25 19:35:38 UTC 2010


On 01/25/2010 02:33 PM, Matthias Bolte wrote:
> 2010/1/25 Dave Allan<dallan at 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




More information about the libvir-list mailing list