Re: [PATCH] Correct two memory leaks triggered by udev events

[Dave Allan <dallan@xxxxxxxxxx>]

On Tue, May 25, 2010 at 04:10:27AM +1200, Nigel Jones wrote:
> Hi List,
> 
> Please find below a patch that should correct two memory leaks within
> the udev device handling code.
> 
> The issue is triggered by 'add' udev calls and will slowly cause
> libvirtd to consume the majority of system memory.
> 
> The first part of the patch deals with udevAddOneDevice() and frees
> the memory allocated to 'def' if the function would return -1 (an
> error condition) due to the inability to look up udev properties (for
> example, if the udev device is already removed).
> 
> The second part of the patch deals with a remaining 8kB/cycle memory
> leak, in which the udev device reference isn't released at the end of
> running udevEventHandleCallback().
> 
> I'm happy to answer any questions about the patch, there is also a bit
> more background at
> https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/571093

Hi Nigel,

Thanks for reporting this, and the patch.  While reviewing your patch,
I found an additional leak in the remove case which I have also
addressed in the attached patch, which is otherwise just a slight
modification of yours.  I was able to reproduce the leaks by running a
device add/remove cycle, and I do not see memory leakage after
applying this patch.  Let me know if it fixes it for you as well.

Dave

>From 076666855d9a9c481ae4c983462e0d3afef92fb7 Mon Sep 17 00:00:00 2001
From: David Allan <dallan@xxxxxxxxxx>
Date: Fri, 28 May 2010 22:22:05 -0400
Subject: [PATCH 1/1] Fix leaks in udev device add/remove

* This patch is a modification of a patch submitted by Nigel Jones.
  It fixes several memory leaks on device addition/removal:

1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return
   value is non-zero

2. The node device reference must always be released after the device
   has been processed, so move the call to udev_device_unref into
   udevAddOneDevice and add a call to udev_device_unref to
   udevRemoveOneDevice
---
 src/node_device/node_device_udev.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 6e3ecd7..5193f5b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1211,6 +1211,8 @@ static int udevRemoveOneDevice(struct udev_device *device)
     }
     nodeDeviceUnlock(driverState);

+    udev_device_unref(device);
+
     return ret;
 }

@@ -1309,13 +1311,14 @@ static int udevAddOneDevice(struct udev_device *device)
         goto out;
     }

+    /* If this is a device change, the old definition will be freed
+     * and the current definition will take its place. */
     nodeDeviceLock(driverState);
     dev = virNodeDeviceAssignDef(&driverState->devs, def);
     nodeDeviceUnlock(driverState);

     if (dev == NULL) {
         VIR_ERROR(_("Failed to create device for '%s'"), def->name);
-        virNodeDeviceDefFree(def);
         goto out;
     }

@@ -1324,6 +1327,12 @@ static int udevAddOneDevice(struct udev_device *device)
     ret = 0;

 out:
+    if (ret != 0) {
+        virNodeDeviceDefFree(def);
+    }
+
+    udev_device_unref(device);
+
     return ret;
 }

@@ -1343,7 +1352,6 @@ static int udevProcessDeviceListEntry(struct udev *udev,
             VIR_INFO("Failed to create node device for udev device '%s'",
                      name);
         }
-        udev_device_unref(device);
         ret = 0;
     }

-- 
1.7.0.1

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