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().
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.
>
> 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.
>
> Michal
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
