[libvirt] [PATCH 0/3] Miscellaneous Coverity & Valgrind changes
These patches resolve a Coverity warning and a found Valgrind leak. Added more patterns to the .valgrind.supp file to suppress the commandtests and seclabeltest output. This will thus result in a mostly clean valgrind run. All that remains is a hotplugtest failure, which some patches were posted: https://www.redhat.com/archives/libvir-list/2013-July/msg01156.html and https://www.redhat.com/archives/libvir-list/2013-July/msg01158.html but did not completely resolve the failures. John Ferlan (3): lxc: Resolve Coverity warning domain_event: Resolve memory leak found by Valgrind valgrind.supp: Add more valgrind suppression paths src/conf/domain_event.c | 3 +++ src/lxc/lxc_process.c | 2 +- tests/.valgrind.supp | 60 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) -- 1.8.1.4
Commit 'c8695053' resulted in the following: Coverity error seen in the output: ERROR: REVERSE_INULL FUNCTION: lxcProcessAutoDestroy Due to the 'dom' being checked before 'dom->persistent' since 'dom' is already dereferenced prior to that. --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 5b83ccb..5c4f8c8 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -74,7 +74,7 @@ lxcProcessAutoDestroy(virDomainObjPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); priv->doneStopEvent = true; - if (dom && !dom->persistent) { + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); dom = NULL; } -- 1.8.1.4
On 07/23/13 17:59, John Ferlan wrote:
Commit 'c8695053' resulted in the following:
Coverity error seen in the output: ERROR: REVERSE_INULL FUNCTION: lxcProcessAutoDestroy
Due to the 'dom' being checked before 'dom->persistent' since 'dom' is already dereferenced prior to that. --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 5b83ccb..5c4f8c8 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -74,7 +74,7 @@ lxcProcessAutoDestroy(virDomainObjPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); priv->doneStopEvent = true;
- if (dom && !dom->persistent) { + if (!dom->persistent) { virDomainObjListRemove(driver->domains, dom); dom = NULL; }
ACK, dom is indeed dereferenced before, thus it will not crash here. Peter
Commit id '4421e257' strdup'd devAlias, but didn't free Running qemuhotplugtest under valgrind resulted in the following: ==7375== 9 bytes in 1 blocks are definitely lost in loss record 11 of 70 ==7375== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==7375== by 0x37C1085D71: strdup (strdup.c:42) ==7375== by 0x4CBBD5F: virStrdup (virstring.c:554) ==7375== by 0x4CFF9CB: virDomainEventDeviceRemovedNew (domain_event.c:1174) ==7375== by 0x427791: qemuDomainRemoveChrDevice (qemu_hotplug.c:2508) ==7375== by 0x42C65D: qemuDomainDetachChrDevice (qemu_hotplug.c:3357) ==7375== by 0x41C94F: testQemuHotplug (qemuhotplugtest.c:115) ==7375== by 0x41D817: virtTestRun (testutils.c:168) ==7375== by 0x41C400: mymain (qemuhotplugtest.c:322) ==7375== by 0x41DF3A: virtTestMain (testutils.c:764) ==7375== by 0x37C1021A04: (below main) (libc-start.c:225) --- src/conf/domain_event.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 640463c..16ae92b 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -520,6 +520,9 @@ void virDomainEventFree(virDomainEventPtr event) case VIR_DOMAIN_EVENT_ID_TRAY_CHANGE: VIR_FREE(event->data.trayChange.devAlias); break; + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: + VIR_FREE(event->data.deviceRemoved.devAlias); + break; } VIR_FREE(event->dom.name); -- 1.8.1.4
On 07/23/13 17:59, John Ferlan wrote:
Commit id '4421e257' strdup'd devAlias, but didn't free
Running qemuhotplugtest under valgrind resulted in the following:
==7375== 9 bytes in 1 blocks are definitely lost in loss record 11 of 70 ==7375== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==7375== by 0x37C1085D71: strdup (strdup.c:42) ==7375== by 0x4CBBD5F: virStrdup (virstring.c:554) ==7375== by 0x4CFF9CB: virDomainEventDeviceRemovedNew (domain_event.c:1174) ==7375== by 0x427791: qemuDomainRemoveChrDevice (qemu_hotplug.c:2508) ==7375== by 0x42C65D: qemuDomainDetachChrDevice (qemu_hotplug.c:3357) ==7375== by 0x41C94F: testQemuHotplug (qemuhotplugtest.c:115) ==7375== by 0x41D817: virtTestRun (testutils.c:168) ==7375== by 0x41C400: mymain (qemuhotplugtest.c:322) ==7375== by 0x41DF3A: virtTestMain (testutils.c:764) ==7375== by 0x37C1021A04: (below main) (libc-start.c:225) --- src/conf/domain_event.c | 3 +++ 1 file changed, 3 insertions(+)
ACK. Peter
Update based on recent run/failures seen --- tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 10cc3c0..f04912d 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp @@ -75,3 +75,63 @@ ... obj:*/lib*/libc-2.*so* } +# +# commandtest validates the various threaded commands. The +# virThreadCreate() routine allocates and passes args to the +# new thread which now owns the 'args' and thus cannot be free'd +# +{ + commandtestLeak1 + Memcheck:Leak + fun:calloc + fun:virAlloc + fun:virThreadCreate + fun:mymain + fun:virtTestMain +} +# +# The Error code requires static memory that is never free'd +# for thread local storage to store error message/data +# +{ + commandtestLeak2 + Memcheck:Leak + fun:calloc + fun:virAlloc + ... + fun:vir*LastError* + fun:virEventRunDefaultImpl + fun:virCommandThreadWorker + fun:virThreadHelper + fun:start_thread + fun:clone +} +# +# Some of the commandtests (test0, test1, test4, & test18) cause the +# following traceback although it appears the memory is properly freed +# +{ + commandtestLeak3 + Memcheck:Leak + fun:calloc + fun:virAllocN + fun:virEventPollRunOnce + fun:virEventRunDefaultImpl + fun:virCommandThreadWorker + fun:virThreadHelper + fun:start_thread + fun:clone +} +# +# seclabeltest relies on 'selabel_close' which is not in libvirt +# +{ + seclabeltestcond1 + Memcheck:Cond + obj:/usr/lib64/libselinux.so.1 + fun:selabel_close + fun:virSecuritySELinuxSecurityDriverClose + fun:virSecurityManagerDispose + fun:virObjectUnref + fun:main +} -- 1.8.1.4
On 07/23/13 17:59, John Ferlan wrote:
Update based on recent run/failures seen --- tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 10cc3c0..f04912d 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp
I'm not an valgrind expert, but those look okay to me. ACK. Peter
On 07/23/2013 09:59 AM, John Ferlan wrote:
Update based on recent run/failures seen --- tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 10cc3c0..f04912d 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp @@ -75,3 +75,63 @@ ... obj:*/lib*/libc-2.*so* } +# +# commandtest validates the various threaded commands. The +# virThreadCreate() routine allocates and passes args to the +# new thread which now owns the 'args' and thus cannot be free'd +# +{ + commandtestLeak1 + Memcheck:Leak + fun:calloc + fun:virAlloc + fun:virThreadCreate + fun:mymain + fun:virtTestMain
Can't we call VIR_FREE(args) in the forked process? Or is this one of those inconsequential leaks right before we exec in the child process?
+} +# +# The Error code requires static memory that is never free'd +# for thread local storage to store error message/data +# +{ + commandtestLeak2 + Memcheck:Leak + fun:calloc + fun:virAlloc + ... + fun:vir*LastError* + fun:virEventRunDefaultImpl + fun:virCommandThreadWorker + fun:virThreadHelper + fun:start_thread + fun:clone
I thought we had the ability to wire up destructors for thread local storage, that gets called when the thread completes? At any rate, since all of these suppressions are solely for test files, we aren't masking leaks in libvirtd proper, so if it makes it easier to spot the introduction of new leaks, I'm okay with this patch as-is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/23/2013 12:45 PM, Eric Blake wrote:
On 07/23/2013 09:59 AM, John Ferlan wrote:
Update based on recent run/failures seen --- tests/.valgrind.supp | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 10cc3c0..f04912d 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp @@ -75,3 +75,63 @@ ... obj:*/lib*/libc-2.*so* } +# +# commandtest validates the various threaded commands. The +# virThreadCreate() routine allocates and passes args to the +# new thread which now owns the 'args' and thus cannot be free'd +# +{ + commandtestLeak1 + Memcheck:Leak + fun:calloc + fun:virAlloc + fun:virThreadCreate + fun:mymain + fun:virtTestMain
Can't we call VIR_FREE(args) in the forked process? Or is this one of those inconsequential leaks right before we exec in the child process?
We do call VIR_FREE() in virThreadHelper() from the pthread_create().
+} +# +# The Error code requires static memory that is never free'd +# for thread local storage to store error message/data +# +{ + commandtestLeak2 + Memcheck:Leak + fun:calloc + fun:virAlloc + ... + fun:vir*LastError* + fun:virEventRunDefaultImpl + fun:virCommandThreadWorker + fun:virThreadHelper + fun:start_thread + fun:clone
I thought we had the ability to wire up destructors for thread local storage, that gets called when the thread completes?
The LastError code does wire up the destructor from how I read it. It's very strange - running the same ranges of tests two times in a row had different results... That is, if I did "make -C tests valgrind TESTS=commandtest VIR_TEST_RANGE=1-4" two times in a row - one time I'd get 1 set of tracebacks and the next time I get 2. If I run just one test (1-1), I get none. I'm not sure I completely understand why
At any rate, since all of these suppressions are solely for test files, we aren't masking leaks in libvirtd proper, so if it makes it easier to spot the introduction of new leaks, I'm okay with this patch as-is.
Strange - I thought commandtestLeak3 was the most odd - only 4 tests cause issues, but "sometimes" running them singly using VIR_TEST_RANGE resulted in no leak shown. I assume the failures were related to perhaps the "timing" or "synchronization" between determining which thread was doing what and how quickly or slowly it did it. I can hold off on patch 3 if that's desired, too. John
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa