[libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver

[John Ferlan]

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
>