[libvirt] [PATCH 1/1] Cleanups for udev init code
Dave Allan
dallan at redhat.com
Thu Jan 28 03:55:52 UTC 2010
On 01/27/2010 02:39 PM, Matthias Bolte wrote:
> 2010/1/27 David Allan<dallan at redhat.com>:
>> This patch contains a fix for a segfault when priv is NULL pointed out
>> by Matthias Bolte.
>> ---
>> src/node_device/node_device_udev.c | 42 +++++++++++++++--------------------
>> 1 files changed, 18 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>> index dcd889d..c76c568 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1363,15 +1363,18 @@ static int udevDeviceMonitorShutdown(void)
>> struct udev_monitor *udev_monitor = NULL;
>> struct udev *udev = NULL;
>>
>> - if (driverState) {
>> + if (driverState != NULL) {
>> nodeDeviceLock(driverState);
>>
>> priv = driverState->privateData;
>>
>> - if (priv->watch != -1)
>> - virEventRemoveHandle(priv->watch);
>> + if (priv != NULL) {
>> + if (priv->watch != -1) {
>> + virEventRemoveHandle(priv->watch);
>> + }
>>
>> - udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
>> + udev_monitor = DRV_STATE_UDEV_MONITOR(driverState);
>> + }
>>
>> if (udev_monitor != NULL) {
>> udev = udev_monitor_get_udev(udev_monitor);
>> @@ -1387,6 +1390,7 @@ static int udevDeviceMonitorShutdown(void)
>> virMutexDestroy(&driverState->lock);
>> VIR_FREE(driverState);
>> VIR_FREE(priv);
>> +
>> } else {
>> ret = -1;
>> }
>> @@ -1547,28 +1551,22 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
>> {
>> udevPrivate *priv = NULL;
>> struct udev *udev = NULL;
>> - int ret = 0;
>> + int ret = -1;
>>
>> - if (VIR_ALLOC(priv)< 0) {
>> + if (VIR_ALLOC(driverState)< 0) {
>> virReportOOMError(NULL);
>> - ret = -1;
>> goto out;
>> }
>>
>> - priv->watch = -1;
>> -
>> - if (VIR_ALLOC(driverState)< 0) {
>> + if (VIR_ALLOC(priv)< 0) {
>> virReportOOMError(NULL);
>> - VIR_FREE(priv);
>> - ret = -1;
>> goto out;
>> }
>>
>> + priv->watch = -1;
>> +
>> if (virMutexInit(&driverState->lock)< 0) {
>> VIR_ERROR0("Failed to initialize mutex for driverState");
>> - VIR_FREE(priv);
>> - VIR_FREE(driverState);
>> - ret = -1;
>
> priv does leak here.
>
> priv is a local variable and it is allocated at this point, but not
> assigned to driverState->privateData yet. So goto out will leak it,
> because driverState->privateData is still NULL when calling
> udevDeviceMonitorShutdown to cleanup.
Indeed, you are correct, and I am clearly rushing through this.
>> goto out;
>> }
>>
>> @@ -1585,10 +1583,8 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
>>
>> priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
>> if (priv->udev_monitor == NULL) {
>> - VIR_FREE(priv);
>> nodeDeviceUnlock(driverState);
>> VIR_ERROR0("udev_monitor_new_from_netlink returned NULL");
>> - ret = -1;
>
> priv does leak here too.
>
> As I said before, moving the driverState->privateData = priv
> assignment directly after the allocation and initialization or priv
> will fix the leak, because then the call to udevDeviceMonitorShutdown
> can free it as the local priv is assigned to driverState->privateData.
>
>> goto out;
>> }
>>
>> @@ -1608,27 +1604,25 @@ static int udevDeviceMonitorStartup(int privileged ATTRIBUTE_UNUSED)
>> priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor),
>> VIR_EVENT_HANDLE_READABLE,
>> udevEventHandleCallback, NULL, NULL);
>> +
>> + nodeDeviceUnlock(driverState);
>> +
>> if (priv->watch == -1) {
>> - nodeDeviceUnlock(driverState);
>> - ret = -1;
>> goto out;
>> }
>>
>> - nodeDeviceUnlock(driverState);
>> -
>> /* Create a fictional 'computer' device to root the device tree. */
>> if (udevSetupSystemDev() != 0) {
>> - ret = -1;
>> goto out;
>> }
>>
>> /* Populate with known devices */
>> -
>> if (udevEnumerateDevices(udev) != 0) {
>> - ret = -1;
>> goto out;
>> }
>>
>> + ret = 0;
>> +
>> out:
>> if (ret == -1) {
>> udevDeviceMonitorShutdown();
>> --
>> 1.6.5.5
>>
>
> Matthias
More information about the libvir-list
mailing list