[libvirt] [PATCH] virDomainObjListAddLocked: fix double free

[Bjoern Walk]

Michal Prívozník <mprivozn at redhat.com> [2018-08-27, 04:03PM +0200]:
> 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

Can we still take this fix for the upcoming release and worry about
doing it right later?

> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
IBM Systems
Linux on Z & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
------------------------------------------------------------------------
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 902 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180829/2d3525b9/attachment-0001.sig>