Bug 769517

Created attachment 548986 [details]
valgrind details

Patch on upstream:
https://www.redhat.com/archives/libvir-list/2011-December/msg00901.html

Hello Dave, in fact, Eric has committed patch to fix this issue on upstream.

(In reply to comment #4)
> Hello Dave, in fact, Eric has committed patch to fix this issue on upstream.

Thanks Alex, would you mind commenting with the commit log?

> Thanks Alex, would you mind commenting with the commit log?

Welcome, however, nobody reviews the patch at present, I will add commit log once the patch is ACKed and pushed. so the bug should be ASSIGNED status not POST now.

Patch has been ACKed and pushed on upstream, so move to 'POST':

commit e957b670613cdc2de5d84b806bb0432b63c990c0
Author: Eric Blake <eblake>
Date:   Thu Dec 22 16:28:04 2011 +0800

    daemon: clean up daemonization
    
    Valgrind detected a pipe fd leak before the parent exits on success,
    introduced in commit 4296cea; by itself, the leak is not bad, since
    we immediately called _exit(), but we might as well be clean to make
    valgrind analysis easier.  Meanwhile, if the daemon grandchild detects
    an error, the parent failed to flush the error message before exiting.
    Also, we had the possibility of both parent and child returning to the
    caller, such that the user could see duplicated reports of failure
    from the two return paths.  And we might as well be robust to the
    (unlikely) situation of being started with stdin closed.
    
    * daemon/libvirtd.c (daemonForkIntoBackground): Use exit if an
    error message was generated, avoid fd leaks for valgrind's sake,
    avoid returning to caller in both parent and child, and don't
    close a just-dup'd stdin.
    Based on a report by Alex Jia.

(In reply to comment #7)
> Patch has been ACKed and pushed on upstream, so move to 'POST':
> 
> commit e957b670613cdc2de5d84b806bb0432b63c990c0

Sorry, please ignore the above Comment 7, this commit should fix bug 769784, and Eric's patch still need to been ACKed on upstream:

https://www.redhat.com/archives/libvir-list/2011-December/msg00901.html

More memory leaks on libvirt daemon:

==29248== 18 bytes in 1 blocks are definitely lost in loss record 408 of 1,473
==29248==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==29248==    by 0x3B7267F6E1: strdup (in /lib64/libc-2.12.so)
==29248==    by 0x3B8B260992: virCopyError (virterror.c:248)
==29248==    by 0x3B8B260CA8: virCopyLastError (virterror.c:345)
==29248==    by 0x494151: qemuMonitorIO (qemu_monitor.c:589)
==29248==    by 0x3B8B245EBE: virEventPollRunOnce (event_poll.c:490)
==29248==    by 0x3B8B244C66: virEventRunDefaultImpl (event.c:247)
==29248==    by 0x3B8B306B8C: virNetServerRun (virnetserver.c:736)
==29248==    by 0x420C18: main (libvirtd.c:1602)


==10193== 1,024 bytes in 1 blocks are definitely lost in loss record 1,346 of 1,492
==10193==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==10193==    by 0x3B8B24D65B: virAllocN (memory.c:129)
==10193==    by 0x3B8B26E028: virDomainVcpuPinAdd (domain_conf.c:9679)
==10193==    by 0x45549B: qemudDomainPinVcpuFlags (qemu_driver.c:3462)
==10193==    by 0x3B8B2C9F23: virDomainPinVcpu (libvirt.c:8415)
==10193==    by 0x4355AB: remoteDispatchDomainPinVcpuHelper (remote_dispatch.h:3595)
==10193==    by 0x3B8B306334: virNetServerProgramDispatch (virnetserverprogram.c:416)
==10193==    by 0x3B8B3075C0: virNetServerHandleJob (virnetserver.c:164)
==10193==    by 0x3B8B25758B: virThreadPoolWorker (threadpool.c:144)
==10193==    by 0x3B8B256EA1: virThreadHelper (threads-pthread.c:157)
==10193==    by 0x3B72A077F0: start_thread (in /lib64/libpthread-2.12.so)
==10193==    by 0x3B726E570C: clone (in /lib64/libc-2.12.so)


==10193== 2,560 bytes in 20 blocks are definitely lost in loss record 1,411 of 1,492
==10193==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==10193==    by 0x3B8B25181C: virProcessInfoGetAffinity (processinfo.c:120)
==10193==    by 0x454D2A: qemudDomainGetVcpus (qemu_driver.c:3671)
==10193==    by 0x3B8B2C9676: virDomainGetVcpus (libvirt.c:8636)
==10193==    by 0x435CB7: remoteDispatchDomainGetVcpusHelper (remote.c:1412)
==10193==    by 0x3B8B306334: virNetServerProgramDispatch (virnetserverprogram.c:416)
==10193==    by 0x3B8B3075C0: virNetServerHandleJob (virnetserver.c:164)
==10193==    by 0x3B8B25758B: virThreadPoolWorker (threadpool.c:144)
==10193==    by 0x3B8B256EA1: virThreadHelper (threads-pthread.c:157)
==10193==    by 0x3B72A077F0: start_thread (in /lib64/libpthread-2.12.so)
==10193==    by 0x3B726E570C: clone (in /lib64/libc-2.12.so)


==14704== 323 (176 direct, 147 indirect) bytes in 1 blocks are definitely lost in loss record 1,203 of 1,489
==14704==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==14704==    by 0x3B8B24D67D: virAlloc (memory.c:101)
==14704==    by 0x3B8B2440A1: virCgroupNew (cgroup.c:605)
==14704==    by 0x3B8B24487B: virCgroupForDomain (cgroup.c:869)
==14704==    by 0x479D14: qemuDomainDetachPciDiskDevice (qemu_hotplug.c:1522)
==14704==    by 0x4537F7: qemuDomainModifyDeviceFlags (qemu_driver.c:5123)
==14704==    by 0x3B8B2C8B0F: virDomainDetachDevice (libvirt.c:8941)
==14704==    by 0x436895: remoteDispatchDomainDetachDeviceHelper (remote_dispatch.h:1109)
==14704==    by 0x3B8B306334: virNetServerProgramDispatch (virnetserverprogram.c:416)
==14704==    by 0x3B8B3075C0: virNetServerHandleJob (virnetserver.c:164)
==14704==    by 0x3B8B25758B: virThreadPoolWorker (threadpool.c:144)
==14704==    by 0x3B8B256EA1: virThreadHelper (threads-pthread.c:157)

commit 927cfaf467a1eb5d2a0fea3a262ea9b212440c91
Author: Eric Blake <eblake>
Date:   Tue Dec 20 15:06:47 2011 -0700

    threads: check for failure to set thread-local value
    
    We had a memory leak on a very arcane OOM situation (unlikely to ever
    hit in practice, but who knows if libvirt.so would ever be linked
    into some other program that exhausts all thread-local storage keys?).
    I found it by code inspection, while analyzing a valgrind report
    generated by Alex Jia.
    
    * src/util/threads.h (virThreadLocalSet): Alter signature.
    * src/util/threads-pthread.c (virThreadHelper): Reduce allocation
    lifetime.
    (virThreadLocalSet): Detect failure.
    * src/util/threads-win32.c (virThreadLocalSet): Likewise.
    (virCondWait): Fix caller.
    * src/util/virterror.c (virLastErrorObject): Likewise.

Still need to fix new memory leaks.

The leaks above seems to be fixed from a manual review perspective, and those
results were gathered a long time ago, so redoing the valgrind test with
a recent version like 0.9.10-4 would be helpful :-)

  thanks,

Daniel

(In reply to comment #11)
> The leaks above seems to be fixed from a manual review perspective, and those
> results were gathered a long time ago, so redoing the valgrind test with
> a recent version like 0.9.10-4 would be helpful :-)

Hello Daniel,

Other leaks are okay on libvirt-0.9.10-4.el6 except the following leak:

 ==18905== 80 bytes in 1 blocks are definitely lost in loss record 945 of 1,615
==18905==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==18905==    by 0x31A9C4E6FD: virAlloc (memory.c:101)
==18905==    by 0x31A9C61837: virLastErrorObject (virterror.c:268)
==18905==    by 0x31A9C626D8: virResetLastError (virterror.c:419)
==18905==    by 0x31A9CC9479: virConnectNumOfDomains (libvirt.c:1876)
==18905==    by 0x435B82: remoteDispatchNumOfDomainsHelper (remote_dispatch.h:9910)
==18905==    by 0x31A9D141F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==18905==    by 0x31A9D12FF0: virNetServerHandleJob (virnetserver.c:164)
==18905==    by 0x31A9C5866B: virThreadPoolWorker (threadpool.c:144)
==18905==    by 0x31A9C57F88: virThreadHelper (threads-pthread.c:161)
==18905==    by 0x38BAA077F0: start_thread (in /lib64/libpthread-2.12.so)
==18905==    by 0x33F68E570C: clone (in /lib64/libc-2.12.so)


Regards,
Alex

(In reply to comment #12)
> (In reply to comment #11)
> > The leaks above seems to be fixed from a manual review perspective, and those
> > results were gathered a long time ago, so redoing the valgrind test with
> > a recent version like 0.9.10-4 would be helpful :-)
> 
> Hello Daniel,
> 
> Other leaks are okay on libvirt-0.9.10-4.el6 except the following leak:

And also found this leak again:

==322== 384 bytes in 3 blocks are definitely lost in loss record 2,348 of 2,824
==322==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==322==    by 0x4E6C7CC: virProcessInfoGetAffinity (processinfo.c:120)
==322==    by 0x457C3A: qemudDomainGetVcpus (qemu_driver.c:3764)
==322==    by 0x4EED1A6: virDomainGetVcpus (libvirt.c:8810)
==322==    by 0x438237: remoteDispatchDomainGetVcpusHelper (remote.c:1418)
==322==    by 0x4F2E1F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==322==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==322==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)
==322==    by 0x4E71F88: virThreadHelper (threads-pthread.c:161)
==322==    by 0x3EB74077F0: start_thread (in /lib64/libpthread-2.12.so)
==322==    by 0x3EB70E570C: clone (in /lib64/libc-2.12.so)

Memory leaks on 0.9.10-4(cont 1):

==322== Thread 1:
==322== 4 bytes in 1 blocks are definitely lost in loss record 91 of 2,824
==322==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==322==    by 0x4E686DB: virAllocN (memory.c:129)
==322==    by 0x4F2E1AF: virNetServerProgramDispatch (virnetserverprogram.c:398)
==322==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==322==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)
==322==    by 0x4E71F88: virThreadHelper (threads-pthread.c:161)
==322==    by 0x3EB74077F0: start_thread (in /lib64/libpthread-2.12.so)
==322==    by 0x3EB70E570C: clone (in /lib64/libc-2.12.so)

