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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
