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?
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
