virDomainObjListAddLocked: fix double free

[libvirt] [PATCH] virDomainObjListAddLocked: fix double free

Posted by Marc Hartmayer 5 years, 7 months ago

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@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@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;
-- 
2.13.4

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

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

Posted by Michal Prívozník 5 years, 7 months ago

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

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

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

Posted by Marc Hartmayer 5 years, 7 months ago

On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn@redhat.com> 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@linux.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay@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?

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.

>
> Michal
>
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

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

Posted by Michal Privoznik 5 years, 7 months ago

On 08/27/2018 06:06 PM, Marc Hartmayer wrote:
> On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn@redhat.com> 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@linux.ibm.com>
>>> Signed-off-by: Marc Hartmayer <mhartmay@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?
> 
> Unfortunately, this doesn’t work as vm->def->name is used by
> virDomainObjListAddObjLocked().

Ah right.

> 
> 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.

Yep, this looks like a good solution. Because we are close to the
release I am going to push this version and what you are suggesting can
then be saved to a follow up patch.

> 
>>
>> 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.

Okay, I'll look into it. By skimming the code quickly I can see a
potential problem there. But maybe I'm wrong.

ACKed and pushed.

Michal

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

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

Posted by Bjoern Walk 5 years, 7 months ago

Michal Prívozník <mprivozn@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@linux.ibm.com>
> > Signed-off-by: Marc Hartmayer <mhartmay@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@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 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list