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

[Michal Privoznik]

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 at 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