Since the virSecretObjListAdd technically consumes @def on success,
the secretDefineXML should fetch the @def from the object afterwards
and manage as an @objdef and set @def = NULL immediately.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/secret/secret_driver.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 8ddae57..32cd8bb 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,10 @@ secretDefineXML(virConnectPtr conn,
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
+ def = NULL;
+ objdef = virSecretObjGetDef(obj);
- if (!def->isephemeral) {
+ if (!objdef->isephemeral) {
if (backup && backup->isephemeral) {
if (virSecretObjSaveData(obj) < 0)
goto restore_backup;
@@ -248,17 +251,16 @@ 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:
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/03/2017 03:27 PM, John Ferlan wrote: > Since the virSecretObjListAdd technically consumes @def on success, > the secretDefineXML should fetch the @def from the object afterwards > and manage as an @objdef and set @def = NULL immediately. Really? virSecretObjListAdd sets ->def pointer in the object, but it doesn't touch the definition otherwise. So I think a caller is safe to continue using the pointer. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/11/2017 11:52 AM, Michal Privoznik wrote: > On 06/03/2017 03:27 PM, John Ferlan wrote: >> Since the virSecretObjListAdd technically consumes @def on success, >> the secretDefineXML should fetch the @def from the object afterwards >> and manage as an @objdef and set @def = NULL immediately. > > Really? virSecretObjListAdd sets ->def pointer in the object, but it > doesn't touch the definition otherwise. So I think a caller is safe to > continue using the pointer. > > Michal > Let's consider the code before my change... @def is added to the @obj "Something" causes us to jump to the "restore_backup:" label and @backup == NULL. That means virSecretObjListRemove runs and because @obj has @def it ends up calling virSecretDefFree The very next thing we do is call virSecretDefFree on the same @def address. While it is the same thing and I could do a VIR_STEAL_PTR(objdef, def); instead, the patch just follows the pattern I've adopted in other patches where @def = NULL and objdef = vir*ObjGetDef() IDC either way, but to avoid a path where @def could be free'd twice, something needs to be done. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2017 06:54 PM, John Ferlan wrote: > > > On 07/11/2017 11:52 AM, Michal Privoznik wrote: >> On 06/03/2017 03:27 PM, John Ferlan wrote: >>> Since the virSecretObjListAdd technically consumes @def on success, >>> the secretDefineXML should fetch the @def from the object afterwards >>> and manage as an @objdef and set @def = NULL immediately. >> >> Really? virSecretObjListAdd sets ->def pointer in the object, but it >> doesn't touch the definition otherwise. So I think a caller is safe to >> continue using the pointer. >> >> Michal >> > > Let's consider the code before my change... > > @def is added to the @obj > > "Something" causes us to jump to the "restore_backup:" label and @backup > == NULL. > > That means virSecretObjListRemove runs and because @obj has @def it ends > up calling virSecretDefFree The only way this can happen is when @obj has refcnt == 1. Because then unref() calls dispose() which calls virSecretDefFree(). However, @obj will have at least 2 references when entering restore_backup label. In virSecretObjListAdd() when creating the new object via virSecretObjNew() obj has refcnt = 1, and then we ref the object again. But wait a second: if the object is already in both of the hash tables we return that reference and don't increase the refcnt! So in the end, virSecretObjListAdd() can return an object with refcnt == 1 and refcnt == 2. This is obviously wrong and root cause of the problem you are seeing. As I describe in the other e-mail, this breaks refcounting and needs to be fixed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/14/2017 04:26 AM, Michal Privoznik wrote: > On 07/13/2017 06:54 PM, John Ferlan wrote: >> >> >> On 07/11/2017 11:52 AM, Michal Privoznik wrote: >>> On 06/03/2017 03:27 PM, John Ferlan wrote: >>>> Since the virSecretObjListAdd technically consumes @def on success, >>>> the secretDefineXML should fetch the @def from the object afterwards >>>> and manage as an @objdef and set @def = NULL immediately. >>> >>> Really? virSecretObjListAdd sets ->def pointer in the object, but it >>> doesn't touch the definition otherwise. So I think a caller is safe to >>> continue using the pointer. >>> >>> Michal >>> >> >> Let's consider the code before my change... >> >> @def is added to the @obj >> >> "Something" causes us to jump to the "restore_backup:" label and @backup >> == NULL. >> >> That means virSecretObjListRemove runs and because @obj has @def it ends >> up calling virSecretDefFree > > The only way this can happen is when @obj has refcnt == 1. Because then > unref() calls dispose() which calls virSecretDefFree(). However, @obj > will have at least 2 references when entering restore_backup label. In > virSecretObjListAdd() when creating the new object via virSecretObjNew() > obj has refcnt = 1, and then we ref the object again. But wait a second: > if the object is already in both of the hash tables we return that > reference and don't increase the refcnt! So in the end, > virSecretObjListAdd() can return an object with refcnt == 1 and refcnt > == 2. This is obviously wrong and root cause of the problem you are > seeing. As I describe in the other e-mail, this breaks refcounting and > needs to be fixed. > > Michal > Ah - I see what's happened - in my mind I'm already at the next patch where the else has a virObjectUnref(obj) after the ListRemove call and thus falling into cleanup and doing a virSecretDefFree would have been bad if @def was not NULL. I don't understand the "But wait a second: if the object is already in both of the hash tables we return that reference and don't increase the refcnt! " When we leave ObjListAdd - the refcnt should include 1 for New and 1 for the HashTable, so it should be 2. This is where I contend domainobj's have it wonky (or wrong) because if the Remove is called each HashRemove will decrement the refcnt by 1. But all the callers there "know" this and thus "choose" to use just Unlock at times rather than EndAPI. When they use EndAPI, they always will Ref the object prior which IMO causes too much thinking/knowledge of the consumer. I'll go read/respond to your 8/8 reply in a moment - the caffeine is starting to work through the morning haze... I understand you object to the virSecretObjGetDef call as unnecessary; however, what if I use VIR_STEAL_PTR. In the long run it's protection against needing to appropriately place the def = NULL much later in this code because we know the object owns it, but we wanted to use it and not create another temporary. It protects against some future adjustment that doesn't account for @def isn't owned by us and jumps to cleanup free'ing @def when we don't own it. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/14/2017 02:01 PM, John Ferlan wrote: > > > On 07/14/2017 04:26 AM, Michal Privoznik wrote: >> On 07/13/2017 06:54 PM, John Ferlan wrote: >>> >>> >>> On 07/11/2017 11:52 AM, Michal Privoznik wrote: >>>> On 06/03/2017 03:27 PM, John Ferlan wrote: >>>>> Since the virSecretObjListAdd technically consumes @def on success, >>>>> the secretDefineXML should fetch the @def from the object afterwards >>>>> and manage as an @objdef and set @def = NULL immediately. >>>> >>>> Really? virSecretObjListAdd sets ->def pointer in the object, but it >>>> doesn't touch the definition otherwise. So I think a caller is safe to >>>> continue using the pointer. >>>> >>>> Michal >>>> >>> >>> Let's consider the code before my change... >>> >>> @def is added to the @obj >>> >>> "Something" causes us to jump to the "restore_backup:" label and @backup >>> == NULL. >>> >>> That means virSecretObjListRemove runs and because @obj has @def it ends >>> up calling virSecretDefFree >> >> The only way this can happen is when @obj has refcnt == 1. Because then >> unref() calls dispose() which calls virSecretDefFree(). However, @obj >> will have at least 2 references when entering restore_backup label. In >> virSecretObjListAdd() when creating the new object via virSecretObjNew() >> obj has refcnt = 1, and then we ref the object again. But wait a second: >> if the object is already in both of the hash tables we return that >> reference and don't increase the refcnt! So in the end, >> virSecretObjListAdd() can return an object with refcnt == 1 and refcnt >> == 2. This is obviously wrong and root cause of the problem you are >> seeing. As I describe in the other e-mail, this breaks refcounting and >> needs to be fixed. >> >> Michal >> > > Ah - I see what's happened - in my mind I'm already at the next patch > where the else has a virObjectUnref(obj) after the ListRemove call and > thus falling into cleanup and doing a virSecretDefFree would have been > bad if @def was not NULL. > > I don't understand the "But wait a second: if the object is already in > both of the hash tables we return that reference and don't increase the > refcnt! " > > When we leave ObjListAdd - the refcnt should include 1 for New and 1 for > the HashTable, so it should be 2. This is where I contend domainobj's > have it wonky (or wrong) because if the Remove is called each HashRemove > will decrement the refcnt by 1. But all the callers there "know" this > and thus "choose" to use just Unlock at times rather than EndAPI. When > they use EndAPI, they always will Ref the object prior which IMO causes > too much thinking/knowledge of the consumer. Oh, you're right. I misread the code. So the virSecretObjListAdd() should return an object with 3 references. Two are for the two hash tables object is in, third is for the caller to use and later free by calling EndAPI. > > I'll go read/respond to your 8/8 reply in a moment - the caffeine is > starting to work through the morning haze... > > I understand you object to the virSecretObjGetDef call as unnecessary; I don't care that much. I just find it surprising that introducing new variable (which I have to remember anyway when reading the code) is considered as more readable than dereferencing directly. Moreover, the Levensthein distance between the two is just 2 ;-) > however, what if I use VIR_STEAL_PTR. How? > In the long run it's protection > against needing to appropriately place the def = NULL much later in this > code because we know the object owns it, but we wanted to use it and not > create another temporary. It protects against some future adjustment > that doesn't account for @def isn't owned by us and jumps to cleanup > free'ing @def when we don't own it. > > John > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/14/2017 08:26 AM, Michal Privoznik wrote:
> On 07/14/2017 02:01 PM, John Ferlan wrote:
>>
>>
>> On 07/14/2017 04:26 AM, Michal Privoznik wrote:
>>> On 07/13/2017 06:54 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>>>>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>>>>> Since the virSecretObjListAdd technically consumes @def on success,
>>>>>> the secretDefineXML should fetch the @def from the object afterwards
>>>>>> and manage as an @objdef and set @def = NULL immediately.
>>>>>
>>>>> Really? virSecretObjListAdd sets ->def pointer in the object, but it
>>>>> doesn't touch the definition otherwise. So I think a caller is safe to
>>>>> continue using the pointer.
>>>>>
>>>>> Michal
>>>>>
>>>>
>>>> Let's consider the code before my change...
>>>>
>>>> @def is added to the @obj
>>>>
>>>> "Something" causes us to jump to the "restore_backup:" label and @backup
>>>> == NULL.
>>>>
>>>> That means virSecretObjListRemove runs and because @obj has @def it ends
>>>> up calling virSecretDefFree
>>>
>>> The only way this can happen is when @obj has refcnt == 1. Because then
>>> unref() calls dispose() which calls virSecretDefFree(). However, @obj
>>> will have at least 2 references when entering restore_backup label. In
>>> virSecretObjListAdd() when creating the new object via virSecretObjNew()
>>> obj has refcnt = 1, and then we ref the object again. But wait a second:
>>> if the object is already in both of the hash tables we return that
>>> reference and don't increase the refcnt! So in the end,
>>> virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
>>> == 2. This is obviously wrong and root cause of the problem you are
>>> seeing. As I describe in the other e-mail, this breaks refcounting and
>>> needs to be fixed.
Coffee is strong this morning or light has dawned on marblehead.
Let me go back to the "existing" code for a moment where @def isn't set
to NULL when we get to restore_backup and call ObjListRemove
After Add, we have R=2,L=1
After Remove, we have R=1
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.
>>>
>>> Michal
>>>
>>
>> Ah - I see what's happened - in my mind I'm already at the next patch
>> where the else has a virObjectUnref(obj) after the ListRemove call and
>> thus falling into cleanup and doing a virSecretDefFree would have been
>> bad if @def was not NULL.
>>
>> I don't understand the "But wait a second: if the object is already in
>> both of the hash tables we return that reference and don't increase the
>> refcnt! "
>>
>> When we leave ObjListAdd - the refcnt should include 1 for New and 1 for
>> the HashTable, so it should be 2. This is where I contend domainobj's
>> have it wonky (or wrong) because if the Remove is called each HashRemove
>> will decrement the refcnt by 1. But all the callers there "know" this
>> and thus "choose" to use just Unlock at times rather than EndAPI. When
>> they use EndAPI, they always will Ref the object prior which IMO causes
>> too much thinking/knowledge of the consumer.
>
> Oh, you're right. I misread the code. So the virSecretObjListAdd()
> should return an object with 3 references. Two are for the two hash
> tables object is in, third is for the caller to use and later free by
> calling EndAPI.
>
Object is only in one hash table at this point in time, so the return is
ref = 2, lock = 1. One ref is for the object allocation and one ref for
the table.
IMO, when EndAPI is called, it's an indication that the caller is done
with the obj; however, because the obj is in a hash table, the other ref
remains. I've always considered EndAPI an indication that we're done
with our reference to the object whether that was from an Add or a
Lookup, both of which should return with an incremented refcnt and a
locked object. As an aside, Add should also increment for each hash
table we're in because Remove will decrement for each. That's where I
think the domainobj code is wrong.
>>
>> I'll go read/respond to your 8/8 reply in a moment - the caffeine is
>> starting to work through the morning haze...
>>
>> I understand you object to the virSecretObjGetDef call as unnecessary;
>
> I don't care that much. I just find it surprising that introducing new
> variable (which I have to remember anyway when reading the code) is
> considered as more readable than dereferencing directly. Moreover, the
> Levensthein distance between the two is just 2 ;-)
>
As long as someone remain vigilant that @def is set to NULL at some
point in the code after Add but before cleanup:, then we're good. But on
the off chance that it doesn't (which I've shown above), then we're not
in a happy state.
>> however, what if I use VIR_STEAL_PTR.
>
> How?
>
Instead of:
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
def = NULL;
objdef = virSecretObjGetDef(obj);
it'd be
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
VIR_STEAL_PTR(objdef, def);
John
>> In the long run it's protection
>> against needing to appropriately place the def = NULL much later in this
>> code because we know the object owns it, but we wanted to use it and not
>> create another temporary. It protects against some future adjustment
>> that doesn't account for @def isn't owned by us and jumps to cleanup
>> free'ing @def when we don't own it.
>>
>> John
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.