[libvirt] [PATCH v2 3/8] virstorageobj: Check for duplicates from virStoragePoolObjAssignDef

Michal Privoznik mprivozn at redhat.com
Mon Aug 20 12:09:46 UTC 2018


Even though we do some checking is not as thorough as it should
be. We already have virStoragePoolObjIsDuplicate but the way we
use it is a typical TOCTOU. Imagine two threads trying to define
two pools with the same name but different UUIDs. With the
current code neither of them finds a duplicate and thus proceed
to virStoragePoolObjAssignDef where only names are compared.
Therefore both threads succeed which is obviously wrong.

We should check for duplicates where we care for them.

Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
---
 src/conf/virstorageobj.c     | 39 +++++++++++++++++++++++++++------------
 src/conf/virstorageobj.h     |  8 ++------
 src/libvirt_private.syms     |  1 -
 src/storage/storage_driver.c | 10 ++--------
 src/test/test_driver.c       | 12 +++---------
 5 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 185822451b..ea0ae6fd86 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1052,22 +1052,28 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
  * @doms : virStoragePoolObjListPtr to search
  * @def  : virStoragePoolDefPtr definition of pool to lookup
  * @check_active: If true, ensure that pool is not active
+ * @objRet: returned pool object
  *
- * Returns: -1 on error
+ * Assumes @pools is locked by caller already.
+ *
+ * Returns: -1 on error (name or UUID mismatch)
  *          0 if pool is new
- *          1 if pool is a duplicate
+ *          1 if pool is a duplicate (name and UUID match)
  */
-int
+static int
 virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
                              virStoragePoolDefPtr def,
