[libvirt] Potential segfault in udev driver

[Matthias Bolte]

2010/1/25 Dave Allan <dallan at redhat.com>:
> 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
>

Here's the patch. Maximilian Wilhelm tested it.

Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: udev-Remove-event-handle-on-shutdown.diff
Type: text/x-diff
Size: 5108 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100126/dc96cc56/attachment-0001.bin>