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

[John Ferlan]

On 08/20/2018 08:09 AM, Michal Privoznik wrote:
> Even though we do some checking is not as thorough as it should

s/is not/it is not/

> 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(-)
> 

After seeing patch 4, would it just be worthwhile to merge it into patch
2. There's 405 lines from the virStoragePoolObjAssignDef comments to
that could move to just above virStoragePoolObjMatch. IDC either way,
but since the patches that follow work off it it...

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

-1 on error (name/uuid mismatch *or* check_active failure)

and /me wonders if we should move the check_active failure to the caller

>   *          0 if pool is new
> - *          1 if pool is a duplicate
> + *          1 if pool is a duplicate (name and UUID match)

"Implied" -1 and 0 return means obj == NULL, while 1 means obj is set.

>   */
> -int
> +static int
>  virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,

I think we should change the name to be

    virStoragePoolObjAssignFindPool

or something similar since "IsDuplicate" probably should have returned
true/false only given other libvirt method naming schemes. I'd be OK
doing it all in one change too.


Reviewed-by: John Ferlan <jferlan at redhat.com>

John