==322== 36 bytes in 1 blocks are definitely lost in loss record 1,095 of 2,824
==322==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==322==    by 0x3EB707F6E1: strdup (in /lib64/libc-2.12.so)
==322==    by 0x3EB8413F91: selinux_trans_to_raw_context (in /lib64/libselinux.so.1)
==322==    by 0x3EB8413967: setfilecon (in /lib64/libselinux.so.1)
==322==    by 0x4F84252: SELinuxSetFileconHelper (security_selinux.c:428)
==322==    by 0x4F84522: SELinuxRestoreSecurityFileLabel (security_selinux.c:483)
==322==    by 0x4F84A70: SELinuxRestoreSecurityAllLabel (security_selinux.c:1030)
==322==    by 0x4F82388: virSecurityStackRestoreSecurityAllLabel (security_stack.c:267)
==322==    by 0x488D7C: qemuProcessStop (qemu_process.c:3701)
==322==    by 0x45B56A: qemuDomainDestroyFlags (qemu_driver.c:1819)
==322==    by 0x4EF40D6: virDomainDestroy (libvirt.c:2222)
==322==    by 0x438F41: remoteDispatchDomainDestroyHelper (remote_dispatch.h:1063)

==322== 45 bytes in 1 blocks are definitely lost in loss record 1,340 of 2,824
==322==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==322==    by 0x3EB707F6E1: strdup (in /lib64/libc-2.12.so)
==322==    by 0x3EB8414468: selinux_raw_to_trans_context (in /lib64/libselinux.so.1)
==322==    by 0x3EB840B419: fgetfilecon (in /lib64/libselinux.so.1)
==322==    by 0x4BF646: virStorageBackendUpdateVolTargetInfoFD (storage_backend.c:1210)
==322==    by 0x4C3244: virStorageBackendFileSystemRefresh (storage_backend_fs.c:88)
==322==    by 0x4B9F0C: storagePoolStart (storage_driver.c:706)
==322==    by 0x4EDD951: virStoragePoolCreate (libvirt.c:11795)
==322==    by 0x432DB4: remoteDispatchStoragePoolCreateHelper (remote_dispatch.h:10876)
==322==    by 0x4F2E1F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==322==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==322==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)

