Re: [PATCH] security_manager: Don't leak seclabel in virSecurityManagerGenLabel()

On a Monday in 2025, Michal Privoznik via Devel wrote: > From: Michal Privoznik <mprivozn(a)redhat.com&gt; > > When a domain is being started, seclabels are generated for it. > This is handled in virSecurityManagerGenLabel() which can either > find pre-existing seclabel in domain def or generate a new one. > At any rate, domainGenSecurityLabel() callback is called and if > it fails then the seclabel is removed from domain definition > using VIR_DELETE_ELEMENT(). While this shrinks down the seclabels > array, it does not free individual item. It has to be freed > manually. > > 80 bytes in 2 blocks are definitely lost in loss record 1,359 of 1,876 >   at 0x484CEF3: calloc (vg_replace_malloc.c:1675) >   by 0x4F19B29: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8200.5) >   by 0x49E4953: virSecurityLabelDefNew (virseclabel.c:59) >   by 0x4BDE0A4: virSecurityManagerGenLabel (security_manager.c:638) >   by 0xBA029B7: qemuProcessPrepareDomain (qemu_process.c:6760) >   by 0xBA07DF2: qemuProcessStart (qemu_process.c:8369) >   by 0xB93DAC0: qemuDomainObjStart (qemu_driver.c:6371) >   by 0xB93DE08: qemuDomainCreateWithFlags (qemu_driver.c:6420) >   by 0xB93DE86: qemuDomainCreate (qemu_driver.c:6438) >   by 0x4CECEA8: virDomainCreate (libvirt-domain.c:7142) > > Now, you might think this may lead to a double free, because > @seclabel is freed under the 'cleanup' label (if @generated is > true). But if @generated is true, then just before calling the > callback there's VIR_APPEND_ELEMENT() which clears @seclabel out > turning the free under 'cleanup' label into a NOP. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/security/security_manager.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/security/security_manager.c b/src/security/ > security_manager.c > index c2460eae37..5fc4eb4872 100644 > --- a/src/security/security_manager.c > +++ b/src/security/security_manager.c > @@ -669,9 +669,14 @@ virSecurityManagerGenLabel(virSecurityManager *mgr, >                 VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, > seclabel); > >             if (sec_managers[i]->drv- > >domainGenSecurityLabel(sec_managers[i], vm) < 0) { > +                virSecurityLabelDef *tmp = vm->seclabels[vm- > >nseclabels - 1]; > + >                 if (VIR_DELETE_ELEMENT(vm->seclabels, > -                                       vm->nseclabels -1, vm- > >nseclabels) < 0) > +                                       vm->nseclabels - 1, vm- > >nseclabels) < 0) { >                     vm->nseclabels--; > +                } > + The braces are not necessary in this case: https://libvirt.org/coding-style.html#curly-braces

> +                virSecurityLabelDefFree(tmp); >                 goto cleanup; >             } Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt;