From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; 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(a)linux.vnet.ibm.com&gt; --- 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);

On 2013年04月02日 14:47, Osier Yang wrote: > On 02/04/13 13:58, Li Zhang wrote: >> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >> >> 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(a)linux.vnet.ibm.com&gt; >> --- >> 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); > > model is always parsed. So it will be leaked if someone specifies > "model" even the type is "none". Okay, it seems that is not always non-NULL although it is always parsed. My XML file is as: <seclabel type='none'/> It should be better to add if clause before VIR_FREE.

> >> VIR_FREE(def->label); >> VIR_FREE(def->imagelabel); >

On 02/04/13 16:04, Li Zhang wrote: > On 2013年04月02日 15:03, Li Zhang wrote: >> On 2013年04月02日 14:47, Osier Yang wrote: >>> On 02/04/13 13:58, Li Zhang wrote: >>>> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >>>> >>>> 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(a)linux.vnet.ibm.com&gt; >>>> --- >>>> 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); >>> >>> model is always parsed. So it will be leaked if someone specifies >>> "model" even the type is "none". >> Okay, it seems that is not always non-NULL although it is always >> parsed. >> >> My XML file is as: >> <seclabel type='none'/> >> >> It should be better to add if clause before VIR_FREE. >> > Sorry, it seems that this is not right solution to resolve the problem. > free(ptr): ptr can be NULL. > This root cause is because of invalid pointer which is not NULL. > I need to find out the invalid pointer. The solution is not to parse "model" when the seclabel type is "none". I might not be 100% correct. But I think the "model" is just useless for "none". Then your patch is right.

> >>> >>>> VIR_FREE(def->label); >>>> VIR_FREE(def->imagelabel); >>> >> > >

On 02.04.2013 07:58, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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?

Michal

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(a)linux.vnet.ibm.com&gt; >>> >>> 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(a)linux.vnet.ibm.com&gt; >>> --- >>> 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(a)redhat.com&gt; AuthorDate: Thu Mar 28 16:13:01 2013 +0100 Commit: Michal Privoznik <mprivozn(a)redhat.com&gt; 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