[libvirt] [PATCH 0/5] Resolve REVERSE_INULL errors found by Coverity
This patch will resolve a set of "REVERSE_INULL (CWE-476):" errors found by Coverity. John Ferlan (5): network: Resolve some issues around vlan copying xen: Remove extraneous checks in xenStoreGetDomainInfo() interface: Need to check ifacedef->mac not just ifacedef after strdup() xen: Need to check validity of domain->conn before deref privateData openvz: Need to check 'vm' first before dereferencing 'def' src/interface/interface_backend_udev.c | 2 +- src/network/bridge_driver.c | 16 +++++++++++++--- src/openvz/openvz_driver.c | 2 +- src/xen/xen_hypervisor.c | 3 ++- src/xen/xs_internal.c | 2 +- 5 files changed, 18 insertions(+), 7 deletions(-) -- 1.7.11.7
Remove extraneous check for 'netdef' when dereferencing for vlan.nTags. Prior code would already check if netdef was NULL. Coverity complained about a path where the 'vlan' was potentially valid, but a prior checks may not have allocated 'iface->data.network.actual', so like other paths it needs to be allocated on the fly. --- src/network/bridge_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f1be954..e2b8d06 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4005,11 +4005,21 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) vlan = &iface->vlan; else if (portgroup && portgroup->vlan.nTags > 0) vlan = &portgroup->vlan; - else if (netdef && netdef->vlan.nTags > 0) + else if (netdef->vlan.nTags > 0) vlan = &netdef->vlan; - if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) - goto error; + if (vlan) { + /* data.network.actual may be NULL here when netdef->foward.type is + * VIR_NETWORK_FORWARD_{NONE|NAT|ROUTE} + */ + if (!iface->data.network.actual + && (VIR_ALLOC(iface->data.network.actual) < 0)) { + virReportOOMError(); + goto error; + } + if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) + goto error; + } validate: /* make sure that everything now specified for the device is -- 1.7.11.7
On 01/15/2013 01:35 PM, John Ferlan wrote:
Remove extraneous check for 'netdef' when dereferencing for vlan.nTags. Prior code would already check if netdef was NULL.
Coverity complained about a path where the 'vlan' was potentially valid, but a prior checks may not have allocated 'iface->data.network.actual', so like other paths it needs to be allocated on the fly. --- src/network/bridge_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f1be954..e2b8d06 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4005,11 +4005,21 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) vlan = &iface->vlan; else if (portgroup && portgroup->vlan.nTags > 0) vlan = &portgroup->vlan; - else if (netdef && netdef->vlan.nTags > 0) + else if (netdef->vlan.nTags > 0) vlan = &netdef->vlan;
Correct. If netdef was NULL, that would mean (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK), and in that case we would have skipped over this code down to validate:
- if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) - goto error; + if (vlan) { + /* data.network.actual may be NULL here when netdef->foward.type is + * VIR_NETWORK_FORWARD_{NONE|NAT|ROUTE} + */ + if (!iface->data.network.actual + && (VIR_ALLOC(iface->data.network.actual) < 0)) { + virReportOOMError(); + goto error; + }
Hmm. This ends up giving us an actual with no type set, which tells me we should have allocated this prior to now, which points out that I added this bit of code in the wrong place. I'll send an alternate patch momentarily.
+ if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) + goto error; + }
validate: /* make sure that everything now specified for the device is
From: John Ferlan <jferlan@redhat.com> Remove extraneous check for 'netdef' when dereferencing for vlan.nTags. Prior code would already check if netdef was NULL. Coverity complained about a path where the 'vlan' was potentially valid, but a prior checks may not have allocated 'iface->data.network.actual', so like other paths it needs to be allocated on the fly. Move the copying of vlan up earlier in networkAllocateActualDevice, so that actual.type gets properly set. Since the first assignment to vlan is redundant except in the case of jumping immediately to validate from the start of the function, eliminate its initial setting at the top of the function in favor of calling the helper function virDomainNetGetActualVlan() (which doesn't depend on the local vlan pointer being initialized) down at validate: --- Difference's between John's V1 and this V2 are described in paragraphs 2 and 3 above. I moved the lines that John changed, but left his changes intact. src/network/bridge_driver.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f1be954..c3cb63d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2,7 +2,7 @@ /* * bridge_driver.c: core driver methods for managing network * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -3715,10 +3715,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) int ii; int ret = -1; - /* it's handy to have this initialized if we skip directly to validate */ - if (iface->vlan.nTags > 0) - vlan = &iface->vlan; - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) goto validate; @@ -3764,6 +3760,24 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) goto error; } + /* copy appropriate vlan info to actualNet */ + if (iface->vlan.nTags > 0) + vlan = &iface->vlan; + else if (portgroup && portgroup->vlan.nTags > 0) + vlan = &portgroup->vlan; + else if (netdef->vlan.nTags > 0) + vlan = &netdef->vlan; + + if (vlan) { + if (!iface->data.network.actual + && (VIR_ALLOC(iface->data.network.actual) < 0)) { + virReportOOMError(); + goto error; + } + if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) + goto error; + } + if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) { @@ -4000,24 +4014,13 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) goto error; - /* copy appropriate vlan info to actualNet */ - if (iface->vlan.nTags > 0) - vlan = &iface->vlan; - else if (portgroup && portgroup->vlan.nTags > 0) - vlan = &portgroup->vlan; - else if (netdef && netdef->vlan.nTags > 0) - vlan = &netdef->vlan; - - if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) - goto error; - validate: /* make sure that everything now specified for the device is * actually supported on this type of network. NB: network, * netdev, and iface->data.network.actual may all be NULL. */ - if (vlan) { + if (virDomainNetGetActualVlan(iface)) { /* vlan configuration via libvirt is only supported for * PCI Passthrough SR-IOV devices and openvswitch bridges. * otherwise log an error and fail -- 1.7.11.7
On 01/16/2013 12:55 PM, Laine Stump wrote:
From: John Ferlan <jferlan@redhat.com>
Remove extraneous check for 'netdef' when dereferencing for vlan.nTags. Prior code would already check if netdef was NULL.
Coverity complained about a path where the 'vlan' was potentially valid, but a prior checks may not have allocated 'iface->data.network.actual', so like other paths it needs to be allocated on the fly.
Move the copying of vlan up earlier in networkAllocateActualDevice, so that actual.type gets properly set.
Since the first assignment to vlan is redundant except in the case of jumping immediately to validate from the start of the function, eliminate its initial setting at the top of the function in favor of calling the helper function virDomainNetGetActualVlan() (which doesn't depend on the local vlan pointer being initialized) down at validate:
---
Difference's between John's V1 and this V2 are described in paragraphs 2 and 3 above. I moved the lines that John changed, but left his changes intact.
src/network/bridge_driver.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)
ACK The change removes the Coverity warning. John
On 01/17/2013 11:05 AM, John Ferlan wrote:
On 01/16/2013 12:55 PM, Laine Stump wrote:
From: John Ferlan <jferlan@redhat.com>
Remove extraneous check for 'netdef' when dereferencing for vlan.nTags. Prior code would already check if netdef was NULL.
Coverity complained about a path where the 'vlan' was potentially valid, but a prior checks may not have allocated 'iface->data.network.actual', so like other paths it needs to be allocated on the fly.
Move the copying of vlan up earlier in networkAllocateActualDevice, so that actual.type gets properly set.
Since the first assignment to vlan is redundant except in the case of jumping immediately to validate from the start of the function, eliminate its initial setting at the top of the function in favor of calling the helper function virDomainNetGetActualVlan() (which doesn't depend on the local vlan pointer being initialized) down at validate:
---
Difference's between John's V1 and this V2 are described in paragraphs 2 and 3 above. I moved the lines that John changed, but left his changes intact.
src/network/bridge_driver.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)
ACK
The change removes the Coverity warning.
Pushed. Thanks!
The VIR_IS_CONNECTED_DOMAIN() will already check/return -1 if domain or domain->conn are NULL. --- src/xen/xs_internal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 9308522..4ccab20 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -382,7 +382,7 @@ xenStoreGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) if (!VIR_IS_CONNECTED_DOMAIN(domain)) return -1; - if ((domain == NULL) || (domain->conn == NULL) || (info == NULL)) { + if (info == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; } -- 1.7.11.7
On Tue, Jan 15, 2013 at 01:35:35PM -0500, John Ferlan wrote:
The VIR_IS_CONNECTED_DOMAIN() will already check/return -1 if domain or domain->conn are NULL. --- src/xen/xs_internal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 9308522..4ccab20 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -382,7 +382,7 @@ xenStoreGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) if (!VIR_IS_CONNECTED_DOMAIN(domain)) return -1;
- if ((domain == NULL) || (domain->conn == NULL) || (info == NULL)) { + if (info == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; }
Actually this entire check should be removed, along with the VIR_IS_CONNECTED_DOMAIN check. This is obsolete code, because the same thing is now checked in the caller (src/libvirt.c virDomainGetInfo). Likewise for similar code in this & other files in src/xen/ Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 10b8a5d..d259043 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -577,7 +577,7 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name) /* MAC address */ ifacedef->mac = strdup(udev_device_get_sysattr_value(dev, "address")); - if (!ifacedef) { + if (!ifacedef->mac) { virReportOOMError(); goto cleanup; } -- 1.7.11.7
On 01/15/13 22:31, Peter Krempa wrote:
On 01/15/13 19:35, John Ferlan wrote:
--- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK. (And will push in a while.)
Pushed now.
Peter
--- src/xen/xen_hypervisor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a770f53..ded1f0a 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -3284,7 +3284,7 @@ xenHypervisorGetDomainState(virDomainPtr domain, int *reason, unsigned int flags) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv; virDomainInfo info; virCheckFlags(0, -1); @@ -3292,6 +3292,7 @@ xenHypervisorGetDomainState(virDomainPtr domain, if (domain->conn == NULL) return -1; + priv = (xenUnifiedPrivatePtr) domain->conn->privateData; if (priv->handle < 0 || domain->id < 0) return -1; -- 1.7.11.7
On 01/15/2013 11:35 AM, John Ferlan wrote:
--- src/xen/xen_hypervisor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a770f53..ded1f0a 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -3284,7 +3284,7 @@ xenHypervisorGetDomainState(virDomainPtr domain, int *reason, unsigned int flags) { - xenUnifiedPrivatePtr priv = domain->conn->privateData; + xenUnifiedPrivatePtr priv; virDomainInfo info;
virCheckFlags(0, -1); @@ -3292,6 +3292,7 @@ xenHypervisorGetDomainState(virDomainPtr domain, if (domain->conn == NULL) return -1;
+ priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
This should be another one of those cases like Dan mentioned for 2/5 where the caller guaranteed that domain->conn is valid, and we can get rid of the duplicate sanity checking here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/openvz/openvz_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index c6f814e..a7cfada 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2053,13 +2053,13 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, openvzDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); - vmdef = vm->def; if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, "%s", _("no domain with matching uuid")); goto cleanup; } + vmdef = vm->def; if (virStrToLong_i(vmdef->name, NULL, 10, &veid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.7.11.7
On 01/15/13 22:34, Peter Krempa wrote:
On 01/15/13 19:35, John Ferlan wrote:
--- src/openvz/openvz_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
and pushed.
Peter
participants (5)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Laine Stump -
Peter Krempa