10:12 a.m.
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
10:12 a.m.
New subject: [libvirt] [PATCH 1/6] Fix memory leak in openvz_conf.c
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(a)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
10:12 a.m.
New subject: [libvirt] [PATCH 2/6] Fix possible memory leak in phyp_driver.c
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(a)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
10:32 a.m.
New subject: [libvirt] [PATCH 2/6] Fix possible memory leak in phyp_driver.c
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(a)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
}
10:12 a.m.
New subject: [libvirt] [PATCH 3/6] Fix possible memory leak in util/virxml.c
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(a)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
10:33 a.m.
New subject: [libvirt] [PATCH 3/6] Fix possible memory leak in util/virxml.c
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(a)redhat.com>
---
src/util/virxml.c | 2 ++
1 file changed, 2 insertions(+)
ACK
John
10:12 a.m.
New subject: [libvirt] [PATCH 4/6] Fix possible memory leak in virsh-domain-monitor.c
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(a)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
7:33 a.m.
New subject: [libvirt] [PATCH v2 4/6] Fix possible memory leak in virsh-domain-monitor.c in cmdDomblklist
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(a)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
7:43 a.m.
New subject: [libvirt] [PATCH v2 4/6] Fix possible memory leak in virsh-domain-monitor.c in cmdDomblklist
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(a)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
8:26 a.m.
New subject: [libvirt] [PATCH v2 4/6] Fix possible memory leak in virsh-domain-monitor.c in cmdDomblklist
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(a)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
10:12 a.m.
New subject: [libvirt] [PATCH 5/6] Fix memory leak in securityselinuxlabeltest.c
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(a)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
10:38 a.m.
New subject: [libvirt] [PATCH 5/6] Fix memory leak in securityselinuxlabeltest.c
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(a)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;
10:12 a.m.
New subject: [libvirt] [PATCH 6/6] Fix coverity complain in commandtest.c
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(a)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
11:05 a.m.
New subject: [libvirt] [PATCH 6/6] Fix coverity complain in commandtest.c
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(a)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
11:17 a.m.
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
3:06 a.m.
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
4:03 a.m.
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
3935
days inactive
3938
days old
16 comments
3 participants
participants (3)
-
John Ferlan -
Osier Yang -
Pavel Hrdina