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

[John Ferlan]

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