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