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

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&gt; > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > --- > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list