[libvirt] [PATCH] virDomainObjListAddLocked: fix double free

[Michal Prívozník]

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?

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.

b) nothing is clearing out the vm->removing flag. The
virDomainObjListAddObjLocked() looks like a good candidate to do so.

Michal