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 <[email protected]> > --- > 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
> [email protected]
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