==322== 262,296 (32 direct, 262,264 indirect) bytes in 1 blocks are definitely lost in loss record 2,823 of 2,824
==322==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==322==    by 0x4E686FD: virAlloc (memory.c:101)
==322==    by 0x4E72372: virThreadPoolSendJob (threadpool.c:322)
==322==    by 0x4F2CC9A: virNetServerDispatchNewMessage (virnetserver.c:223)
==322==    by 0x4F2F6C1: virNetServerClientDispatchRead (virnetserverclient.c:911)
==322==    by 0x4F2F7C9: virNetServerClientDispatchEvent (virnetserverclient.c:1086)
==322==    by 0x4E61B1E: virEventPollRunOnce (event_poll.c:490)
==322==    by 0x4E608C6: virEventRunDefaultImpl (event.c:247)
==322==    by 0x4F2C5BC: virNetServerRun (virnetserver.c:736)
==322==    by 0x421D65: main (libvirtd.c:1609)


==3496== 67 (48 direct, 19 indirect) bytes in 1 blocks are definitely lost in loss record 1,617 of 2,841
==3496==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==3496==    by 0x4E686FD: virAlloc (memory.c:101)
==3496==    by 0x4ED3D18: virGetDomain (datatypes.c:191)
==3496==    by 0x422CA2: get_nonnull_domain (remote.c:3671)
==3496==    by 0x434BAF: remoteDispatchDomainBlockStatsHelper (remote_dispatch.h:718)
==3496==    by 0x4F2E1F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==3496==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==3496==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)
==3496==    by 0x4E71F88: virThreadHelper (threads-pthread.c:161)
==3496==    by 0x3EB74077F0: start_thread (in /lib64/libpthread-2.12.so)
==3496==    by 0x3EB70E570C: clone (in /lib64/libc-2.12.so)


