1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Fix more memory leaks
  4. View patch

[libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it

Michal Privoznik posted 3 patches 6 years, 11 months ago
[libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
Posted by Michal Privoznik 6 years, 11 months ago 
While this leak happens in tests only, it is still worth fixing.

==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331
==12962==    at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
==12962==    by 0x5D285D5: virAlloc (viralloc.c:144)
==12962==    by 0x5E823EB: virCPUGetHost (cpu.c:411)
==12962==    by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874)
==12962==    by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
==12962==    by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
==12962==    by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
==12962==    by 0x13AD0B: virTestMain (testutils.c:1120)
==12962==    by 0x1372FD: main (qemuxml2argvtest.c:2883)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_capabilities.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e2e76e4dd8..9ec9089d5b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
 {
     virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
 
+    virCPUDefFree(cpuData->reported);
+    virCPUDefFree(cpuData->migratable);
+    virCPUDefFree(cpuData->full);
+
     cpuData->reported = reported;
     cpuData->migratable = migratable;
     cpuData->full = full;
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
Posted by Peter Krempa 6 years, 11 months ago 
On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
> While this leak happens in tests only, it is still worth fixing.
> 
> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331
> ==12962==    at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
> ==12962==    by 0x5D285D5: virAlloc (viralloc.c:144)
> ==12962==    by 0x5E823EB: virCPUGetHost (cpu.c:411)
> ==12962==    by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874)
> ==12962==    by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
> ==12962==    by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
> ==12962==    by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
> ==12962==    by 0x13AD0B: virTestMain (testutils.c:1120)
> ==12962==    by 0x1372FD: main (qemuxml2argvtest.c:2883)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e2e76e4dd8..9ec9089d5b 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
>  {
>      virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
>  
> +    virCPUDefFree(cpuData->reported);
> +    virCPUDefFree(cpuData->migratable);
> +    virCPUDefFree(cpuData->full);

This looks fishy. The test code always unrefs the qemuCaps object, so
there should be no leak. Could you please elaborate on how this happens?

My problem is that this is in the setter code.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
Posted by Michal Privoznik 6 years, 11 months ago 
On 05/30/2018 06:57 PM, Peter Krempa wrote:
> On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
>> While this leak happens in tests only, it is still worth fixing.
>>
>> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331
>> ==12962==    at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
>> ==12962==    by 0x5D285D5: virAlloc (viralloc.c:144)
>> ==12962==    by 0x5E823EB: virCPUGetHost (cpu.c:411)
>> ==12962==    by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874)
>> ==12962==    by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
>> ==12962==    by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
>> ==12962==    by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
>> ==12962==    by 0x13AD0B: virTestMain (testutils.c:1120)
>> ==12962==    by 0x1372FD: main (qemuxml2argvtest.c:2883)
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_capabilities.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index e2e76e4dd8..9ec9089d5b 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
>>  {
>>      virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
>>  
>> +    virCPUDefFree(cpuData->reported);
>> +    virCPUDefFree(cpuData->migratable);
>> +    virCPUDefFree(cpuData->full);
> 
> This looks fishy. The test code always unrefs the qemuCaps object, so
> there should be no leak. Could you please elaborate on how this happens?

Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
(which exists only for sake of tests) which does two things:

1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
where cpuData is firstly allocated

2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
whatever was loaded in 1).

> 
> My problem is that this is in the setter code.
> 

Not at all. It's pretty common that setter frees whatever it is
replacing (e.g. virQEMUCapsSetGICCapabilities()).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
Posted by Jiri Denemark 6 years, 11 months ago 
On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
> On 05/30/2018 06:57 PM, Peter Krempa wrote:
> > On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
> >> While this leak happens in tests only, it is still worth fixing.
> >>
> >> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331
> >> ==12962==    at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
> >> ==12962==    by 0x5D285D5: virAlloc (viralloc.c:144)
> >> ==12962==    by 0x5E823EB: virCPUGetHost (cpu.c:411)
> >> ==12962==    by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874)
> >> ==12962==    by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
> >> ==12962==    by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
> >> ==12962==    by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
> >> ==12962==    by 0x13AD0B: virTestMain (testutils.c:1120)
> >> ==12962==    by 0x1372FD: main (qemuxml2argvtest.c:2883)
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_capabilities.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> >> index e2e76e4dd8..9ec9089d5b 100644
> >> --- a/src/qemu/qemu_capabilities.c
> >> +++ b/src/qemu/qemu_capabilities.c
> >> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
> >>  {
> >>      virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
> >>  
> >> +    virCPUDefFree(cpuData->reported);
> >> +    virCPUDefFree(cpuData->migratable);
> >> +    virCPUDefFree(cpuData->full);
> > 
> > This looks fishy. The test code always unrefs the qemuCaps object, so
> > there should be no leak. Could you please elaborate on how this happens?
> 
> Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
> (which exists only for sake of tests) which does two things:
> 
> 1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
> where cpuData is firstly allocated

virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which
sets cpuData->info

> 2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
> whatever was loaded in 1).

while virQEMUCapsInitHostCPUModel fills cpuData->reported,
cpuData->migratable, and cpuData->full using the data from
cpuData->info.

It must be something else which causes the leak.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virQEMUCapsSetHostModel: Free cpuData before setting it
Posted by Michal Privoznik 6 years, 11 months ago 
On 05/31/2018 10:57 AM, Jiri Denemark wrote:
> On Thu, May 31, 2018 at 10:06:37 +0200, Michal Privoznik wrote:
>> On 05/30/2018 06:57 PM, Peter Krempa wrote:
>>> On Wed, May 30, 2018 at 18:04:29 +0200, Michal Privoznik wrote:
>>>> While this leak happens in tests only, it is still worth fixing.
>>>>
>>>> ==12962== 2,035 (104 direct, 1,931 indirect) bytes in 1 blocks are definitely lost in loss record 325 of 331
>>>> ==12962==    at 0x4C2CF26: calloc (vg_replace_malloc.c:711)
>>>> ==12962==    by 0x5D285D5: virAlloc (viralloc.c:144)
>>>> ==12962==    by 0x5E823EB: virCPUGetHost (cpu.c:411)
>>>> ==12962==    by 0x56DE953: virQEMUCapsInitHostCPUModel (qemu_capabilities.c:2874)
>>>> ==12962==    by 0x56E062F: virQEMUCapsLoadCache (qemu_capabilities.c:3435)
>>>> ==12962==    by 0x13802F: qemuTestParseCapabilitiesArch (testutilsqemu.c:500)
>>>> ==12962==    by 0x1371F0: mymain (qemuxml2argvtest.c:2871)
>>>> ==12962==    by 0x13AD0B: virTestMain (testutils.c:1120)
>>>> ==12962==    by 0x1372FD: main (qemuxml2argvtest.c:2883)
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/qemu/qemu_capabilities.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>>>> index e2e76e4dd8..9ec9089d5b 100644
>>>> --- a/src/qemu/qemu_capabilities.c
>>>> +++ b/src/qemu/qemu_capabilities.c
>>>> @@ -1857,6 +1857,10 @@ virQEMUCapsSetHostModel(virQEMUCapsPtr qemuCaps,
>>>>  {
>>>>      virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
>>>>  
>>>> +    virCPUDefFree(cpuData->reported);
>>>> +    virCPUDefFree(cpuData->migratable);
>>>> +    virCPUDefFree(cpuData->full);
>>>
>>> This looks fishy. The test code always unrefs the qemuCaps object, so
>>> there should be no leak. Could you please elaborate on how this happens?
>>
>> Thing is, qemuTestParseCapabilitiesArch() calls virQEMUCapsLoadCache()
>> (which exists only for sake of tests) which does two things:
>>
>> 1) roughly in the middle it calls virQEMUCapsLoadHostCPUModelInfo()
>> where cpuData is firstly allocated
> 
> virQEMUCapsLoadHostCPUModelInfo calls virQEMUCapsSetCPUModelInfo which
> sets cpuData->info
> 
>> 2) at the end it calls virQEMUCapsInitHostCPUModel() which overwrites
>> whatever was loaded in 1).
> 
> while virQEMUCapsInitHostCPUModel fills cpuData->reported,
> cpuData->migratable, and cpuData->full using the data from
> cpuData->info.
> 
> It must be something else which causes the leak.

