Re: [libvirt] [PATCH] virDomainObjListAddLocked: fix double free

On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn(a)redhat.com&gt; 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(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? 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.