On Fri, Jul 14, 2017 at 10:04:39AM -0400, John Ferlan wrote:

Make use of an error: label to handle the failure and need to call virSecretObjEndAPI for the object to set it to NULL for return.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e3bcbe5..bedcdbd 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, { virSecretObjPtr obj; virSecretDefPtr objdef; - virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL;

@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s is already defined for " "use with %s"), uuidstr, objdef->usage_id); - goto cleanup; + goto error; }

if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); - goto cleanup; + goto error; }

if (oldDef) @@ -369,8 +368,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virSecretDefFree(objdef); obj->def = newdef; } else { + /* No existing secret with same UUID, - * try look for matching usage instead */ + * try to look for matching usage instead */ if ((obj = virSecretObjListFindByUsageLocked(secrets, newdef->usage_type, newdef->usage_id))) { @@ -381,7 +381,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s already defined for " "use with %s"), uuidstr, newdef->usage_id); - goto cleanup; + goto error; }

/* Generate the possible configFile and base64File strings @@ -395,7 +395,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, goto cleanup;

if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) - goto cleanup; + goto error;

obj->def = newdef; VIR_STEAL_PTR(obj->configFile, configFile); @@ -403,15 +403,15 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virObjectRef(obj); }

- ret = obj; - obj = NULL; - cleanup: - virSecretObjEndAPI(&obj); VIR_FREE(configFile); VIR_FREE(base64File); virObjectUnlock(secrets); - return ret; + return obj; + + error: + virSecretObjEndAPI(&obj); + goto cleanup;

I don't see what's wrong with the current code, it's commonly used pattern to steal the pointer. The extra error label would make sense if the error path would be more complex, but in this case it doesn't add any value. Pavel

On Tue, Jul 25, 2017 at 01:36:20PM +0200, Pavel Hrdina wrote:

On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:

...

Since the virSecretObjListAdd technically consumes @def on success, the secretDefineXML should set @def = NULL immediately and process the remaining calls using a new @objdef variable. We can use use VIR_STEAL_PTR since we know the Add function just stores @def in obj->def.

This fixes a possible double free of @def if the code jumps to restore_backup: and calls virSecretObjListRemove without setting def = NULL. In this case, the subsequent call to DefFree would succeed and free @def; however, the call to EndAPI would also call DefFree because the Unref done would be the last one for the @obj meaning the obj->def would be used to call DefFree, but it's already been free'd because @def wasn't managed right within this error path.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 30124b4..77351d8 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr obj = NULL; + virSecretDefPtr objdef;

s/objdef/objDef/

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Actually this patch introduces a memory leak in case we restore the @backup definition. virSecretObjListAdd() adds the @def into @obj and sets the old def into @backup, so far good. VIR_STEAL_PTR(objdef, def) leaves the @def == NULL, still good But if we in any point jump to "restore_backup" after the VIR_STEAL_PTR and the @backup != NULL we simply changes the @obj->def back to @backup and the new def is only in @objdef which is not freed anywhere. Possible fix is to set @def = NULL after the virSecretObjListRemove() in restore_backup: ... } else { virSecretObjListRemove(driver->secrets, obj); virObjectUnref(obj); obj = NULL; def = NULL; } ... and there is no need to have yet another @objdef. Pavel

On Tue, Jul 25, 2017 at 08:37:23AM -0400, John Ferlan wrote:

On 07/25/2017 08:21 AM, Pavel Hrdina wrote:

...

On Tue, Jul 25, 2017 at 01:36:20PM +0200, Pavel Hrdina wrote:

...

On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:

...

Since the virSecretObjListAdd technically consumes @def on success, the secretDefineXML should set @def = NULL immediately and process the remaining calls using a new @objdef variable. We can use use VIR_STEAL_PTR since we know the Add function just stores @def in obj->def.

This fixes a possible double free of @def if the code jumps to restore_backup: and calls virSecretObjListRemove without setting def = NULL. In this case, the subsequent call to DefFree would succeed and free @def; however, the call to EndAPI would also call DefFree because the Unref done would be the last one for the @obj meaning the obj->def would be used to call DefFree, but it's already been free'd because @def wasn't managed right within this error path.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 30124b4..77351d8 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr obj = NULL; + virSecretDefPtr objdef;

s/objdef/objDef/

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Actually this patch introduces a memory leak in case we restore the @backup definition.

virSecretObjListAdd() adds the @def into @obj and sets the old def into @backup, so far good.

VIR_STEAL_PTR(objdef, def) leaves the @def == NULL, still good

But if we in any point jump to "restore_backup" after the VIR_STEAL_PTR and the @backup != NULL we simply changes the @obj->def back to @backup and the new def is only in @objdef which is not freed anywhere.

I went through this in the previous review, but for the opposite condition where backup == NULL (repeated for ease)

Without the stealing of objdef, when return from the Add function we have "refcnt=2" and "lock=true".

For some reason we jump to restore_backup and call ObjListRemove which returns and the object refcnt=1 and lock=false.

We fall into cleanup:

Call virSecretDefFree(def) where @def != NULL, so it free's it Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling virSecretDefFree(obj->def)... But wait, we already did that because we allowed the DefFree(def) to free something it didn't own.

...

Possible fix is to set @def = NULL after the virSecretObjListRemove() in restore_backup:

... } else { virSecretObjListRemove(driver->secrets, obj); virObjectUnref(obj); obj = NULL; def = NULL; } ...

The other fix to the leak you point out is:

if (backup) { virSecretObjSetDef(obj, backup); def = objdef; } else { virSecretObjListRemove(driver->secrets, obj); }

That works for me, but still I would like to use objDef. Pavel

On Tue, Jul 25, 2017 at 08:23:04AM -0400, John Ferlan wrote:

On 07/25/2017 07:36 AM, Pavel Hrdina wrote:

...

On Fri, Jul 14, 2017 at 10:04:41AM -0400, John Ferlan wrote:

...

Since the virSecretObjListAdd technically consumes @def on success, the secretDefineXML should set @def = NULL immediately and process the remaining calls using a new @objdef variable. We can use use VIR_STEAL_PTR since we know the Add function just stores @def in obj->def.

This fixes a possible double free of @def if the code jumps to restore_backup: and calls virSecretObjListRemove without setting def = NULL. In this case, the subsequent call to DefFree would succeed and free @def; however, the call to EndAPI would also call DefFree because the Unref done would be the last one for the @obj meaning the obj->def would be used to call DefFree, but it's already been free'd because @def wasn't managed right within this error path.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 30124b4..77351d8 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr obj = NULL; + virSecretDefPtr objdef;

s/objdef/objDef/

Why? I've been using objdef in general and not the camel case one

Well, the naming convention is usually a camelCase or snake_case. In that case other usages of objdef are not correct as well. Yes in this case it's easy to distinguish the two parts on the variable name, but I still thing that camelCase is the preferred form. Pavel

John

...

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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