From: "Daniel P. Berrange" <berrange@redhat.com> If virDomainVcpuPinDefArrayFree is called with def != NULL, but nvcpupin == 0, then it leaks memory for 'def'. This is an unusual scenario, but it hits when cleaning up after an OOM during parsing of XML. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 240f318..81314bc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1853,7 +1853,7 @@ virDomainVcpuPinDefArrayFree(virDomainVcpuPinDefPtr *def, { size_t i; - if (!def || !nvcpupin) + if (!def) return; for (i = 0; i < nvcpupin; i++) { -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If virDomainSoundCodecDefParseXML returns an error (eg due to OOM), then the xml nodeset codecNodes is leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 81314bc..ccb61aa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8470,8 +8470,10 @@ virDomainSoundDefParseXML(const xmlNodePtr node, for (i = 0; i < ncodecs; i++) { virDomainSoundCodecDefPtr codec = virDomainSoundCodecDefParseXML(codecNodes[i]); - if (codec == NULL) + if (codec == NULL) { + VIR_FREE(codecNodes); goto error; + } codec->cad = def->ncodecs; /* that will do for now */ def->codecs[def->ncodecs++] = codec; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If the virBitmapParse method fails due to OOM, we leak the 'tmp' variable string. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ccb61aa..aab6781 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10614,8 +10614,10 @@ virDomainVcpuPinDefParseXML(const xmlNodePtr node, int cpumasklen = VIR_DOMAIN_CPUMASK_LEN; if (virBitmapParse(set, 0, &def->cpumask, - cpumasklen) < 0) - goto error; + cpumasklen) < 0) { + VIR_FREE(tmp); + goto error; + } VIR_FREE(tmp); } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If virBitmapNew fails due to OOM, the 'vcpupin' variable is leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/domain_conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aab6781..4ff7593 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11192,8 +11192,10 @@ virDomainDefParseXML(xmlDocPtr xml, if (VIR_ALLOC(vcpupin) < 0) goto error; - if (!(vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) + if (!(vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { + VIR_FREE(vcpupin); goto error; + } virBitmapCopy(vcpupin->cpumask, def->cpumask); vcpupin->vcpuid = i; def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> qemuDomainPCIAddressGetNextSlot has a loop for finding compatible PCI buses. In the loop body it creates a PCI address string, but never frees this. This causes a leak if the loop executes more than one iteration, or if a call in the loop body fails. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0376611..3296d09 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2194,12 +2194,12 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, /* Start the search at the last used bus and slot */ for (a.slot++; a.bus < addrs->nbuses; a.bus++) { - addrStr = NULL; if (!(addrStr = qemuDomainPCIAddressAsString(&a))) goto error; if (!qemuDomainPCIAddressFlagsCompatible(&a, addrStr, addrs->buses[a.bus].flags, flags, false, false)) { + VIR_FREE(addrStr); VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); continue; @@ -2212,6 +2212,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, a.domain, a.bus, a.slot); } a.slot = 1; + VIR_FREE(addrStr); } /* There were no free slots after the last used one */ -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If qemuAddRBDHost fails due to parsing problems or OOM, then qemuParseRBDString cleanup is skipped causing a memory leak. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3296d09..bf07bdc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3325,9 +3325,9 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk) break; } } - if (qemuAddRBDHost(disk, h) < 0) { - return -1; - } + if (qemuAddRBDHost(disk, h) < 0) + goto error; + h = sep; } } -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> In qemuParseNBDString, if the virURIParse fails, the error is not reported to the caller. Instead execution falls through to the non-URI codepath causing memory leaks later on. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bf07bdc..e0fd38e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3477,9 +3477,9 @@ qemuParseNBDString(virDomainDiskDefPtr disk) virURIPtr uri = NULL; if (strstr(disk->src, "://")) { - uri = virURIParse(disk->src); - if (uri) - return qemuParseDriveURIString(disk, uri, "nbd"); + if (!(uri = virURIParse(disk->src))) + return -1; + return qemuParseDriveURIString(disk, uri, "nbd"); } if (VIR_ALLOC(h) < 0) -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> The qemuBuildCommandLine code for parsing sound cards will leak an intermediate variable if an OOM occurs. Move the free'ing of the variable earlier to avoid the leak. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e0fd38e..a82c5dd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9009,7 +9009,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; virCommandAddArg(cmd, str); - + VIR_FREE(str); if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { char *codecstr = NULL; @@ -9036,8 +9036,6 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(codecstr); } } - - VIR_FREE(str); } } } else { -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If OOM occurs in qemuParseCommandLineDisk some intermediate variables will be leaked when parsing Sheepdog or RBD disks. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a82c5dd..f6a3aa2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9935,8 +9935,10 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_STRDUP(def->src, p + strlen("rbd:")) < 0) goto error; /* old-style CEPH_ARGS env variable is parsed later */ - if (!old_style_ceph_args && qemuParseRBDString(def) < 0) - goto cleanup; + if (!old_style_ceph_args && qemuParseRBDString(def) < 0) { + VIR_FREE(p); + goto error; + } VIR_FREE(p); } else if (STRPREFIX(def->src, "gluster:") || @@ -9960,17 +9962,20 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, def->protocol = VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG; if (VIR_STRDUP(def->src, p + strlen("sheepdog:")) < 0) goto error; + VIR_FREE(p); /* def->src must be [vdiname] or [host]:[port]:[vdiname] */ port = strchr(def->src, ':'); if (port) { - *port++ = '\0'; - vdi = strchr(port, ':'); + *port = '\0'; + vdi = strchr(port + 1, ':'); if (!vdi) { + *port = ':'; virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse sheepdog filename '%s'"), p); + _("cannot parse sheepdog filename '%s'"), def->src); goto error; } + port++; *vdi++ = '\0'; if (VIR_ALLOC(def->hosts) < 0) goto error; @@ -9983,8 +9988,6 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_STRDUP(def->src, vdi) < 0) goto error; } - - VIR_FREE(p); } else def->type = VIR_DOMAIN_DISK_TYPE_FILE; } else { -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> In a number of places in qemuParseCommandLineDisk, an error is reported, but no 'goto error' jump is used. This causes failure to report OOM conditions to the caler. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f6a3aa2..fe53cf7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10068,6 +10068,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if ((def->iomode = virDomainDiskIoTypeFromString(values[i])) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse io mode '%s'"), values[i]); + goto error; } } else if (STREQ(keywords[i], "cyls")) { if (virStrToLong_ui(values[i], NULL, 10, @@ -10077,6 +10078,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse cylinders value'%s'"), values[i]); + goto error; } } else if (STREQ(keywords[i], "heads")) { if (virStrToLong_ui(values[i], NULL, 10, @@ -10086,6 +10088,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse heads value'%s'"), values[i]); + goto error; } } else if (STREQ(keywords[i], "secs")) { if (virStrToLong_ui(values[i], NULL, 10, @@ -10095,6 +10098,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse sectors value'%s'"), values[i]); + goto error; } } else if (STREQ(keywords[i], "trans")) { def->geometry.trans = @@ -10106,6 +10110,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse translation value'%s'"), values[i]); + goto error; } } } -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> The 'qemuStringToArgvEnv' method splits up a string of command line env/args to an 'arglist' array. It then copies env vars to a 'progenv' array and args to a 'progargv' array. When copyin the env vars, it NULL-ifies the element in 'arglist' that is copied. Upon OOM the 'virStringListFree' is called on progenv and arglist. Unfortunately, because the elements in 'arglist' related to env vars have been set to NULL, the call to virStringListFree(arglist) doesn't free anything, even though some non-NULL args vars still exist later in the array. To fix this leak, stop NULL-ifying the 'arglist' elements, and change the cleanup code to only free elements in the 'arglist' array, not 'progenv'. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fe53cf7..7029335 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9724,10 +9724,8 @@ static int qemuStringToArgvEnv(const char *args, if (envend > 0) { if (VIR_REALLOC_N(progenv, envend+1) < 0) goto error; - for (i = 0; i < envend; i++) { + for (i = 0; i < envend; i++) progenv[i] = arglist[i]; - arglist[i] = NULL; - } progenv[i] = NULL; } @@ -9746,7 +9744,8 @@ static int qemuStringToArgvEnv(const char *args, return 0; error: - virStringFreeList(progenv); + VIR_FREE(progenv); + VIR_FREE(progargv); virStringFreeList(arglist); return -1; } -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If the call to virDomainControllerInsert fails in qemuParseCommandLine, the controller struct is leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7029335..a8b9a74 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11630,8 +11630,10 @@ qemuParseCommandLine(virCapsPtr qemuCaps, ctldef->type = VIR_DOMAIN_CONTROLLER_TYPE_USB; ctldef->idx = 0; ctldef->model = -1; - if (virDomainControllerInsert(def, ctldef) < 0) + if (virDomainControllerInsert(def, ctldef) < 0) { + VIR_FREE(ctldef); goto error; + } } else if (STREQ(arg, "-pidfile")) { WANT_VALUE(); if (pidfile) -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If qemuParseCommandLine finds an arg it does not understand it adds it to the QEMU passthrough custom arg list. If the qemuParseCommandLine method hits an error for any reason though, it just does 'VIR_FREE(cmd)' on the custom arg list. This means all actual args / env vars are leaked. Introduce a qemuDomainCmdlineDefFree method to be used for cleanup. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_conf.c | 18 ++++++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 15 +-------------- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a8b9a74..724e1d4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11868,13 +11868,13 @@ qemuParseCommandLine(virCapsPtr qemuCaps, def->namespaceData = cmd; } else - VIR_FREE(cmd); + qemuDomainCmdlineDefFree(cmd); return def; error: virDomainDiskDefFree(disk); - VIR_FREE(cmd); + qemuDomainCmdlineDefFree(cmd); virDomainDefFree(def); VIR_FREE(nics); if (monConfig) { diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 1f57f72..1a41caf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -87,6 +87,24 @@ qemuDriverUnlock(virQEMUDriverPtr driver) virMutexUnlock(&driver->lock); } +void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) +{ + size_t i; + + if (!def) + return; + + for (i = 0; i < def->num_args; i++) + VIR_FREE(def->args[i]); + for (i = 0; i < def->num_env; i++) { + VIR_FREE(def->env_name[i]); + VIR_FREE(def->env_value[i]); + } + VIR_FREE(def->args); + VIR_FREE(def->env_name); + VIR_FREE(def->env_value); + VIR_FREE(def); +} virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 206f2c6..da29a2a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -247,6 +247,8 @@ struct _qemuDomainCmdlineDef { # define QEMUD_MIGRATION_NUM_PORTS 64 +void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def); + virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged); int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f2cca70..968e323 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -535,21 +535,8 @@ static void qemuDomainDefNamespaceFree(void *nsdata) { qemuDomainCmdlineDefPtr cmd = nsdata; - size_t i; - - if (!cmd) - return; - for (i = 0; i < cmd->num_args; i++) - VIR_FREE(cmd->args[i]); - for (i = 0; i < cmd->num_env; i++) { - VIR_FREE(cmd->env_name[i]); - VIR_FREE(cmd->env_value[i]); - } - VIR_FREE(cmd->args); - VIR_FREE(cmd->env_name); - VIR_FREE(cmd->env_value); - VIR_FREE(cmd); + qemuDomainCmdlineDefFree(cmd); } static int -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If an OOM occurs in xenParseXM, a virDomainChrDef may be leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/xenxs/xen_xm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 1ffea84..4386fef 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1080,8 +1080,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (!(chr = xenParseSxprChar(port, NULL))) goto cleanup; - if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) + if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { + virDomainChrDefFree(chr); goto cleanup; + } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; chr->target.port = portnum; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> The xenParseXM sets def->nconsoles to 1 before claling VIR_REALLOC_N on def->consoles. So if the alloc fails due to OOM, the cleanup code will crash accessing a console that does not exist. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/xenxs/xen_xm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 4386fef..1652fff 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1112,9 +1112,9 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, } } } else { - def->nconsoles = 1; if (VIR_ALLOC_N(def->consoles, 1) < 0) goto cleanup; + def->nconsoles = 1; if (!(def->consoles[0] = xenParseSxprChar("pty", NULL))) goto cleanup; def->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If an OOM occurs when xenFormatXM is setting the 'hpet' variable it is silently ignored. Fix it to propagate to the callers. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/xenxs/xen_xm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 1652fff..7550a07 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1601,7 +1601,7 @@ virConfPtr xenFormatXM(virConnectPtr conn, if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && def->clock.timers[i]->present != -1 && xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) - break; + goto cleanup; } if (xendConfigVersion == XEND_CONFIG_VERSION_3_0_2) { -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> If an OOM occurs in xenFormatXM when formatting to the serial device value, the value is leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/xenxs/xen_xm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 7550a07..9e07f95 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1959,8 +1959,10 @@ virConfPtr xenFormatXM(virConnectPtr conn, break; } } - if (xenFormatXMSerial(serialVal, chr) < 0) + if (xenFormatXMSerial(serialVal, chr) < 0) { + virConfFreeValue(serialVal); goto cleanup; + } } if (serialVal->list != NULL) { -- 1.8.3.1