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(a)linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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