virstorageobj: Check for duplicates from virStoragePoolObjAssignDef

Even though we do some checking it 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@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>

diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 185822451bd64c7b97e8330462dfaec2b998e0fa..dbc3e3f57bbc5c619ea6127766a4bd553390ce01 100644 (file)
--- 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/uuid mismatch or check_active failure)
  *          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 dd7001c4b2d98d292835bc4f3cd8f17df5344a7b..9fabf34e4fd80ebcbcb7b636c248d28d4675edfe 100644 (file)
--- 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 954ab4b66ca2aa4318f30941c52b34d2aa897ab3..fe518f7325d10557a51e6dc47ebf43f16955bfd1 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1143,7 +1143,6 @@ virStoragePoolObjGetVolumesCount;
 virStoragePoolObjIncrAsyncjobs;
 virStoragePoolObjIsActive;
 virStoragePoolObjIsAutostart;
-virStoragePoolObjIsDuplicate;
 virStoragePoolObjListExport;
 virStoragePoolObjListForEach;
 virStoragePoolObjListNew;

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index c108f026ce37d29f7ae2fe89f2b2e38d75a58ec0..18a67ec8ac0fd08e8d9525389e68629c064a2c4e 100644 (file)
--- 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 6697a7dfe67f52701c086894d528b95e3b997690..c1f31b461cb41f48be3e99ec64a721e03b8fe7fc 100644 (file)
--- 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);