==11931== 1,024 bytes in 1 blocks are definitely lost in loss record 2,575 of 2,825
==11931==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==11931==    by 0x4E686DB: virAllocN (memory.c:129)
==11931==    by 0x4E8E418: virDomainVcpuPinAdd (domain_conf.c:10172)
==11931==    by 0x4580CB: qemudDomainPinVcpuFlags (qemu_driver.c:3554)
==11931==    by 0x4EEDA53: virDomainPinVcpu (libvirt.c:8589)
==11931==    by 0x437B2B: remoteDispatchDomainPinVcpuHelper (remote_dispatch.h:3756)
==11931==    by 0x4F2E1F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==11931==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==11931==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)
==11931==    by 0x4E71F88: virThreadHelper (threads-pthread.c:161)
==11931==    by 0x3EB74077F0: start_thread (in /lib64/libpthread-2.12.so)
==11931==    by 0x3EB70E570C: clone (in /lib64/libc-2.12.so)


==16462== 323 (176 direct, 147 indirect) bytes in 1 blocks are definitely lost in loss record 2,328 of 2,835
==16462==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==16462==    by 0x4E686FD: virAlloc (memory.c:101)
==16462==    by 0x4E5FD01: virCgroupNew (cgroup.c:606)
==16462==    by 0x4E604DB: virCgroupForDomain (cgroup.c:870)
==16462==    by 0x4802D4: qemuDomainDetachPciDiskDevice (qemu_hotplug.c:1545)
==16462==    by 0x456697: qemuDomainModifyDeviceFlags (qemu_driver.c:5221)
==16462==    by 0x4EEC3BF: virDomainDetachDevice (libvirt.c:9293)
==16462==    by 0x438E15: remoteDispatchDomainDetachDeviceHelper (remote_dispatch.h:1167)
==16462==    by 0x4F2E1F4: virNetServerProgramDispatch (virnetserverprogram.c:416)
==16462==    by 0x4F2CFF0: virNetServerHandleJob (virnetserver.c:164)
==16462==    by 0x4E7266B: virThreadPoolWorker (threadpool.c:144)
==16462==    by 0x4E71F88: virThreadHelper (threads-pthread.c:161)


==25975== 54 bytes in 1 blocks are definitely lost in loss record 1,287 of 2,532
==25975==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==25975==    by 0x3EB707F6E1: strdup (in /lib64/libc-2.12.so)
==25975==    by 0x4E7C482: virCopyError (virterror.c:246)
==25975==    by 0x4E7C878: virCopyLastError (virterror.c:346)
==25975==    by 0x49B501: qemuMonitorIO (qemu_monitor.c:603)
==25975==    by 0x4E61B1E: virEventPollRunOnce (event_poll.c:490)
==25975==    by 0x4E608C6: virEventRunDefaultImpl (event.c:247)
==25975==    by 0x4F2C5BC: virNetServerRun (virnetserver.c:736)
==25975==    by 0x421D65: main (libvirtd.c:1609)


And will continue to add others in here.

This patch tries to fix a small nit:

