1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 0/4] More virsecretobj changes to prepare for common object
  4. View patch

[libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver

John Ferlan posted 4 patches 8 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by John Ferlan 8 years, 5 months ago 
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;
     virSecretDefPtr backup = NULL;
     virSecretDefPtr def;
     virObjectEventPtr event = NULL;
@@ -225,8 +226,9 @@ secretDefineXML(virConnectPtr conn,
     if (!(obj = virSecretObjListAdd(driver->secrets, def,
                                     driver->configDir, &backup)))
         goto cleanup;
+    VIR_STEAL_PTR(objdef, def);
 
-    if (!def->isephemeral) {
+    if (!objdef->isephemeral) {
         if (backup && backup->isephemeral) {
             if (virSecretObjSaveData(obj) < 0)
                 goto restore_backup;
@@ -248,22 +250,21 @@ secretDefineXML(virConnectPtr conn,
     /* Saved successfully - drop old values */
     virSecretDefFree(backup);
 
-    event = virSecretEventLifecycleNew(def->uuid,
-                                       def->usage_type,
-                                       def->usage_id,
+    event = virSecretEventLifecycleNew(objdef->uuid,
+                                       objdef->usage_type,
+                                       objdef->usage_id,
                                        VIR_SECRET_EVENT_DEFINED,
                                        0);
 
     ret = virGetSecret(conn,
-                       def->uuid,
-                       def->usage_type,
-                       def->usage_id);
-    def = NULL;
+                       objdef->uuid,
+                       objdef->usage_type,
+                       objdef->usage_id);
     goto cleanup;
 
  restore_backup:
     /* If we have a backup, then secret was defined before, so just restore
-     * the backup. The current def will be handled below.
+     * the backup. The current def will be Free'd below.
      * Otherwise, this is a new secret, thus remove it.
      */
     if (backup)
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by Pavel Hrdina 8 years, 4 months ago 
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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by Pavel Hrdina 8 years, 4 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by John Ferlan 8 years, 4 months ago 

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

> and there is no need to have yet another @objdef.

Unless you want the other condition of having two free's of @def

> 
> Pavel
> 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by Pavel Hrdina 8 years, 4 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by John Ferlan 8 years, 4 months ago 

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

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] secret: Properly handle @def after virSecretObjAdd in driver
Posted by Pavel Hrdina 8 years, 4 months ago 
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>
> > 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.