External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-23 06:05:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

virstorageobj: Check for duplicates from virStoragePoolObjAssignDef

Browse Source 
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>
This commit is contained in:
Michal Privoznik 2018-08-17 12:30:08 +02:00
parent db867c6bfd
commit 4391b5222f
5 changed files with 34 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
src/conf/virstorageobj.c
View File

@ -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,

8
src/conf/virstorageobj.h
View File

@ -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,

1
src/libvirt_private.syms
View File

@ -1143,7 +1143,6 @@ virStoragePoolObjGetVolumesCount;
virStoragePoolObjIncrAsyncjobs;
virStoragePoolObjIsActive;
virStoragePoolObjIsAutostart;
virStoragePoolObjIsDuplicate;
virStoragePoolObjListExport;
virStoragePoolObjListForEach;
virStoragePoolObjListNew;

10
src/storage/storage_driver.c
View File

@ -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);

12
src/test/test_driver.c
View File

@ -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);