https://www.redhat.com/archives/libvir-list/2012-March/msg00674.html

However I am not backporting it for now, because we might want to backport all subsequent patches at once.

commit a4650316d104ca559101d2944a9ecb43d35b7d72
Author: Laine Stump <laine>
Date:   Mon Apr 2 00:26:44 2012 -0400

    qemu: fix memory leak in virDomainGetVcpus

Please see bug 808979.

# service libvirtd stop
Stopping libvirtd daemon:                                  [  OK  ]

# valgrind --leak-check=full -v libvirtd >& /root/detach-disk.log 2>&1

In another terminal:

# virsh detach-disk test vdb --persistent
Disk detached successfully
(test has two disk of "vda" and "vdb")

Ctl-C for the valgrind command in the first terminal.

# cat /root/detach-disk.log
.....
==8084== 306 (176 direct, 130 indirect) bytes in 1 blocks are definitely lost in loss record 704 of 807
==8084==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==8084==    by 0x36ADA4F1FD: virAlloc (memory.c:101)
==8084==    by 0x36ADA467F1: virCgroupNew (cgroup.c:606)
==8084==    by 0x36ADA46FCB: virCgroupForDomain (cgroup.c:870)
==8084==    by 0x484D74: qemuDomainDetachPciDiskDevice (qemu_hotplug.c:1585)
==8084==    by 0x459FC2: qemuDomainModifyDeviceFlags (qemu_driver.c:5292)
==8084==    by 0x36ADAD4965: virDomainDetachDeviceFlags (libvirt.c:9391)
==8084==    by 0x42CB08: remoteDispatchDomainDetachDeviceFlagsHelper (remote_dispatch.h:1219)
==8084==    by 0x36ADB17874: virNetServerProgramDispatch (virnetserverprogram.c:416)
==8084==    by 0x36ADB18B00: virNetServerHandleJob (virnetserver.c:164)
==8084==    by 0x36ADA5945B: virThreadPoolWorker (threadpool.c:144)
==8084==    by 0x36ADA58D78: virThreadHelper (threads-pthread.c:161)

leak for "virDomainDetachDeviceFlags".

(In reply to comment #19)
> .....
> ==8084== 306 (176 direct, 130 indirect) bytes in 1 blocks are definitely
> lost in loss record 704 of 807
> ==8084==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
> ==8084==    by 0x36ADA4F1FD: virAlloc (memory.c:101)
> ==8084==    by 0x36ADA467F1: virCgroupNew (cgroup.c:606)
> ==8084==    by 0x36ADA46FCB: virCgroupForDomain (cgroup.c:870)
> ==8084==    by 0x484D74: qemuDomainDetachPciDiskDevice (qemu_hotplug.c:1585)
> ==8084==    by 0x459FC2: qemuDomainModifyDeviceFlags (qemu_driver.c:5292)
> ==8084==    by 0x36ADAD4965: virDomainDetachDeviceFlags (libvirt.c:9391)
> ==8084==    by 0x42CB08: remoteDispatchDomainDetachDeviceFlagsHelper
> (remote_dispatch.h:1219)
> ==8084==    by 0x36ADB17874: virNetServerProgramDispatch
> (virnetserverprogram.c:416)
> ==8084==    by 0x36ADB18B00: virNetServerHandleJob (virnetserver.c:164)
> ==8084==    by 0x36ADA5945B: virThreadPoolWorker (threadpool.c:144)
> ==8084==    by 0x36ADA58D78: virThreadHelper (threads-pthread.c:161)
> 
> leak for "virDomainDetachDeviceFlags".

In fact, virDomainDetachDeviceFlags()->qemuDomainModifyDeviceFlags() then this leak have been given on Comment 9 and Comment 14.

The leak given in Comment 20 was fixed upstream in this commit:

commit 362c3b33e6f68bd0823c811767ed2c63bb1334c9
Author: Michal Privoznik <mprivozn>
Date:   Thu Mar 15 11:47:13 2012 +0100

    qemuDomainDetachPciDiskDevice: Free allocated cgroup
    
    This function potentially allocates new virCgroup but never
    frees it.

Alex,

since it has been a long time I guess many mem leaks have been fixed. If you happen to find a new ones, please open a new bug. I am closing this as WORKSFORME.

(In reply to comment #23)
> Alex,
> 
> since it has been a long time I guess many mem leaks have been fixed. If you
> happen to find a new ones, please open a new bug. I am closing this as
> WORKSFORME.

Okay, I will check it again, if I find memory leaks then will file a new bug.