[libvirt] [PATCH 0/6] Resolve FORWARD_NULL errors found by Coverity
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=880388 This set of patches resolves "Error: FORWARD_NULL (CWE-476)" errors found by Coverity. John Ferlan (6): xen: Avoid possible NULL dereference vmware: Avoid NULL dereference for 'caps' remote: Avoid calling virAuthConfigLookup() if 'credname' is NULL. lxc: Check for non NULL usb prior to calling usbFreeDevice() lxc: Avoid possible NULL dereference on *root prior to opendir(). cpu: Avoid NULL dereference src/cpu/cpu_powerpc.c | 3 ++- src/lxc/lxc_container.c | 6 ++++++ src/lxc/lxc_driver.c | 6 ++++-- src/remote/remote_driver.c | 3 ++- src/vmware/vmware_conf.c | 3 ++- src/xen/xen_driver.c | 6 +++--- 6 files changed, 19 insertions(+), 8 deletions(-) -- 1.7.11.7
Change calling sequence to only call xenUnifiedDomainSetVcpusFlags() when 'dom' is not NULL. Use the GET_PRIVATE() macro to reference privateData. Just return -1 if dom is NULL. --- src/xen/xen_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 2ef2c57..559025e 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1253,17 +1253,17 @@ static int xenUnifiedDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { unsigned int flags = VIR_DOMAIN_VCPU_LIVE; - xenUnifiedPrivatePtr priv; /* Per the documented API, it is hypervisor-dependent whether this * affects just _LIVE or _LIVE|_CONFIG; in xen's case, that * depends on xendConfigVersion. */ if (dom) { - priv = dom->conn->privateData; + GET_PRIVATE(dom->conn); if (priv->xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) flags |= VIR_DOMAIN_VCPU_CONFIG; + return xenUnifiedDomainSetVcpusFlags(dom, nvcpus, flags); } - return xenUnifiedDomainSetVcpusFlags(dom, nvcpus, flags); + return -1; } static int -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:29PM -0500, John Ferlan wrote:
Change calling sequence to only call xenUnifiedDomainSetVcpusFlags() when 'dom' is not NULL. Use the GET_PRIVATE() macro to reference privateData. Just return -1 if dom is NULL. --- src/xen/xen_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 2ef2c57..559025e 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1253,17 +1253,17 @@ static int xenUnifiedDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { unsigned int flags = VIR_DOMAIN_VCPU_LIVE; - xenUnifiedPrivatePtr priv;
/* Per the documented API, it is hypervisor-dependent whether this * affects just _LIVE or _LIVE|_CONFIG; in xen's case, that * depends on xendConfigVersion. */ if (dom) { - priv = dom->conn->privateData; + GET_PRIVATE(dom->conn); if (priv->xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) flags |= VIR_DOMAIN_VCPU_CONFIG; + return xenUnifiedDomainSetVcpusFlags(dom, nvcpus, flags); } - return xenUnifiedDomainSetVcpusFlags(dom, nvcpus, flags); + return -1; }
static int
ACK 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 :|
When virCapabilitiesNew() fails, caps will be NULL resulting in possible core when deref'd in cpuDataFree() call. --- src/vmware/vmware_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index b514636..fd9c473 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -127,7 +127,8 @@ vmwareCapsInit(void) cleanup: virCPUDefFree(cpu); - cpuDataFree(caps->host.arch, data); + if (caps) + cpuDataFree(caps->host.arch, data); return caps; -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:30PM -0500, John Ferlan wrote:
When virCapabilitiesNew() fails, caps will be NULL resulting in possible core when deref'd in cpuDataFree() call. --- src/vmware/vmware_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index b514636..fd9c473 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -127,7 +127,8 @@ vmwareCapsInit(void)
cleanup: virCPUDefFree(cpu); - cpuDataFree(caps->host.arch, data); + if (caps) + cpuDataFree(caps->host.arch, data);
return caps;
ACK 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/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c078cb5..65fcc5b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3738,7 +3738,8 @@ static int remoteAuthFillFromConfig(virConnectPtr conn, break; } - if (virAuthConfigLookup(state->config, + if (credname && + virAuthConfigLookup(state->config, "libvirt", VIR_URI_SERVER(conn->uri), credname, -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:31PM -0500, John Ferlan wrote:
--- src/remote/remote_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c078cb5..65fcc5b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3738,7 +3738,8 @@ static int remoteAuthFillFromConfig(virConnectPtr conn, break; }
- if (virAuthConfigLookup(state->config, + if (credname && + virAuthConfigLookup(state->config, "libvirt", VIR_URI_SERVER(conn->uri), credname,
ACK 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/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8050ce6..d4b4989 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3468,7 +3468,8 @@ cleanup: if (ret < 0 && created) unlink(dstfile); - usbFreeDevice(usb); + if (usb) + usbFreeDevice(usb); virCgroupFree(&group); VIR_FREE(src); VIR_FREE(dstfile); @@ -4022,7 +4023,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, ret = 0; cleanup: - usbFreeDevice(usb); + if (usb) + usbFreeDevice(usb); VIR_FREE(dst); virCgroupFree(&group); return ret; -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:32PM -0500, John Ferlan wrote:
--- src/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8050ce6..d4b4989 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3468,7 +3468,8 @@ cleanup: if (ret < 0 && created) unlink(dstfile);
- usbFreeDevice(usb); + if (usb) + usbFreeDevice(usb); virCgroupFree(&group); VIR_FREE(src); VIR_FREE(dstfile); @@ -4022,7 +4023,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, ret = 0;
cleanup: - usbFreeDevice(usb); + if (usb) + usbFreeDevice(usb); VIR_FREE(dst); virCgroupFree(&group); return ret;
NACK to this one. Can you change the 'usbFreeDevice' method to contain a line if (!dev) return so that it matches our preferred coding style of allowing NULLS to 'free()' like methods. Also then add that method to the variable 'useless_free_options' in cfg.mk Regards, 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 :|
If running on older Linux without mounted cgroups then its possible that *root would be NULL. --- src/lxc/lxc_container.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index d3a2968..d234426 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1762,6 +1762,12 @@ static int lxcContainerIdentifyCGroups(struct lxcContainerCGroup **mountsret, VIR_DEBUG("Grabbed '%s'", mntent.mnt_dir); } + if (!*root) { + VIR_DEBUG("No mounted cgroups found"); + ret = 0; + goto cleanup; + } + VIR_DEBUG("Checking for symlinks in %s", *root); if (!(dh = opendir(*root))) { virReportSystemError(errno, -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:33PM -0500, John Ferlan wrote:
If running on older Linux without mounted cgroups then its possible that *root would be NULL. --- src/lxc/lxc_container.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index d3a2968..d234426 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1762,6 +1762,12 @@ static int lxcContainerIdentifyCGroups(struct lxcContainerCGroup **mountsret, VIR_DEBUG("Grabbed '%s'", mntent.mnt_dir); }
+ if (!*root) { + VIR_DEBUG("No mounted cgroups found"); + ret = 0; + goto cleanup; + } + VIR_DEBUG("Checking for symlinks in %s", *root); if (!(dh = opendir(*root))) { virReportSystemError(errno,
ACK 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 :|
Don't dereference 'model' in PowerPCBaseline when there's no outputModel --- src/cpu/cpu_powerpc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 8bef627..5e1a7b9 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -605,7 +605,8 @@ PowerPCBaseline(virCPUDefPtr *cpus, goto error; } - base_model->data->ppc.pvr = model->data->ppc.pvr; + if (outputModel) + base_model->data->ppc.pvr = model->data->ppc.pvr; if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) goto error; -- 1.7.11.7
On Mon, Jan 07, 2013 at 12:09:34PM -0500, John Ferlan wrote:
Don't dereference 'model' in PowerPCBaseline when there's no outputModel --- src/cpu/cpu_powerpc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 8bef627..5e1a7b9 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -605,7 +605,8 @@ PowerPCBaseline(virCPUDefPtr *cpus, goto error; }
- base_model->data->ppc.pvr = model->data->ppc.pvr; + if (outputModel) + base_model->data->ppc.pvr = model->data->ppc.pvr; if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) goto error;
ACK 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 :|
On 01/07/2013 11:29 AM, Daniel P. Berrange wrote:
On Mon, Jan 07, 2013 at 12:09:34PM -0500, John Ferlan wrote:
Don't dereference 'model' in PowerPCBaseline when there's no outputModel --- src/cpu/cpu_powerpc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 8bef627..5e1a7b9 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -605,7 +605,8 @@ PowerPCBaseline(virCPUDefPtr *cpus, goto error; }
- base_model->data->ppc.pvr = model->data->ppc.pvr; + if (outputModel) + base_model->data->ppc.pvr = model->data->ppc.pvr; if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) goto error;
ACK
I've pushed 1-3 and 5-6; waiting for a v2 of patch 4. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan