[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 4/4] secret: Handle object list removal and deletion properly



On Fri, Jul 14, 2017 at 10:04:42AM -0400, John Ferlan wrote:
> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
> after the call to virSecretObjListRemove, be more explicit by calling
> virObjectUnref and setting @obj to NULL for secretUndefine and in
> the error path of secretDefineXML. Calling EndAPI will end up calling
> Unlock on an already unlocked object which has indeteriminate results
> (usually an ignored error).
> 
> The virSecretObjEndAPI will both Unref and Unlock the object; however,
> the virSecretObjListRemove would have already Unlock'd the object so
> calling Unlock again is incorrect. Once the virSecretObjListRemove
> is called all that's left is to Unref our interest since that's the
> corrollary to the virSecretObjListAdd which returned our ref interest
> plus references for each hash table in which the object resides. In math
> terms, after an Add there's 2 refs on the object (1 for the object and
> 1 for the list). After calling Remove there's just 1 ref on the object.
> For the Add callers, calling EndAPI removes the ref for the object and
> unlocks it, but since it's in a list the other 1 remains.
> 
> This also fixes a leak during virSecretLoad if the virSecretLoadValue
> fails the code jumps to cleanup without setting @ret = obj, thus calling
> virSecretObjListRemove which only accounts for the object reference
> related to adding the object to the list during virSecretObjListAdd,
> but does not account for the reference to the object itself as the
> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
> on the object recently added thus reducing the refcnt to zero. Thus
> cleaning up the virSecretLoadValue error path to make it clearer what
> needs to be done on failure.
> 
> Signed-off-by: John Ferlan <jferlan redhat com>
> ---
>  src/conf/virsecretobj.c    | 14 ++++++--------
>  src/secret/secret_driver.c |  9 +++++++--
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index dd36ce6..0e7675e 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -914,7 +914,6 @@ virSecretLoad(virSecretObjListPtr secrets,
>  {
>      virSecretDefPtr def = NULL;
>      virSecretObjPtr obj = NULL;
> -    virSecretObjPtr ret = NULL;
>  
>      if (!(def = virSecretDefParseFile(path)))
>          goto cleanup;
> @@ -926,16 +925,15 @@ virSecretLoad(virSecretObjListPtr secrets,
>          goto cleanup;
>      def = NULL;
>  
> -    if (virSecretLoadValue(obj) < 0)
> -        goto cleanup;
> -
> -    ret = obj;
> -    obj = NULL;
> +    if (virSecretLoadValue(obj) < 0) {
> +        virSecretObjListRemove(secrets, obj);
> +        virObjectUnref(obj);
> +        obj = NULL;
> +    }
>  
>   cleanup:
> -    virSecretObjListRemove(secrets, obj);
>      virSecretDefFree(def);
> -    return ret;
> +    return obj;
>  }

This should be split into two separate patches, the first part that
fixes virSecretLoad() address memory leak and the second part for
secretDefineXML() and secretUndefine() fixes a double unlock.

Pavel

>  
>  
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 77351d8..19ba6fd 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -267,10 +267,13 @@ secretDefineXML(virConnectPtr conn,
>       * the backup. The current def will be Free'd below.
>       * Otherwise, this is a new secret, thus remove it.
>       */
> -    if (backup)
> +    if (backup) {
>          virSecretObjSetDef(obj, backup);
> -    else
> +    } else {
>          virSecretObjListRemove(driver->secrets, obj);
> +        virObjectUnref(obj);
> +        obj = NULL;
> +    }
>  
>   cleanup:
>      virSecretDefFree(def);
> @@ -410,6 +413,8 @@ secretUndefine(virSecretPtr secret)
>      virSecretObjDeleteData(obj);
>  
>      virSecretObjListRemove(driver->secrets, obj);
> +    virObjectUnref(obj);
> +    obj = NULL;
>  
>      ret = 0;
>  
> -- 
> 2.9.4
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]