Re: [libvirt] [PATCH 1/1] Cleanups for udev init code

2010/1/27 David Allan&lt;dallan(a)redhat.com&gt;: > 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.

> 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