[libvirt] [PATCH] virDomainObjListAddLocked: fix double free

[Marc Hartmayer]

On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn at redhat.com> 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 at linux.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay at linux.ibm.com>
>> ---
>>  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