-                             bool check_active)
+                             bool check_active,
+                             virStoragePoolObjPtr *objRet)
 {
     int ret = -1;
     virStoragePoolObjPtr obj = NULL;
 
     /* See if a Pool with matching UUID already exists */
-    obj = virStoragePoolObjFindByUUID(pools, def->uuid);
+    obj = virStoragePoolObjFindByUUIDLocked(pools, def->uuid);
     if (obj) {
+        virObjectLock(obj);
+
         /* UUID matches, but if names don't match, refuse it */
         if (STRNEQ(obj->def->name, def->name)) {
             char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1088,11 +1094,14 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
             }
         }
 
+        VIR_STEAL_PTR(*objRet, obj);
         ret = 1;
     } else {
         /* UUID does not match, but if a name matches, refuse it */
-        obj = virStoragePoolObjFindByName(pools, def->name);
+        obj = virStoragePoolObjFindByNameLocked(pools, def->name);
         if (obj) {
+            virObjectLock(obj);
+
             char uuidstr[VIR_UUID_STRING_BUFLEN];
             virUUIDFormat(obj->def->uuid, uuidstr);
             virReportError(VIR_ERR_OPERATION_FAILED,
@@ -1113,6 +1122,7 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
  * virStoragePoolObjAssignDef:
  * @pools: Storage Pool object list pointer
  * @def: Storage pool definition to add or update
+ * @check_active: If true, ensure that pool is not active
  *
  * Lookup the @def to see if it already exists in the @pools in order
  * to either update or add if it does not exist.
@@ -1121,15 +1131,20 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
  */
 virStoragePoolObjPtr
 virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
-                           virStoragePoolDefPtr def)
+                           virStoragePoolDefPtr def,
+                           bool check_active)
 {
-    virStoragePoolObjPtr obj;
+    virStoragePoolObjPtr obj = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
+    int rc;
 
     virObjectRWLockWrite(pools);
 
-    if ((obj = virStoragePoolObjFindByNameLocked(pools, def->name))) {
-        virObjectLock(obj);
+    rc = virStoragePoolObjIsDuplicate(pools, def, check_active, &obj);
+
+    if (rc < 0)
+        goto error;
+    if (rc > 0) {
         if (!virStoragePoolObjIsActive(obj)) {
             virStoragePoolDefFree(obj->def);
             obj->def = def;
@@ -1186,7 +1201,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
         return NULL;
     }
 
-    if (!(obj = virStoragePoolObjAssignDef(pools, def))) {
+    if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) {
         virStoragePoolDefFree(def);
         return NULL;
     }
@@ -1248,7 +1263,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools,
     }
 
     /* create the object */
-    if (!(obj = virStoragePoolObjAssignDef(pools, def)))
+    if (!(obj = virStoragePoolObjAssignDef(pools, def, true)))
         goto error;
 
     /* XXX: future handling of some additional useful status data,
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index dd7001c4b2..9fabf34e4f 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -189,7 +189,8 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
 
 virStoragePoolObjPtr
 virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
-                           virStoragePoolDefPtr def);
+                           virStoragePoolDefPtr def,
+                           bool check_active);
 
 int
 virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
@@ -244,11 +245,6 @@ void
 virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
                         virStoragePoolObjPtr obj);
 
-int
-virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
-                             virStoragePoolDefPtr def,
-                             bool check_active);
-
 int
 virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
                                      virStoragePoolObjListPtr pools,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ca4a192a4a..572d1a1e22 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1140,7 +1140,6 @@ virStoragePoolObjGetVolumesCount;
 virStoragePoolObjIncrAsyncjobs;
 virStoragePoolObjIsActive;
 virStoragePoolObjIsAutostart;
-virStoragePoolObjIsDuplicate;
 virStoragePoolObjListExport;
 virStoragePoolObjListForEach;
 virStoragePoolObjListNew;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index c108f026ce..18a67ec8ac 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -705,16 +705,13 @@ storagePoolCreateXML(virConnectPtr conn,
     if (virStoragePoolCreateXMLEnsureACL(conn, newDef) < 0)
         goto cleanup;
 
-    if (virStoragePoolObjIsDuplicate(driver->pools, newDef, true) < 0)
-        goto cleanup;
-
     if (virStoragePoolObjSourceFindDuplicate(conn, driver->pools, newDef) < 0)
         goto cleanup;
 
     if ((backend = virStorageBackendForType(newDef->type)) == NULL)
         goto cleanup;
 
-    if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
+    if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, true)))
         goto cleanup;
     newDef = NULL;
     def = virStoragePoolObjGetDef(obj);
@@ -799,16 +796,13 @@ storagePoolDefineXML(virConnectPtr conn,
     if (virStoragePoolDefineXMLEnsureACL(conn, newDef) < 0)
         goto cleanup;
 
-    if (virStoragePoolObjIsDuplicate(driver->pools, newDef, false) < 0)
-        goto cleanup;
-
     if (virStoragePoolObjSourceFindDuplicate(conn, driver->pools, newDef) < 0)
         goto cleanup;
 
     if (virStorageBackendForType(newDef->type) == NULL)
         goto cleanup;
 
-    if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
+    if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, false)))
         goto cleanup;
     newDef = virStoragePoolObjGetNewDef(obj);
     def = virStoragePoolObjGetDef(obj);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6697a7dfe6..c1f31b461c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1108,7 +1108,7 @@ testParseStorage(testDriverPtr privconn,
         if (!def)
             goto error;
 
-        if (!(obj = virStoragePoolObjAssignDef(privconn->pools, def))) {
+        if (!(obj = virStoragePoolObjAssignDef(privconn->pools, def, false))) {
             virStoragePoolDefFree(def);
             goto error;
         }
@@ -4515,10 +4515,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
     if (!(newDef = virStoragePoolDefParseString(xml)))
         goto cleanup;
 
-    if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, true) < 0)
-        goto cleanup;
-
-    if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef)))
+    if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, true)))
         goto cleanup;
     newDef = NULL;
     def = virStoragePoolObjGetDef(obj);
@@ -4589,10 +4586,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
     newDef->allocation = defaultPoolAlloc;
     newDef->available = defaultPoolCap - defaultPoolAlloc;
 
-    if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, false) < 0)
-        goto cleanup;
-
-    if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef)))
+    if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, false)))
         goto cleanup;
     newDef = NULL;
     def = virStoragePoolObjGetDef(obj);
-- 
2.16.4




More information about the libvir-list mailing list