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

[Dave Allan <dallan@xxxxxxxxxx>]

On 01/27/2010 02:39 PM, Matthias Bolte wrote:
> 2010/1/27 David Allan<dallan@xxxxxxxxxx>:
> > 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list