[libvirt] [PATCH 0/6] Fix some memory leaks and other issues find by coverity tool
This patch series fixes few memory leaks found by coverity tool to make that tool happy. The last patch is adding only one comment to hide "double_close" error as coverity tool is wrong about this and we don't have to see it in every report. Check the patch for more information. Pavel Hrdina (6): Fix memory leak in openvz_conf.c Fix possible memory leak in phyp_driver.c Fix possible memory leak in util/virxml.c Fix possible memory leak in virsh-domain-monitor.c Fix memory leak in securityselinuxlabeltest.c Fix coverity complain in commandtest.c src/openvz/openvz_conf.c | 5 +++-- src/phyp/phyp_driver.c | 4 +++- src/util/virxml.c | 2 ++ tests/commandtest.c | 1 + tests/securityselinuxlabeltest.c | 5 ++++- tools/virsh-domain-monitor.c | 4 ++++ 6 files changed, 17 insertions(+), 4 deletions(-) -- 1.8.3.1
If there is no error while executing a function "openvzParseBarrierLimit" a "str" string where is duplicate of a "value" string isn't freed and it leads into memory leak. This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/openvz/openvz_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 0dbaa4a..91b16f8 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -136,6 +136,7 @@ openvzParseBarrierLimit(const char* value, char *token; char *saveptr = NULL; char *str; + int ret = -1; if (VIR_STRDUP(str, value) < 0) goto error; @@ -158,10 +159,10 @@ openvzParseBarrierLimit(const char* value, goto error; } } - return 0; + ret = 0; error: VIR_FREE(str); - return -1; + return ret; } -- 1.8.3.1
There could be a memory leak caused by "managed_system" string, if any error occurs before "managed_system" is assigned into "phyp_driver->managed_system". The "managed_system" string wouldn't be freed at all. The better way is free the "managed_system" instead of the one assigned in the "phyp_driver". This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/phyp/phyp_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 32165ed..6833e44 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1153,9 +1153,11 @@ phypConnectOpen(virConnectPtr conn, return VIR_DRV_OPEN_SUCCESS; failure: + if (conn->uri->path) + VIR_FREE(managed_system); + if (phyp_driver != NULL) { virObjectUnref(phyp_driver->caps); - VIR_FREE(phyp_driver->managed_system); VIR_FREE(phyp_driver); } -- 1.8.3.1
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
There could be a memory leak caused by "managed_system" string, if any error occurs before "managed_system" is assigned into "phyp_driver->managed_system". The "managed_system" string wouldn't be freed at all. The better way is free the "managed_system" instead of the one assigned in the "phyp_driver".
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/phyp/phyp_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 32165ed..6833e44 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1153,9 +1153,11 @@ phypConnectOpen(virConnectPtr conn, return VIR_DRV_OPEN_SUCCESS;
failure: + if (conn->uri->path) + VIR_FREE(managed_system); +
Since managed_system is declared = NULL, so you could just drop the 'if' condition...
if (phyp_driver != NULL) { virObjectUnref(phyp_driver->caps); - VIR_FREE(phyp_driver->managed_system); VIR_FREE(phyp_driver);
not found by Coverity, but phyp_driver->xmlopt needs a 'virObjectUnref' as well ACK with the changes John
}
A "xmlstr" string may not be assigned into a "doc" pointer and it could cause memory leak. To fix it if the "doc" pointer is NULL and the "xmlstr" string is not assigned we should free it. This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/virxml.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 5852374..dd530a6 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1047,6 +1047,8 @@ virXMLExtractNamespaceXML(xmlNodePtr root, cleanup: if (doc) *doc = xmlstr; + else + VIR_FREE(xmlstr); xmlFreeNode(nodeCopy); return ret; } -- 1.8.3.1
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
A "xmlstr" string may not be assigned into a "doc" pointer and it could cause memory leak. To fix it if the "doc" pointer is NULL and the "xmlstr" string is not assigned we should free it.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/virxml.c | 2 ++ 1 file changed, 2 insertions(+)
ACK John
In a "for" loop there are created two new strings and they may not be freed if a "target" string cannot be obtained. We have to free the two created strings to prevent the memory leak. This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh-domain-monitor.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b29b82a..3a310df 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -560,6 +560,10 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) target = virXPathString("string(./target/@dev)", ctxt); if (!target) { vshError(ctl, "unable to query block list"); + if (details) { + VIR_FREE(type); + VIR_FREE(device); + } goto cleanup; } source = virXPathString("string(./source/@file" -- 1.8.3.1
In a "for" loop there are created two new strings and they may not be freed if a "target" string cannot be obtained. We have to free the two created strings to prevent the memory leak. This has been found by coverity. John also pointed out that we should somehow care about the "type" and "device" and Osier agreed to exit with error message if one of them is set to NULL. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- version 2: - initialize "type" and "device" to NULL - exit with error message if "type" and "device" is NULL - remove a "if (details)" condition before freeing "type" and "device" tools/virsh-domain-monitor.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b29b82a..de4afbb 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -545,8 +545,8 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "------------------------------------------------\n"); for (i = 0; i < ndisks; i++) { - char *type; - char *device; + char *type = NULL; + char *device = NULL; char *target; char *source; @@ -555,11 +555,19 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (details) { type = virXPathString("string(./@type)", ctxt); device = virXPathString("string(./@device)", ctxt); + if (!type || !device) { + vshPrint(ctl, "unable to query block list details"); + VIR_FREE(type); + VIR_FREE(device); + goto cleanup; + } } target = virXPathString("string(./target/@dev)", ctxt); if (!target) { vshError(ctl, "unable to query block list"); + VIR_FREE(type); + VIR_FREE(device); goto cleanup; } source = virXPathString("string(./source/@file" -- 1.8.3.1
On 01/16/2014 08:33 AM, Pavel Hrdina wrote:
In a "for" loop there are created two new strings and they may not be freed if a "target" string cannot be obtained. We have to free the two created strings to prevent the memory leak.
This has been found by coverity.
John also pointed out that we should somehow care about the "type" and "device" and Osier agreed to exit with error message if one of them is set to NULL.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- version 2: - initialize "type" and "device" to NULL - exit with error message if "type" and "device" is NULL - remove a "if (details)" condition before freeing "type" and "device"
tools/virsh-domain-monitor.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
ACK John
On 16.1.2014 14:43, John Ferlan wrote:
On 01/16/2014 08:33 AM, Pavel Hrdina wrote:
In a "for" loop there are created two new strings and they may not be freed if a "target" string cannot be obtained. We have to free the two created strings to prevent the memory leak.
This has been found by coverity.
John also pointed out that we should somehow care about the "type" and "device" and Osier agreed to exit with error message if one of them is set to NULL.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- version 2: - initialize "type" and "device" to NULL - exit with error message if "type" and "device" is NULL - remove a "if (details)" condition before freeing "type" and "device"
tools/virsh-domain-monitor.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
ACK
John
Thanks, pushed. Pavel
Strings "file" and "context" may not be freed if "VIR_EXPAND_N" fails and it leads into memory leak. This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/securityselinuxlabeltest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index d1fd92f..08b2997 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -127,8 +127,11 @@ testSELinuxLoadFileList(const char *testname, context = NULL; } - if (VIR_EXPAND_N(*files, *nfiles, 1) < 0) + if (VIR_EXPAND_N(*files, *nfiles, 1) < 0) { + VIR_FREE(file); + VIR_FREE(context); goto cleanup; + } (*files)[(*nfiles)-1].file = file; (*files)[(*nfiles)-1].context = context; -- 1.8.3.1
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
Strings "file" and "context" may not be freed if "VIR_EXPAND_N" fails and it leads into memory leak.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/securityselinuxlabeltest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index d1fd92f..08b2997 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -127,8 +127,11 @@ testSELinuxLoadFileList(const char *testname, context = NULL; }
When 'file' is declared above, you'll need to add a 'file = NULL'... Probably could do the same for context, thus removing the need for the context = NULL else condition. ACK with the adjustments John
- if (VIR_EXPAND_N(*files, *nfiles, 1) < 0) + if (VIR_EXPAND_N(*files, *nfiles, 1) < 0) { + VIR_FREE(file); + VIR_FREE(context); goto cleanup; + }
(*files)[(*nfiles)-1].file = file; (*files)[(*nfiles)-1].context = context;
For a "newfd1" the coverity tool thinks that the fd is closed in a "virCommandPassFD", but with "flags == 0" it cannot be never closed. The code itself is ok, but coverity tool thinks that there is "double_close" of the "newfd1" and to prevent showing this error we simply add a comment before the proper close. This has been found by coverity. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/commandtest.c b/tests/commandtest.c index 08e9e21..7ff4d5f 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -215,6 +215,7 @@ static int test3(const void *unused ATTRIBUTE_UNUSED) cleanup: virCommandFree(cmd); + /* coverity[double_close] */ VIR_FORCE_CLOSE(newfd1); VIR_FORCE_CLOSE(newfd2); return ret; -- 1.8.3.1
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
For a "newfd1" the coverity tool thinks that the fd is closed in a "virCommandPassFD", but with "flags == 0" it cannot be never closed.
I think you meant "cannot be close" the never is a double negative.
The code itself is ok, but coverity tool thinks that there is "double_close" of the "newfd1" and to prevent showing this error we simply add a comment before the proper close.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+)
When this was first changed/submitted: http://www.redhat.com/archives/libvir-list/2013-July/msg00831.html and thus seen by my nightly Coverity run, I remember asking Dan about this, but I think in the end the thought was it was a Coverity bug. I had always meant to go back to it to see if I could put together a reproducer to submit to Coverity as a bug report, but never quite got around to it. I suppose though, this is a viable "way around" the message, although it may be a good idea to still revisit as a Coverity issue assuming a reproducer program could be put together. ACK with the grammar fix from above John
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
This patch series fixes few memory leaks found by coverity tool to make that tool happy.
The last patch is adding only one comment to hide "double_close" error as coverity tool is wrong about this and we don't have to see it in every report. Check the patch for more information.
Pavel Hrdina (6): Fix memory leak in openvz_conf.c Fix possible memory leak in phyp_driver.c Fix possible memory leak in util/virxml.c Fix possible memory leak in virsh-domain-monitor.c Fix memory leak in securityselinuxlabeltest.c Fix coverity complain in commandtest.c
src/openvz/openvz_conf.c | 5 +++-- src/phyp/phyp_driver.c | 4 +++- src/util/virxml.c | 2 ++ tests/commandtest.c | 1 + tests/securityselinuxlabeltest.c | 5 ++++- tools/virsh-domain-monitor.c | 4 ++++ 6 files changed, 17 insertions(+), 4 deletions(-)
hrmph... for some reason 1/6 and 4/6 haven't made it to my inbox yet. I hope the Red Hat mail filter isn't acting up again... Anyway 1/6 is an ACK 4/6 I think still issues First off 'type' and 'device' need to be initialized to NULL, then don't worry about the "if details()" within the (!target) condition. Second either we have to choose to exit if the virXPathString() fails for type/device or when we go to vshPrint() there needs to be a "type ? type : "-"" and "device ? device : "-"" in order to ensure we're not printing garbage or old data Not sure which of the methods is better if we fail to alloc type or device is better. Note that virXPathString() can return NULL for more reasons than just allocation failure. Since Osier wrote the code, maybe he can help decide the course of action to take. Is it better to display "-" or just fail? On an allocation failure, then virXPathString for target will probably fail too landing us in that error path, so we could just take that option too! John
On 13.1.2014 18:17, John Ferlan wrote:
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
This patch series fixes few memory leaks found by coverity tool to make that tool happy.
The last patch is adding only one comment to hide "double_close" error as coverity tool is wrong about this and we don't have to see it in every report. Check the patch for more information.
Pavel Hrdina (6): Fix memory leak in openvz_conf.c Fix possible memory leak in phyp_driver.c Fix possible memory leak in util/virxml.c Fix possible memory leak in virsh-domain-monitor.c Fix memory leak in securityselinuxlabeltest.c Fix coverity complain in commandtest.c
src/openvz/openvz_conf.c | 5 +++-- src/phyp/phyp_driver.c | 4 +++- src/util/virxml.c | 2 ++ tests/commandtest.c | 1 + tests/securityselinuxlabeltest.c | 5 ++++- tools/virsh-domain-monitor.c | 4 ++++ 6 files changed, 17 insertions(+), 4 deletions(-)
hrmph... for some reason 1/6 and 4/6 haven't made it to my inbox yet. I hope the Red Hat mail filter isn't acting up again...
Anyway
1/6 is an ACK
4/6 I think still issues
First off 'type' and 'device' need to be initialized to NULL, then don't worry about the "if details()" within the (!target) condition.
Second either we have to choose to exit if the virXPathString() fails for type/device or when we go to vshPrint() there needs to be a "type ? type : "-"" and "device ? device : "-"" in order to ensure we're not printing garbage or old data
Not sure which of the methods is better if we fail to alloc type or device is better. Note that virXPathString() can return NULL for more reasons than just allocation failure.
Hi John, Thanks for the review, if I'll obtain git write access I'll bush the ACKed patches by myself with the modifications or I'll ask someone to do it.
Since Osier wrote the code, maybe he can help decide the course of action to take. Is it better to display "-" or just fail? On an allocation failure, then virXPathString for target will probably fail too landing us in that error path, so we could just take that option too!
Hi Osier, Let me know what you think is the better way. Pavel
John
On 14/01/14 01:17, John Ferlan wrote:
On 01/13/2014 11:12 AM, Pavel Hrdina wrote:
This patch series fixes few memory leaks found by coverity tool to make that tool happy.
The last patch is adding only one comment to hide "double_close" error as coverity tool is wrong about this and we don't have to see it in every report. Check the patch for more information.
Pavel Hrdina (6): Fix memory leak in openvz_conf.c Fix possible memory leak in phyp_driver.c Fix possible memory leak in util/virxml.c Fix possible memory leak in virsh-domain-monitor.c Fix memory leak in securityselinuxlabeltest.c Fix coverity complain in commandtest.c
src/openvz/openvz_conf.c | 5 +++-- src/phyp/phyp_driver.c | 4 +++- src/util/virxml.c | 2 ++ tests/commandtest.c | 1 + tests/securityselinuxlabeltest.c | 5 ++++- tools/virsh-domain-monitor.c | 4 ++++ 6 files changed, 17 insertions(+), 4 deletions(-)
hrmph... for some reason 1/6 and 4/6 haven't made it to my inbox yet. I hope the Red Hat mail filter isn't acting up again...
Anyway
1/6 is an ACK
4/6 I think still issues
First off 'type' and 'device' need to be initialized to NULL, then don't worry about the "if details()" within the (!target) condition.
Second either we have to choose to exit if the virXPathString() fails for type/device or when we go to vshPrint() there needs to be a "type ? type : "-"" and "device ? device : "-"" in order to ensure we're not printing garbage or old data
Both the "type" and "device" are not mandatory attributes for disk device, "type" defaults to "file", and "device" defaults to "disk". So in principle they can't be NULL, if virXPathString returns NULL for them, it means either the internal data structure is broken (unlikely but critical), or virXPathString itself had some funny things. For either of them, we should fail. It's unlike "source" element, since the disk source can be empty, e.g. for a Floppy drive.
Not sure which of the methods is better if we fail to alloc type or device is better. Note that virXPathString() can return NULL for more reasons than just allocation failure.
Since Osier wrote the code, maybe he can help decide the course of action to take. Is it better to display "-" or just fail? On an allocation failure, then virXPathString for target will probably fail too landing us in that error path, so we could just take that option too!
Regards, Osier
participants (3)
-
John Ferlan -
Osier Yang -
Pavel Hrdina