Ah right. It's different call trace:

Hardware watchpoint 2: -location qemuCaps->kvmCPU->full

Old value = (virCPUDef *) 0x0
New value = (virCPUDef *) 0x5555558f1740
virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555558f0770, migratable=0x5555558f1de0, full=0x5555558f1740) at qemu/qemu_capabilities.c:1863
1863    }
virQEMUCapsSetHostModel 1 $ bt
#0  virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555558f0770, migratable=0x5555558f1de0, full=0x5555558f1740) at qemu/qemu_capabilities.c:1863
#1  0x00007ffff7211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x5555557e7ea0, hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at qemu/qemu_capabilities.c:2903
#2  0x00007ffff7213630 in virQEMUCapsLoadCache (hostArch=VIR_ARCH_X86_64, qemuCaps=0x5555557e7ea0, filename=0x5555557eaef0 "tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at qemu/qemu_capabilities.c:3435
#3  0x0000555555584030 in qemuTestParseCapabilitiesArch (arch=VIR_ARCH_X86_64, capsFile=0x5555557eaef0 "tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml") at testutilsqemu.c:500
#4  0x000055555555fe1b in mymain () at qemuxml2argvtest.c:802
#5  0x0000555555586d0c in virTestMain (argc=1, argv=0x7fffffffd698, func=0x55555555f4f9 <mymain>) at testutils.c:1120
#6  0x00005555555832fe in main (argc=1, argv=0x7fffffffd698) at qemuxml2argvtest.c:2883
virQEMUCapsSetHostModel 1 $ c
Continuing.

Hardware watchpoint 2: -location qemuCaps->kvmCPU->full

Old value = (virCPUDef *) 0x5555558f1740
New value = (virCPUDef *) 0x5555558f1820
virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555557e2ad0, migratable=0x5555558f3190, full=0x5555558f1820) at qemu/qemu_capabilities.c:1863
1863    }
virQEMUCapsSetHostModel 1 $ bt
#0  virQEMUCapsSetHostModel (qemuCaps=0x5555557e7ea0, type=VIR_DOMAIN_VIRT_KVM, reported=0x5555557e2ad0, migratable=0x5555558f3190, full=0x5555558f1820) at qemu/qemu_capabilities.c:1863
#1  0x00007ffff7211acd in virQEMUCapsInitHostCPUModel (qemuCaps=0x5555557e7ea0, hostArch=VIR_ARCH_X86_64, type=VIR_DOMAIN_VIRT_KVM) at qemu/qemu_capabilities.c:2903
#2  0x000055555555eb34 in testUpdateQEMUCaps (info=0x55555579f800 <info>, vm=0x5555557e2eb0, caps=0x5555557e18b0) at qemuxml2argvtest.c:391
#3  0x000055555555f116 in testCompareXMLToArgv (data=0x55555579f800 <info>) at qemuxml2argvtest.c:518
#4  0x0000555555584c50 in virTestRun (title=0x555555588e28 "QEMU XML-2-ARGV genid.x86_64-latest", body=0x55555555ecc3 <testCompareXMLToArgv>, data=0x55555579f800 <info>) at testutils.c:180
#5  0x000055555555fe52 in mymain () at qemuxml2argvtest.c:802
#6  0x0000555555586d0c in virTestMain (argc=1, argv=0x7fffffffd698, func=0x55555555f4f9 <mymain>) at testutils.c:1120
#7  0x00005555555832fe in main (argc=1, argv=0x7fffffffd698) at qemuxml2argvtest.c:2883

So testUpdateQEMUCaps() should free cpuData before calling virQEMUCapsInitHostCPUModel(). I'll prepare a patch that does that.

Michal

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.