10:02 a.m.
With the build fix, my coverity build worked again (good), but found
more issues (boo).
Consider these changes partially as a replacement for Pavel's patch:
http://www.redhat.com/archives/libvir-list/2015-January/msg00240.html
with some additional changes for checks that need to be made in failure
path scenario. I didn't make any of the code formatting changes Pavel
had made for virDomainNetIpsFormat, but I can do that. I just wanted to
get these on the list for a comparison. Additionally, Pavel's change for
virDomainNetRoutesFormat didn't take into account that the alloc's are
done in a for loop, so both the VIR_FREE's needed to be done in the loop.
There are still some Coverity issues left over after this, but I'm not
quite sure how to fix them, hence the reply on the Xen-xl parser:
http://www.redhat.com/archives/libvir-list/2015-January/msg00243.html
John Ferlan (6):
openvz: Resolve Coverity RESOURCE_LEAK
openvz: Check errors from virSocketAddrFormat
domain_conf: Resolve Coverity RESOURCE_LEAK
domain_conf: Check errors from virSocketAddrFormat
xenconfig: Resolve Coverity RESOURCE_LEAK
virconf: Resolve a possible memory leak in virConfSetValue
src/conf/domain_conf.c | 32 ++++++++++++++++++++++++--------
src/openvz/openvz_driver.c | 3 +++
src/util/virconf.c | 4 +++-
src/xenconfig/xen_xl.c | 6 +++---
4 files changed, 33 insertions(+), 12 deletions(-)
--
2.1.0
10:02 a.m.
New subject: [libvirt] [PATCH 1/6] openvz: Resolve Coverity RESOURCE_LEAK
Commit id 'a4e86390' modified the command line to allow --ipadd multiple
times, which caused Coverity to notice a latent memory leak with the
'ipAddr' string not being VIR_FREE()'d
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/openvz/openvz_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index c144eca..64f5219 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -911,6 +911,7 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
for (i = 0; i < net->nips; i++) {
char *ipStr = virSocketAddrFormat(&net->ips[i]->address);
virCommandAddArgList(cmd, "--ipadd", ipStr, NULL);
+ VIR_FREE(ipStr);
}
}
--
2.1.0
10:02 a.m.
New subject: [libvirt] [PATCH 2/6] openvz: Check errors from virSocketAddrFormat
Commit id 'a4e86390' modified the command line to allow --ipadd multiple
times; however, it did not account for the condition where a NULL is
returned which will could lead to some interesting errors with multiple
--ipadd's without parameters.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/openvz/openvz_driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 64f5219..17919be 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -910,6 +910,8 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
/* --ipadd ip */
for (i = 0; i < net->nips; i++) {
char *ipStr = virSocketAddrFormat(&net->ips[i]->address);
+ if (!ipStr)
+ goto cleanup;
virCommandAddArgList(cmd, "--ipadd", ipStr, NULL);
VIR_FREE(ipStr);
}
--
2.1.0
10:02 a.m.
New subject: [libvirt] [PATCH 3/6] domain_conf: Resolve Coverity RESOURCE_LEAK
Commit id's 'c9a641f1e' and 'aa2cc721' added calls to
virSocketAddrFormat
and did not VIR_FREE() the return memory.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d1a483a..b53cb8d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17270,6 +17270,7 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips,
size_t nips)
familyStr = "ipv4";
virBufferAsprintf(buf, "<ip address='%s'",
ipStr);
+ VIR_FREE(ipStr);
if (familyStr)
virBufferAsprintf(buf, " family='%s'", familyStr);
if (ips[i]->prefix != 0)
@@ -17296,10 +17297,12 @@ virDomainNetRoutesFormat(virBufferPtr buf,
else if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET))
familyStr = "ipv4";
virBufferAsprintf(buf, "<route family='%s'
via='%s'", familyStr, via);
+ VIR_FREE(via);
if (VIR_SOCKET_ADDR_VALID(&route->to)) {
to = virSocketAddrFormat(&route->to);
virBufferAsprintf(buf, " address='%s'", to);
+ VIR_FREE(to);
}
if (route->prefix > 0)
--
2.1.0
10:02 a.m.
New subject: [libvirt] [PATCH 4/6] domain_conf: Check errors from virSocketAddrFormat
Commit id's 'c9a641f1e' and 'aa2cc721' added calls to
virSocketAddrFormat
but did not check for a NULL (error) return which could lead to bad output
in the XML file. Need to check for NULL return and cause failure.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b53cb8d..ef517b4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17254,7 +17254,7 @@ virDomainFSDefFormat(virBufferPtr buf,
return 0;
}
-static void
+static int
virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips)
{
size_t i;
@@ -17264,6 +17264,9 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips,
size_t nips)
virSocketAddrPtr address = &ips[i]->address;
char *ipStr = virSocketAddrFormat(address);
const char *familyStr = NULL;
+
+ if (!ipStr)
+ return -1;
if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6))
familyStr = "ipv6";
else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET))
@@ -17277,9 +17280,10 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr
*ips, size_t nips)
virBufferAsprintf(buf, " prefix='%u'", ips[i]->prefix);
virBufferAddLit(buf, "/>\n");
}
+ return 0;
}
-static void
+static int
virDomainNetRoutesFormat(virBufferPtr buf,
virDomainNetRouteDefPtr *routes,
size_t nroutes)
@@ -17292,6 +17296,8 @@ virDomainNetRoutesFormat(virBufferPtr buf,
char *via = virSocketAddrFormat(&route->via);
char *to = NULL;
+ if (!via)
+ return -1;
if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET6))
familyStr = "ipv6";
else if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET))
@@ -17301,6 +17307,8 @@ virDomainNetRoutesFormat(virBufferPtr buf,
if (VIR_SOCKET_ADDR_VALID(&route->to)) {
to = virSocketAddrFormat(&route->to);
+ if (!to)
+ return -1;
virBufferAsprintf(buf, " address='%s'", to);
VIR_FREE(to);
}
@@ -17310,6 +17318,7 @@ virDomainNetRoutesFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
+ return 0;
}
static int
@@ -17465,10 +17474,12 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf,
virBufferAddLit(buf, "</source>\n");
if (def->source.caps.type == VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET) {
- virDomainNetIpsFormat(buf, def->source.caps.u.net.ips,
- def->source.caps.u.net.nips);
- virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes,
- def->source.caps.u.net.nroutes);
+ if (virDomainNetIpsFormat(buf, def->source.caps.u.net.ips,
+ def->source.caps.u.net.nips) < 0)
+ return -1;
+ if (virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes,
+ def->source.caps.u.net.nroutes) < 0)
+ return -1;
}
return 0;
@@ -17857,8 +17868,10 @@ virDomainNetDefFormat(virBufferPtr buf,
return -1;
}
- virDomainNetIpsFormat(buf, def->ips, def->nips);
- virDomainNetRoutesFormat(buf, def->routes, def->nroutes);
+ if (virDomainNetIpsFormat(buf, def->ips, def->nips) < 0)
+ return -1;
+ if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0)
+ return -1;
virBufferEscapeString(buf, "<script path='%s'/>\n",
def->script);
--
2.1.0
10:02 a.m.
New subject: [libvirt] [PATCH 5/6] xenconfig: Resolve Coverity RESOURCE_LEAK
Commit id '2c78051a' added xenFormatXLDomainDisks which used the
virConfSetValue to set the 'diskVal' and handle/own the memory for
diskVal. However, if diskVal->list == NULL, the memory for diskVal
wouldn't be VIR_FREE()'d like other similar uses - so added the
VIR_FREE() to resolve the memory leak.
I also noted that the cleanup path should have returned -1 (like other
uses), so I modified that as well while I was making the change.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/xenconfig/xen_xl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 8d1d2a7..45c479a 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -396,8 +396,7 @@ xenFormatXLDomainDisks(virConfPtr conf, virDomainDefPtr def)
if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
continue;
if (xenFormatXLDisk(diskVal, def->disks[i]) < 0)
-
- goto cleanup;
+ goto cleanup;
}
if (diskVal->list != NULL) {
@@ -407,11 +406,12 @@ xenFormatXLDomainDisks(virConfPtr conf, virDomainDefPtr def)
goto cleanup;
}
+ VIR_FREE(diskVal);
return 0;
cleanup:
virConfFreeValue(diskVal);
- return 0;
+ return -1;
}
--
2.1.0
1:25 a.m.
New subject: [libvirt] [PATCH 5/6] xenconfig: Resolve Coverity RESOURCE_LEAK
On 01/09/2015 05:02 PM, John Ferlan wrote:
Commit id '2c78051a' added xenFormatXLDomainDisks which used
the
virConfSetValue to set the 'diskVal' and handle/own the memory for
diskVal. However, if diskVal->list == NULL, the memory for diskVal
wouldn't be VIR_FREE()'d like other similar uses - so added the
VIR_FREE() to resolve the memory leak.
I also noted that the cleanup path should have returned -1 (like other
uses), so I modified that as well while I was making the change.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/xenconfig/xen_xl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 8d1d2a7..45c479a 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -396,8 +396,7 @@ xenFormatXLDomainDisks(virConfPtr conf, virDomainDefPtr def)
if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
continue;
if (xenFormatXLDisk(diskVal, def->disks[i]) < 0)
-
- goto cleanup;
+ goto cleanup;
}
if (diskVal->list != NULL) {
@@ -407,11 +406,12 @@ xenFormatXLDomainDisks(virConfPtr conf, virDomainDefPtr def)
goto cleanup;
}
+ VIR_FREE(diskVal);
return 0;
cleanup:
virConfFreeValue(diskVal);
- return 0;
+ return -1;
}
NACK to this one as it won't be necessary to fix this memory leak,
because the whole file will be removed by reverting the commit 2c78051a
by the new patch series to implement xl parser
<https://www.redhat.com/archives/libvir-list/2015-January/msg00268.html>;.
Pavel
10:02 a.m.
New subject: [libvirt] [PATCH 6/6] virconf: Resolve a possible memory leak in virConfSetValue
Found this one by inspection... The API claims to "own" the input
value even in the case of error. However, in the initial entry
to the API if the value exists, was STRING, but without a str value
it just returned without freeing the 'value' which it claims to now
own. So I added the virConfFreeValue() call in order to resolve.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virconf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index b1509fe..01e5a6a 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -887,8 +887,10 @@ virConfSetValue(virConfPtr conf,
{
virConfEntryPtr cur, prev = NULL;
- if (value && value->type == VIR_CONF_STRING && value->str ==
NULL)
+ if (value && value->type == VIR_CONF_STRING && value->str ==
NULL) {
+ virConfFreeValue(value);
return -1;
+ }
cur = conf->entries;
while (cur != NULL) {
--
2.1.0
1:36 a.m.
On 01/09/2015 05:02 PM, John Ferlan wrote:
With the build fix, my coverity build worked again (good), but found
more issues (boo).
Consider these changes partially as a replacement for Pavel's patch:
http://www.redhat.com/archives/libvir-list/2015-January/msg00240.html
with some additional changes for checks that need to be made in failure
path scenario. I didn't make any of the code formatting changes Pavel
had made for virDomainNetIpsFormat, but I can do that. I just wanted to
get these on the list for a comparison. Additionally, Pavel's change for
virDomainNetRoutesFormat didn't take into account that the alloc's are
done in a for loop, so both the VIR_FREE's needed to be done in the loop.
There are still some Coverity issues left over after this, but I'm not
quite sure how to fix them, hence the reply on the Xen-xl parser:
http://www.redhat.com/archives/libvir-list/2015-January/msg00243.html
John Ferlan (6):
openvz: Resolve Coverity RESOURCE_LEAK
openvz: Check errors from virSocketAddrFormat
domain_conf: Resolve Coverity RESOURCE_LEAK
domain_conf: Check errors from virSocketAddrFormat
xenconfig: Resolve Coverity RESOURCE_LEAK
virconf: Resolve a possible memory leak in virConfSetValue
src/conf/domain_conf.c | 32 ++++++++++++++++++++++++--------
src/openvz/openvz_driver.c | 3 +++
src/util/virconf.c | 4 +++-
src/xenconfig/xen_xl.c | 6 +++---
4 files changed, 33 insertions(+), 12 deletions(-)
Oh, thanks for noticing that the code was ignoring return values. I've
just blindly looked at the memory leaks and fixed them without wondering
if there is anything else wrong.
ACK to the series except the 5/6 patch for xen_xl parser as there is a
new series waiting for review which will revert the code and introduce
a new approach to parse the xen xl configuration.
Pavel
7:58 a.m.
On 01/12/2015 02:36 AM, Pavel Hrdina wrote:
On 01/09/2015 05:02 PM, John Ferlan wrote:
> With the build fix, my coverity build worked again (good), but found
> more issues (boo).
>
> Consider these changes partially as a replacement for Pavel's patch:
>
> http://www.redhat.com/archives/libvir-list/2015-January/msg00240.html
>
> with some additional changes for checks that need to be made in failure
> path scenario. I didn't make any of the code formatting changes Pavel
> had made for virDomainNetIpsFormat, but I can do that. I just wanted to
> get these on the list for a comparison. Additionally, Pavel's change for
> virDomainNetRoutesFormat didn't take into account that the alloc's are
> done in a for loop, so both the VIR_FREE's needed to be done in the loop.
>
> There are still some Coverity issues left over after this, but I'm not
> quite sure how to fix them, hence the reply on the Xen-xl parser:
>
> http://www.redhat.com/archives/libvir-list/2015-January/msg00243.html
>
> John Ferlan (6):
> openvz: Resolve Coverity RESOURCE_LEAK
> openvz: Check errors from virSocketAddrFormat
> domain_conf: Resolve Coverity RESOURCE_LEAK
> domain_conf: Check errors from virSocketAddrFormat
> xenconfig: Resolve Coverity RESOURCE_LEAK
> virconf: Resolve a possible memory leak in virConfSetValue
>
> src/conf/domain_conf.c | 32 ++++++++++++++++++++++++--------
> src/openvz/openvz_driver.c | 3 +++
> src/util/virconf.c | 4 +++-
> src/xenconfig/xen_xl.c | 6 +++---
> 4 files changed, 33 insertions(+), 12 deletions(-)
>
Oh, thanks for noticing that the code was ignoring return values. I've
just blindly looked at the memory leaks and fixed them without wondering
if there is anything else wrong.
ACK to the series except the 5/6 patch for xen_xl parser as there is a
new series waiting for review which will revert the code and introduce
a new approach to parse the xen xl configuration.
OK - thanks for the review. However, I think I'm going to wait a few
cycles before addressing 3/6 & 4/6 since it seems Cedric Bosdonnat is
already in the process of adjusting virDomainNetRoutesFormat() and these
changes may partially be duplicitous.
Cedric - perhaps as part of your changes, you could also make
adjustments to cover the cases found through Coverity using the two
domain_conf.c patches I've posted to resolve Coverity issues? I believe
they were issues as a result of the 15 patch series pushed recently.
I will push 1, 2, and 6 shortly.
John
8:35 a.m.
On Mon, 2015-01-12 at 08:58 -0500, John Ferlan wrote:
On 01/12/2015 02:36 AM, Pavel Hrdina wrote:
> On 01/09/2015 05:02 PM, John Ferlan wrote:
>> With the build fix, my coverity build worked again (good), but found
>> more issues (boo).
>>
>> Consider these changes partially as a replacement for Pavel's patch:
>>
>> http://www.redhat.com/archives/libvir-list/2015-January/msg00240.html
>>
>> with some additional changes for checks that need to be made in failure
>> path scenario. I didn't make any of the code formatting changes Pavel
>> had made for virDomainNetIpsFormat, but I can do that. I just wanted to
>> get these on the list for a comparison. Additionally, Pavel's change for
>> virDomainNetRoutesFormat didn't take into account that the alloc's are
>> done in a for loop, so both the VIR_FREE's needed to be done in the loop.
>>
>> There are still some Coverity issues left over after this, but I'm not
>> quite sure how to fix them, hence the reply on the Xen-xl parser:
>>
>> http://www.redhat.com/archives/libvir-list/2015-January/msg00243.html
>>
>> John Ferlan (6):
>> openvz: Resolve Coverity RESOURCE_LEAK
>> openvz: Check errors from virSocketAddrFormat
>> domain_conf: Resolve Coverity RESOURCE_LEAK
>> domain_conf: Check errors from virSocketAddrFormat
>> xenconfig: Resolve Coverity RESOURCE_LEAK
>> virconf: Resolve a possible memory leak in virConfSetValue
>>
>> src/conf/domain_conf.c | 32 ++++++++++++++++++++++++--------
>> src/openvz/openvz_driver.c | 3 +++
>> src/util/virconf.c | 4 +++-
>> src/xenconfig/xen_xl.c | 6 +++---
>> 4 files changed, 33 insertions(+), 12 deletions(-)
>>
>
> Oh, thanks for noticing that the code was ignoring return values. I've
> just blindly looked at the memory leaks and fixed them without wondering
> if there is anything else wrong.
>
> ACK to the series except the 5/6 patch for xen_xl parser as there is a
> new series waiting for review which will revert the code and introduce
> a new approach to parse the xen xl configuration.
>
OK - thanks for the review. However, I think I'm going to wait a few
cycles before addressing 3/6 & 4/6 since it seems Cedric Bosdonnat is
already in the process of adjusting virDomainNetRoutesFormat() and these
changes may partially be duplicitous.
Cedric - perhaps as part of your changes, you could also make
adjustments to cover the cases found through Coverity using the two
domain_conf.c patches I've posted to resolve Coverity issues? I believe
they were issues as a result of the 15 patch series pushed recently.
That was on my TODO list... just didn't manage to come to it yet.
--
Cedric
3603
days inactive
3606
days old
10 comments
3 participants
participants (3)
-
Cedric Bosdonnat -
John Ferlan -
Pavel Hrdina