12:35 p.m.
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
12:35 p.m.
New subject: [libvirt] [PATCH 1/5] network: Resolve some issues around vlan copying
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
1:16 p.m.
New subject: [libvirt] [PATCH 1/5] network: Resolve some issues around vlan copying
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
11:55 a.m.
New subject: [libvirt] [PATCH] network: Resolve some issues around vlan copying
From: John Ferlan <jferlan(a)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
10:05 a.m.
New subject: [libvirt] [PATCH] network: Resolve some issues around vlan copying
On 01/16/2013 12:55 PM, Laine Stump wrote:
From: John Ferlan <jferlan(a)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
11:40 a.m.
New subject: [libvirt] [PATCH] network: Resolve some issues around vlan copying
On 01/17/2013 11:05 AM, John Ferlan wrote:
On 01/16/2013 12:55 PM, Laine Stump wrote:
> From: John Ferlan <jferlan(a)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!
12:35 p.m.
New subject: [libvirt] [PATCH 2/5] xen: Remove extraneous checks in xenStoreGetDomainInfo()
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
12:42 p.m.
New subject: [libvirt] [PATCH 2/5] xen: Remove extraneous checks in xenStoreGetDomainInfo()
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 :|
12:35 p.m.
New subject: [libvirt] [PATCH 3/5] interface: Need to check ifacedef->mac not just ifacedef after strdup()
---
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
4:45 p.m.
New subject: [libvirt] [PATCH 3/5] interface: Need to check ifacedef->mac not just ifacedef after strdup()
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
12:35 p.m.
New subject: [libvirt] [PATCH 4/5] xen: Need to check validity of domain->conn before deref privateData
---
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
4:48 p.m.
New subject: [libvirt] [PATCH 4/5] xen: Need to check validity of domain->conn before deref privateData
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
12:35 p.m.
New subject: [libvirt] [PATCH 5/5] openvz: Need to check 'vm' first before dereferencing 'def'
---
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
4:46 p.m.
New subject: [libvirt] [PATCH 5/5] openvz: Need to check 'vm' first before dereferencing 'def'
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
4442
days inactive
4444
days old
15 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Laine Stump -
Peter Krempa