[libvirt] [PATCH] storage: avoid null deref and leak on failure
by Eric Blake
Detected by clang. NULL deref added in commit 343a27a (Mar 11),
but leak of voldef present since commit 2cd9b2d (Apr 09).
* src/storage/storage_driver.c (storageVolumeCreateXML): Don't
leak voldef or dereference null volobj.
---
src/storage/storage_driver.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 1ea5d12..19c7d63 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1319,8 +1319,10 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
pool->volumes.objs[pool->volumes.count++] = voldef;
volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
voldef->key);
+ if (!volobj)
+ goto cleanup;
- if (volobj && backend->buildVol) {
+ if (backend->buildVol) {
int buildret;
virStorageVolDefPtr buildvoldef = NULL;
--
1.7.4.4
13 years, 6 months
[libvirt] qemu qxl video memory parameter - windows bsod with spice
by Emre Erenoglu
Hi,
Please see the below discussion with the spice community.
libvirt is adding a parameter about video ram to qemu process and this
parameter might be responsible of a crash in the qxl at higher resolutions.
Spice people tell me that their default ram should be 64 Mbytes.
I'm using libvirt 0.9.0 and qemu 0.14, virt-manager 0.8.7. When configuring
a guest in virt-manager, cirrus driver is added as default, then you can
change it to qxl, but the 9 MB videoram parameter can't be adjusted and it
does not automatically change to 64 MB. If you add a 2nd qxl video card,
then it has 64 MB memory.
The parameter that qemu gets with one qxl device is:
-vga qxl -global qxl-vga.vram_size=9437184
The parameter that qemu gets with a second qxl device is:
-vga qxl -global qxl-vga.vram_size=9437184 -device
qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x8
Is this a bug in libvirt or other related tools?
Thanks,
Br,
Emre
---------- Forwarded message ----------
From: Emre Erenoglu <erenoglu(a)gmail.com>
Date: Wed, May 4, 2011 at 7:42 PM
Subject: Re: [Spice-devel] windows bsod with spice 0.8.1
To: Emre Erenoglu <erenoglu(a)gmail.com>, spice-devel(a)freedesktop.org
On Wed, May 4, 2011 at 6:00 PM, Alon Levy <alevy(a)redhat.com> wrote:
> On Wed, May 04, 2011 at 05:10:31PM +0400, Emre Erenoglu wrote:
> > On Wed, May 4, 2011 at 2:12 PM, Alon Levy <alevy(a)redhat.com> wrote:
> >
> > > On Wed, May 04, 2011 at 01:45:39PM +0400, Emre Erenoglu wrote:
> > > > On Wed, Apr 27, 2011 at 1:16 AM, Emre Erenoglu <erenoglu(a)gmail.com>
> > > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I started to see frequent crashes with my guests running windows xp
> and
> > > > > windows server 2003 x86. The bsod appears on qxldd.dll. I can give
> > > further
> > > > > info if needed.
> > > > >
> > > > > The host is Linux x86_64, qemu 0.14, spice 0.8.1. Guest has qxl
> 0.6.1
> > > > > drivers loaded. I may revert back to 0.8.0 since it was not
> crashing.
> > > > > Nothing changed in the guests when I upgraded spice from 0.8.0 to
> > > 0.8.1.
> > > > >
> > > > >
> > > > Some updates on the original issue. I checked a lot and I found out
> that
> > > the
> > > > issue continued with any version of spice and guest qxl drivers.
> > > >
> > > > I finally digged down to the point that, the qemu comment line which
> > > states
> > > > " -global qxl-vga.vram_size=9xxxxxx " parameter is the problem.
> When
> > > the
> > > > guest is started with 9 MB of video memory given as comment line
> > > parameter
> > > > to qemu, the guest qxldd driver crashes with BSOD.
> > >
> > > Why was it started with 9MB? anyway, good catch, we should add a check
> for
> > > this
> > > in the driver (and probably just refuse to load). Could you open a bug
> on
> > > this
> > > in bugzilla.freedesktop.org?
> > >
> >
> > This is added by libvirt (virt-manager) when launching qemu. Do we need
> to
> > file it against libvirt or against spice? I will post this to the
> libvirt
> > list also.
> >
> > Btw, rather than refusing to load, I think we should just limit the
> possible
> > resolutions. I'm not sure if the resolutions are the issue though. The
> crash
> > was not happening immediately and it was working OK with that 9 MB
> memory.
> > When I launched outlook, or IE8, or just in the middle of an application,
> it
> > could crash and bsod.
>
> But that number doesn't make any sense - the spice default is 64MB, we
> never tested
> with less then that. It is probably fixable like you say, but until that's
> fixed why
> not stick to the spice default?
Well, this parameter is not configurable from the virt-manager UI and it
configures 9 MB to the command line of qemu by libvirt (even latest 0.9.0)
version.
When you add a 2nd VGA adapter, it adds 64 MB to that adapter correctly
(-device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x8)
I guess i need to file a bug report to libvirt too.
Emre
13 years, 6 months
[libvirt] [PATCH] esx: avoid null dereference on error
by Eric Blake
Detected by clang.
* src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error.
---
src/esx/esx_driver.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 1f8f90b..e929208 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (perfEntityMetric == NULL) {
VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetricBase->_type));
+ goto cleanup;
}
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] remote: avoid null dereference on error
by Eric Blake
Clang found three instances of uninitialized use of nparams in
the cleanup path. Unfortunately, one is a false positive: clang
couldn't see that ret->params.params_val is guaranteed to be
NULL unless allocated within a function, and that nparams is
guaranteed to be assigned prior to the allocation; hoisting the
assignment to nparams to be earlier in the function shuts up
that false positive. But two of the reports also happened to
highlight a real bug - the error path can dereference NULL.
* daemon/remote.c (remoteDispatchDomainGetMemoryParameters)
(remoteDispatchDomainGetBlkioParameters): Don't clear fields if
array was not allocated.
(remoteDispatchDomainGetSchedulerParameters): Initialize nparams
earlier.
---
daemon/remote.c | 29 ++++++++++++++++-------------
1 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index eedbc77..214f7a4 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -886,7 +886,8 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
{
virDomainPtr dom = NULL;
virSchedParameterPtr params = NULL;
- int i, nparams;
+ int i;
+ int nparams = args->nparams;
int rv = -1;
if (!conn) {
@@ -894,8 +895,6 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
goto cleanup;
}
- nparams = args->nparams;
-
if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
goto cleanup;
@@ -2970,7 +2969,8 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
{
virDomainPtr dom = NULL;
virMemoryParameterPtr params = NULL;
- int i, nparams;
+ int i;
+ int nparams = args->nparams;
unsigned int flags;
int rv = -1;
@@ -2979,7 +2979,6 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
goto cleanup;
}
- nparams = args->nparams;
flags = args->flags;
if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
@@ -3060,9 +3059,11 @@ success:
cleanup:
if (rv < 0) {
remoteDispatchError(rerr);
- for (i = 0; i < nparams; i++)
- VIR_FREE(ret->params.params_val[i].field);
- VIR_FREE(ret->params.params_val);
+ if (ret->params.params_val) {
+ for (i = 0; i < nparams; i++)
+ VIR_FREE(ret->params.params_val[i].field);
+ VIR_FREE(ret->params.params_val);
+ }
}
if (dom)
virDomainFree(dom);
@@ -3186,7 +3187,8 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
{
virDomainPtr dom = NULL;
virBlkioParameterPtr params = NULL;
- int i, nparams;
+ int i;
+ int nparams = args->nparams;
unsigned int flags;
int rv = -1;
@@ -3195,7 +3197,6 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
goto cleanup;
}
- nparams = args->nparams;
flags = args->flags;
if (nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) {
@@ -3276,9 +3277,11 @@ success:
cleanup:
if (rv < 0) {
remoteDispatchError(rerr);
- for (i = 0; i < nparams; i++)
- VIR_FREE(ret->params.params_val[i].field);
- VIR_FREE(ret->params.params_val);
+ if (ret->params.params_val) {
+ for (i = 0; i < nparams; i++)
+ VIR_FREE(ret->params.params_val[i].field);
+ VIR_FREE(ret->params.params_val);
+ }
}
VIR_FREE(params);
if (dom)
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] esx: remove dead store
by Eric Blake
Detected by clang.
* src/esx/esx_util.c (esxUtil_ParseDatastorePath): No need to
increment.
---
src/esx/esx_util.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 9ef947c..640e984 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -2,7 +2,7 @@
/*
* esx_util.c: utility functions for the VMware ESX driver
*
- * Copyright (C) 2010 Red Hat, Inc.
+ * Copyright (C) 2010-2011 Red Hat, Inc.
* Copyright (C) 2009 Matthias Bolte <matthias.bolte(a)googlemail.com>
* Copyright (C) 2009 Maximilian Wilhelm <max(a)rfc2324.org>
*
@@ -332,7 +332,7 @@ esxUtil_ParseDatastorePath(const char *datastorePath, char **datastoreName,
preliminaryFileName = strrchr(preliminaryDirectoryAndFileName, '/');
if (preliminaryFileName != NULL) {
- *preliminaryFileName++ = '\0';
+ *preliminaryFileName = '\0';
}
if (esxVI_String_DeepCopyValue(directoryName,
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] cgroup: avoid leaking a file
by Eric Blake
Clang detected a dead store to rc. It turns out that in fixing this,
I also found a FILE* leak.
* src/util/cgroup.c (virCgroupKillInternal): Abort rather than
resuming loop on fscanf failure, and cleanup file on error.
---
As a bonus, this also fixes a decl-after-statement for fp.
src/util/cgroup.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index afe8731..62b371d 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -1351,7 +1351,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
int killedAny = 0;
char *keypath = NULL;
bool done = false;
- VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids);
+ FILE *fp = NULL;
+ VIR_DEBUG("group=%p path=%s signum=%d pids=%p",
+ group, group->path, signum, pids);
rc = virCgroupPathOfController(group, -1, "tasks", &keypath);
if (rc != 0) {
@@ -1364,7 +1366,6 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
*/
while (!done) {
done = true;
- FILE *fp;
if (!(fp = fopen(keypath, "r"))) {
rc = -errno;
VIR_DEBUG("Failed to read %s: %m\n", keypath);
@@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
if (feof(fp))
break;
rc = -errno;
- break;
+ VIR_DEBUG("Failed to read %s: %m\n", keypath);
+ goto cleanup;
}
if (virHashLookup(pids, (void*)pid))
continue;
@@ -1403,6 +1405,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
cleanup:
VIR_FREE(keypath);
+ VIR_FORCE_FCLOSE(fp);
return rc;
}
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] qemu: update qemuCgroupControllerActive signature
by Eric Blake
Clang warned about a dead assignment. In the process, I noticed
that we are only using the function for a bool value. I audited
all other callers in qemu_{migration,cgroup,driver,hotplug), and
all were making the call in a bool context.
* src/qemu/qemu_cgroup.c (qemuSetupCgroup): Delete dead
assignment.
(qemuCgroupControllerActive): Change return type to bool.
* src/qemu/qemu_cgroup.h (qemuCgroupControllerActive): Likewise.
---
src/qemu/qemu_cgroup.c | 14 +++++++-------
src/qemu/qemu_cgroup.h | 4 ++--
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 7e88a67..ac1c016 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -43,16 +43,16 @@ static const char *const defaultDeviceACL[] = {
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
-int qemuCgroupControllerActive(struct qemud_driver *driver,
- int controller)
+bool qemuCgroupControllerActive(struct qemud_driver *driver,
+ int controller)
{
if (driver->cgroup == NULL)
- return 0;
+ return false;
if (!virCgroupMounted(driver->cgroup, controller))
- return 0;
+ return false;
if (driver->cgroupControllers & (1 << controller))
- return 1;
- return 0;
+ return true;
+ return false;
}
static int
@@ -312,7 +312,7 @@ int qemuSetupCgroup(struct qemud_driver *driver,
if (vm->def->mem.hard_limit != 0 ||
vm->def->mem.soft_limit != 0 ||
vm->def->mem.swap_hard_limit != 0) {
- if ((rc = qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY))) {
+ if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
if (vm->def->mem.hard_limit != 0) {
rc = virCgroupSetMemoryHardLimit(cgroup, vm->def->mem.hard_limit);
if (rc != 0) {
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 299bd2d..e8abfb4 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -34,8 +34,8 @@ struct _qemuCgroupData {
};
typedef struct _qemuCgroupData qemuCgroupData;
-int qemuCgroupControllerActive(struct qemud_driver *driver,
- int controller);
+bool qemuCgroupControllerActive(struct qemud_driver *driver,
+ int controller);
int qemuSetupDiskCgroup(struct qemud_driver *driver,
virDomainObjPtr vm,
virCgroupPtr cgroup,
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] lxc: report correct error
by Eric Blake
Clang noticed a dead assignment, which turned out to be the use
of the wrong variable. rc starts life as -1, and is only ever
assigned to 0 just before a successful cleanup.
* src/lxc/lxc_driver.c (lxcSetupInterfaces): Don't call
virReportSystemError(-1).
---
src/lxc/lxc_driver.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e905302..b94941d 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1100,10 +1100,9 @@ static int lxcSetupInterfaces(virConnectPtr conn,
}
if ((ret = brAddInterface(brctl, bridge, parentVeth)) != 0) {
- virReportSystemError(rc,
+ virReportSystemError(ret,
_("Failed to add %s device to %s"),
parentVeth, bridge);
- rc = -1;
goto error_exit;
}
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] util: remove dead assignment
by Eric Blake
Clang complained about this, and it was easy enough to fix.
* src/util/util.c (virFileOpenAs): Drop dead assignment.
---
src/util/util.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 37472bb..9041ab6 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1523,8 +1523,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
if (ret < 0 && errno != EACCES) {
ret = -errno;
VIR_FORCE_CLOSE(pair[0]);
- while ((waitret = waitpid(pid, NULL, 0) == -1)
- && (errno == EINTR));
+ while (waitpid(pid, NULL, 0) == -1 && errno == EINTR);
goto parenterror;
} else {
fd = ret;
--
1.7.4.4
13 years, 6 months
[libvirt] [PATCH] libxl: avoid compiler warning
by Eric Blake
Detected by gcc:
libxl/libxl_driver.c: In function 'libxlDomainDestroy':
libxl/libxl_drier.c:1351:30: error: variable 'priv' set but not used [-Werror=unused-but-set-variable]
* src/libxl/libxl_driver.c (libxlDomainDestroy): Delete unused
variable.
---
Trivial, so pushing under the build-breaker rule.
src/libxl/libxl_driver.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 247d78e..dec4f43 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1348,7 +1348,6 @@ libxlDomainDestroy(virDomainPtr dom)
libxlDriverPrivatePtr driver = dom->conn->privateData;
virDomainObjPtr vm;
int ret = -1;
- libxlDomainObjPrivatePtr priv;
virDomainEventPtr event = NULL;
libxlDriverLock(driver);
@@ -1370,7 +1369,6 @@ libxlDomainDestroy(virDomainPtr dom)
event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
- priv = vm->privateData;
if (libxlVmReap(driver, vm, 1) != 0) {
libxlError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"), dom->id);
--
1.7.4.4
13 years, 6 months