[libvirt] [PATCH 0/6] Resolve some Coverity errors and other issues found
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
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@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
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@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
Commit id's 'c9a641f1e' and 'aa2cc721' added calls to virSocketAddrFormat and did not VIR_FREE() the return memory. Signed-off-by: John Ferlan <jferlan@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
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@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
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@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
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@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
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@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
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
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
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
participants (3)
-
Cedric Bosdonnat -
John Ferlan -
Pavel Hrdina