[libvirt] [PATCH 1/1] Fix the crash when seclable is freed

[Michal Privoznik]

On 03.04.2013 04:24, Li Zhang wrote:
> On 2013年04月02日 19:29, Michal Privoznik wrote:
>> On 02.04.2013 07:58, Li Zhang wrote:
>>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>>
>>> When seclabel's type is VIR_DOMAIN_SECLABEL_NONE,
>>> virSecurityLabelDefPtr's members are not allocated.
>>> So it will cause crash when calling VIR_FREE.
>>>
>>> This problem is found when running autotest on PPC.
>>>
>>>   Failed to remove cgroup for virt-tests-vm1
>>>   *** glibc detected *** /usr/sbin/libvirtd: free(): invalid pointer:
>>> 0x00003fff9c187510 ***
>>>   ======= Backtrace: =========
>>>   /lib64/libc.so.6(+0xb89c4)[0x3fffa9bc89c4]
>>>   /lib64/libvirt.so.0(virFree-0x3e2320)[0x3fffaa82e9c0]
>>>   /lib64/libvirt.so.0(virSecurityLabelDefFree-0x378984)[0x3fffaa89d69c]
>>>   /lib64/libvirt.so.0(virDomainDefFree-0x367c98)[0x3fffaa8ae968]
>>>  
>>> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(qemuProcessStop-0xc85f8)[0x3fffa2899d58]
>>>
>>>  
>>> /usr/lib64/libvirt/connection-driver/libvirt_driver_qemu.so(+0xc3668)[0x3fffa28e3668]
>>>
>>>   /lib64/libvirt.so.0(virDomainDestroy-0x309bd0)[0x3fffaa90f6f0]
>>>   /usr/sbin/libvirtd[0x10035230]
>>>  
>>> /lib64/libvirt.so.0(virNetServerProgramDispatch-0x289b50)[0x3fffaa995930]
>>>
>>>   /lib64/libvirt.so.0(+0x20db18)[0x3fffaa98db18]
>>>   /lib64/libvirt.so.0(+0xfbd24)[0x3fffaa87bd24]
>>>   /lib64/libvirt.so.0(+0xfaec8)[0x3fffaa87aec8]
>>>   /lib64/libpthread.so.0(+0xc604)[0x3fffa9d7c604]
>>>   /lib64/libc.so.6(clone-0xb8fe4)[0x3fffa9c3f094]
>>>
>>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>> ---
>>>   src/conf/domain_conf.c |    2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index f3fca7f..2856660 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -1006,6 +1006,8 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr
>>> def)
>>>   {
>>>       if (!def)
>>>           return;
>>> +    if (def->type == VIR_DOMAIN_SECLABEL_NONE)
>>> +        return;
>>>       VIR_FREE(def->model);
>>>       VIR_FREE(def->label);
>>>       VIR_FREE(def->imagelabel);
>>>
>> NACK
>>
>> As you already found out, we are freeing invalid pointers. We need to
>> find out root cause. I wonder where those pointers come from, as
>> VIR_ALLOC(), which is used to alloc a virSecurityLabelDefPtr, fill
>> allocated memory with zeros, so calling VIR_FREE() even for struct
>> members is just fine. Are you able to reproduce this crash? What are the
>> steps?
> 
> I think it is freed twice.
> After the pointer is freed, it will be a wild pointer.
> When freeing it the second time, this error occurs.
> 
> I am trying to reproduce this crash.
> This steps are:
> 1. start libvirtd
> 2. create VM
> #virsh create /etc/libvirt/qemu/virt-tests-vm1.xml
> 
> 3. Run autotest tests
> #cd /DIR/autotest-power
> #./client/autotest-local client/tests/virt/libvirt/control
> 


Did you try the latest release? I've fixed a seclabel double free just
before a while ago:

commit a1c68a1fcbc27fff19e11d0b2a801b416e94366d
Author:     Michal Privoznik <mprivozn at redhat.com>
AuthorDate: Thu Mar 28 16:13:01 2013 +0100
Commit:     Michal Privoznik <mprivozn at redhat.com>
CommitDate: Thu Mar 28 16:13:01 2013 +0100

    security_manager.c: Append seclabel iff generated

    With my previous patches, we unconditionally appended a seclabel,
    even if it wasn't generated but found in array of defined seclabels.
    This resulted in double free later when doing virDomainDefFree
    and iterating over the array of defined seclabels.

    Moreover, there was another possibility of double free, if the
    seclabel was generated in the last iteration of the process of
    walking trough security managers array.


Michal