node_device_udev: Set @def to NULL

@def is owned by @obj after adding it the node device object list. As soon as
the @obj lock has been released, another thread could free @obj and therefore
@def. If now someone accesses @def this would lead to a heap-use-after-free and
therefore most likely to a segmentation fault, therefore set @def to NULL after
the ownership has moved.

While at it, add comments to other code places why @def is set to NULL.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 85a8d4e206a863bcc9eca648e5fc189c3395c9de..3a7a02cf15221fb5def456b9317f2f57b405a71c 100644 (file)
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1588,6 +1588,8 @@ udevAddOneDevice(struct udev_device *device)
      * and the current definition will take its place. */
     if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
         goto cleanup;
+    /* @def is now owned by @obj */
+    def = NULL;
     virNodeDeviceObjSetPersistent(obj, persistent);
     virNodeDeviceObjSetAutostart(obj, autostart);
     objdef = virNodeDeviceObjGetDef(obj);
@@ -1983,6 +1985,8 @@ udevSetupSystemDev(void)
     if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
         goto cleanup;
 
+    /* @def is now owned by @obj */
+    def = NULL;
     virNodeDeviceObjSetActive(obj, true);
     virNodeDeviceObjSetAutostart(obj, true);
     virNodeDeviceObjSetPersistent(obj, true);

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index d2d1bc43e3dade772da03111717bc9ec6ece21fd..e93f3f481cd992203615eb99ea85037c390222eb 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7674,8 +7674,9 @@ testNodeDeviceMockCreateVport(testDriver *driver,
 
     if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
         goto cleanup;
-    virNodeDeviceObjSetSkipUpdateCaps(obj, true);
+    /* @def is now owned by @obj */
     def = NULL;
+    virNodeDeviceObjSetSkipUpdateCaps(obj, true);
     objdef = virNodeDeviceObjGetDef(obj);
 
     event = virNodeDeviceEventLifecycleNew(objdef->name,