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

[Matthias Bolte]

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.

>         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