On 08/27/2018 06:06 PM, Marc Hartmayer wrote:
> On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:
>> On 08/27/2018 03:20 PM, Marc Hartmayer wrote:
>>> If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked
>>> returns NULL (although the definition actually exists). Therefore, the
>>> possibility exits that "virHashAddEntry" will raise the error
>>> "Duplicate key" => virDomainObjListAddObjLocked fails =>
>>> virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def
>>> since @def is already assigned to vm->def. But actually this leads to
>>> a double free since the common usage pattern is that the caller of
>>> virDomainObjListAdd(Locked) is responsible for freeing @def in case of
>>> an error.
>>>
>>> Let's fix this by setting vm->def to NULL in case of an error.
>>>
>>> Backtrace:
>>>
>>> ➤ bt
>>> #0 virFree (ptrptr=0x7575757575757575)
>>> #1 0x000003ffb5b25b3e in virDomainResourceDefFree
>>> #2 0x000003ffb5b37c34 in virDomainDefFree
>>> #3 0x000003ff9123f734 in qemuDomainDefineXMLFlags
>>> #4 0x000003ff9123f7f4 in qemuDomainDefineXML
>>> #5 0x000003ffb5cd2c84 in virDomainDefineXML
>>> #6 0x000000011745aa82 in remoteDispatchDomainDefineXML
>>> ...
>>>
>>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
>>> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
>>> ---
>>> src/conf/virdomainobjlist.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>>> index 52171594f34f..805fe9440afe 100644
>>> --- a/src/conf/virdomainobjlist.c
>>> +++ b/src/conf/virdomainobjlist.c
>>> @@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>>> goto cleanup;
>>> vm->def = def;
>>>
>>> - if (virDomainObjListAddObjLocked(doms, vm) < 0)
>>> + if (virDomainObjListAddObjLocked(doms, vm) < 0) {
>>> + vm->def = NULL;
>>> goto error;
>>> + }
>>> }
>>> cleanup:
>>> return vm;
>>>
>>
>>
>> How about setting vm->def only after virDomainObjListAddObjLocked()
>> succeded?
>
> Unfortunately, this doesn’t work as vm->def->name is used by
> virDomainObjListAddObjLocked().
Ah right.
>
> Another solution that came to my mind is to “hand over” the
> responsibility for @def to virDomainObjListAdd(Locked) (this would
> require that a 'virDomainDefPtr *def' instead of 'virDomainDefPtr def'
> is passed). This would also eliminates the “def = NULL;” lines after a
> successful virDomainObjListAddObj call and it would solve this bug.
Yep, this looks like a good solution. Because we are close to the
release I am going to push this version and what you are suggesting can
then be saved to a follow up patch.
>
>>
>> However, this points to a more sever bug. If a domain is being removed,
>> and some other thread is trying to define it back, I guess the whole
>> operation should succeed. Definitely not fail with some (for user)
>> cryptic message like "double key found in a hash table".
>>
>> The problem is that:
>>
>> a) both virDomainObjListFindByUUIDLocked() and
>> virDomainObjListFindByNameLocked() return NULL even if domain exists.
>> They should return the found domain and only the caller should decide if
>> vm->removing is relevant or not.
>
> Yep, but this is another problem since virDomainObjListAddObjLocked can
> also fail under other circumstances.
>
>>
>> b) nothing is clearing out the vm->removing flag. The
>> virDomainObjListAddObjLocked() looks like a good candidate to do so.
>
> Haven’t looked into much detail for that part.
Okay, I'll look into it. By skimming the code quickly I can see a
potential problem there. But maybe I'm wrong.
ACKed and pushed.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
