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

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(a)redhat.com&gt; --- 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