On 6/17/25 5:46 PM, Ján Tomko wrote:
> On a Monday in 2025, Michal Privoznik via Devel wrote:
>> From: Michal Privoznik <mpriv...@redhat.com>
>>
>> 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 <mpriv...@redhat.com>
>> ---
>> 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
Yeah, I know, it's a personal preference.
>
>> + virSecurityLabelDefFree(tmp);
>> goto cleanup;
>> }
>
> Reviewed-by: Ján Tomko <jto...@redhat.com>
Merged, thanks.
Michal
