[libvirt] [PATCH] Add missing OOM error checks, reports and cleanups

--- daemon/remote.c | 10 +-- src/conf/domain_conf.c | 4 +- src/conf/node_device_conf.c | 13 +++- src/esx/esx_driver.c | 5 ++ src/lxc/lxc_driver.c | 27 +++++-- src/openvz/openvz_conf.c | 12 ++- src/openvz/openvz_driver.c | 17 +++-- src/phyp/phyp_driver.c | 7 ++- src/qemu/qemu_conf.c | 64 +++++++++++++---- src/qemu/qemu_driver.c | 12 ++-- src/qemu/qemu_monitor_text.c | 4 +- src/remote/remote_driver.c | 135 +++++++++++++++++++++++++++++++---- src/secret/secret_driver.c | 8 ++- src/security/security_apparmor.c | 5 +- src/storage/storage_backend_scsi.c | 15 ++++ src/util/conf.c | 14 ++++ src/vbox/vbox_tmpl.c | 25 +++++-- src/xen/xen_driver.c | 12 +++- src/xen/xen_hypervisor.c | 15 ++++- src/xen/xen_inotify.c | 13 ++-- src/xen/xend_internal.c | 45 ++++++++++--- src/xen/xm_internal.c | 47 ++++++++++--- src/xen/xs_internal.c | 64 ++++++++++++----- 23 files changed, 444 insertions(+), 129 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 4296fc3..31d9841 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -565,7 +565,7 @@ remoteDispatchDomainSetSchedulerParameters (struct qemud_server *server ATTRIBUT remoteDispatchFormatError (rerr, "%s", _("nparams too large")); return -1; } - if (VIR_ALLOC_N(params, nparams)) { + if (VIR_ALLOC_N(params, nparams) < 0) { remoteDispatchOOMError(rerr); return -1; } @@ -721,10 +721,8 @@ remoteDispatchDomainBlockPeek (struct qemud_server *server ATTRIBUTE_UNUSED, ret->buffer.buffer_len = size; if (VIR_ALLOC_N (ret->buffer.buffer_val, size) < 0) { - char ebuf[1024]; virDomainFree (dom); - remoteDispatchFormatError (rerr, "%s", - virStrerror(errno, ebuf, sizeof ebuf)); + remoteDispatchOOMError(rerr); return -1; } @@ -772,10 +770,8 @@ remoteDispatchDomainMemoryPeek (struct qemud_server *server ATTRIBUTE_UNUSED, ret->buffer.buffer_len = size; if (VIR_ALLOC_N (ret->buffer.buffer_val, size) < 0) { - char ebuf[1024]; virDomainFree (dom); - remoteDispatchFormatError (rerr, "%s", - virStrerror(errno, ebuf, sizeof ebuf)); + remoteDispatchOOMError(rerr); return -1; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 918a5d7..28bee54 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3431,8 +3431,10 @@ static virDomainObjPtr virDomainObjParseXML(virConnectPtr conn, goto error; if (n) { obj->nvcpupids = n; - if (VIR_REALLOC_N(obj->vcpupids, obj->nvcpupids) < 0) + if (VIR_REALLOC_N(obj->vcpupids, obj->nvcpupids) < 0) { + virReportOOMError(conn); goto error; + } for (i = 0 ; i < n ; i++) { char *pidstr = virXMLPropString(nodes[i], "pid"); diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c2c5a44..c5083cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create /* Extract device name */ if (create == EXISTING_DEVICE) { def->name = virXPathString(conn, "string(./name[1])", ctxt); + + if (!def->name) { + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); + goto error; + } } else { def->name = strdup("new device"); - } - if (!def->name) { - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); - goto error; + if (!def->name) { + virReportOOMError(conn); + goto error; + } } /* Extract device parent, if any */ diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 93fb5a9..f81f744 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2262,6 +2262,7 @@ esxListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) esxVI_DynamicProperty *dynamicProperty = NULL; esxVI_VirtualMachinePowerState powerState; int count = 0; + int i; if (names == NULL || maxnames < 0) { goto failure; @@ -2329,6 +2330,10 @@ esxListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) return count; failure: + for (i = 0; i < count; ++i) { + VIR_FREE(names[i]); + } + count = -1; goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0fd7a17..f44901c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -445,6 +445,9 @@ static char *lxcGetOSType(virDomainPtr dom) ret = strdup(vm->def->os.type); + if (ret == NULL) + virReportOOMError(dom->conn); + cleanup: if (vm) virDomainObjUnlock(vm); @@ -724,14 +727,18 @@ static int lxcSetupInterfaces(virConnectPtr conn, if (NULL == def->nets[i]->ifname) { def->nets[i]->ifname = strdup(parentVeth); } - if (VIR_REALLOC_N(*veths, (*nveths)+1) < 0) + if (VIR_REALLOC_N(*veths, (*nveths)+1) < 0) { + virReportOOMError(conn); goto error_exit; - if (((*veths)[(*nveths)++] = strdup(containerVeth)) == NULL) + } + if (((*veths)[(*nveths)] = strdup(containerVeth)) == NULL) { + virReportOOMError(conn); goto error_exit; + } + (*nveths)++; if (NULL == def->nets[i]->ifname) { - lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to allocate veth names")); + virReportOOMError(conn); goto error_exit; } @@ -1771,13 +1778,19 @@ static int lxcVersion(virConnectPtr conn, unsigned long *version) return 0; } -static char *lxcGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, - int *nparams) +static char *lxcGetSchedulerType(virDomainPtr domain, int *nparams) { + char *schedulerType = NULL; + if (nparams) *nparams = 1; - return strdup("posix"); + schedulerType = strdup("posix"); + + if (schedulerType == NULL) + virReportOOMError(domain->conn); + + return schedulerType; } static int lxcSetSchedulerParameters(virDomainPtr domain, diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index c928afb..6eeece8 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -386,8 +386,8 @@ openvzReadFSConf(virConnectPtr conn, if (VIR_ALLOC(fs) < 0) goto no_memory; - if(virAsprintf(&veid_str, "%d", veid) < 0) - goto error; + if (virAsprintf(&veid_str, "%d", veid) < 0) + goto no_memory; fs->type = VIR_DOMAIN_FS_TYPE_MOUNT; fs->src = openvz_replace(temp, "$VEID", veid_str); @@ -547,8 +547,10 @@ openvzWriteConfigParam(const char * conf_file, const char *param, const char *va int fd = -1, temp_fd = -1; char line[PATH_MAX] ; - if (virAsprintf(&temp_file, "%s.tmp", conf_file)<0) + if (virAsprintf(&temp_file, "%s.tmp", conf_file)<0) { + virReportOOMError(NULL); return -1; + } fd = open(conf_file, O_RDONLY); if (fd == -1) @@ -733,8 +735,10 @@ openvzCopyDefaultConfig(int vpsid) if (confdir == NULL) goto cleanup; - if (virAsprintf(&default_conf_file, "%s/ve-%s.conf-sample", confdir, configfile_value) < 0) + if (virAsprintf(&default_conf_file, "%s/ve-%s.conf-sample", confdir, configfile_value) < 0) { + virReportOOMError(NULL); goto cleanup; + } if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 53fcaee..467c03b 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -767,9 +767,11 @@ openvzDomainDefineXML(virConnectPtr conn, const char *xml) VIR_DOMAIN_XML_INACTIVE)) == NULL) goto cleanup; - if (vmdef->os.init == NULL && - !(vmdef->os.init = strdup("/sbin/init"))) { - goto cleanup; + if (vmdef->os.init == NULL) { + if (!(vmdef->os.init = strdup("/sbin/init"))) { + virReportOOMError(conn); + goto cleanup; + } } vm = virDomainFindByName(&driver->domains, vmdef->name); @@ -844,9 +846,12 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, VIR_DOMAIN_XML_INACTIVE)) == NULL) goto cleanup; - if (vmdef->os.init == NULL && - !(vmdef->os.init = strdup("/sbin/init"))) - goto cleanup; + if (vmdef->os.init == NULL) { + if (!(vmdef->os.init = strdup("/sbin/init"))) { + virReportOOMError(conn); + goto cleanup; + } + } vm = virDomainFindByName(&driver->domains, vmdef->name); if (vm) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index a92046a..6aee504 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1732,7 +1732,8 @@ phypUUIDTable_ReadFile(virConnectPtr conn) goto err; } } - } + } else + virReportOOMError(conn); close(fd); return 0; @@ -1833,8 +1834,10 @@ phypUUIDTable_Init(virConnectPtr conn) VIR_WARN("%s %d", "Unable to generate UUID for domain", ids[i]); } - } else + } else { + virReportOOMError(conn); goto err; + } if (phypUUIDTable_WriteFile(conn) == -1) goto err; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1e24c3..28567b2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -417,17 +417,17 @@ qemudParseMachineTypesStr(const char *output, continue; if (VIR_ALLOC(machine) < 0) - goto error; + goto no_memory; if (!(machine->name = strndup(p, t - p))) { VIR_FREE(machine); - goto error; + goto no_memory; } if (VIR_REALLOC_N(list, nitems + 1) < 0) { VIR_FREE(machine->name); VIR_FREE(machine); - goto error; + goto no_memory; } p = t; @@ -446,7 +446,7 @@ qemudParseMachineTypesStr(const char *output, continue; if (!(machine->canonical = strndup(p, t - p))) - goto error; + goto no_memory; } } while ((p = next)); @@ -455,7 +455,8 @@ qemudParseMachineTypesStr(const char *output, return 0; -error: + no_memory: + virReportOOMError(NULL); virCapabilitiesFreeMachines(list, nitems); return -1; } @@ -537,23 +538,22 @@ qemudGetOldMachinesFromInfo(virCapsGuestDomainInfoPtr info, return 0; } - if (VIR_ALLOC_N(list, info->nmachines) < 0) + if (VIR_ALLOC_N(list, info->nmachines) < 0) { + virReportOOMError(NULL); return 0; + } for (i = 0; i < info->nmachines; i++) { if (VIR_ALLOC(list[i]) < 0) { - virCapabilitiesFreeMachines(list, info->nmachines); - return 0; + goto no_memory; } if (info->machines[i]->name && !(list[i]->name = strdup(info->machines[i]->name))) { - virCapabilitiesFreeMachines(list, info->nmachines); - return 0; + goto no_memory; } if (info->machines[i]->canonical && !(list[i]->canonical = strdup(info->machines[i]->canonical))) { - virCapabilitiesFreeMachines(list, info->nmachines); - return 0; + goto no_memory; } } @@ -561,6 +561,11 @@ qemudGetOldMachinesFromInfo(virCapsGuestDomainInfoPtr info, *nmachines = info->nmachines; return 1; + + no_memory: + virReportOOMError(NULL); + virCapabilitiesFreeMachines(list, info->nmachines); + return 0; } static int @@ -668,15 +673,19 @@ qemudCapsInitGuest(virCapsPtr caps, if (info->machine) { virCapsGuestMachinePtr machine; - if (VIR_ALLOC(machine) < 0) + if (VIR_ALLOC(machine) < 0) { + virReportOOMError(NULL); return -1; + } if (!(machine->name = strdup(info->machine))) { + virReportOOMError(NULL); VIR_FREE(machine); return -1; } if (VIR_ALLOC_N(machines, nmachines) < 0) { + virReportOOMError(NULL); VIR_FREE(machine->name); VIR_FREE(machine); return -1; @@ -1166,7 +1175,10 @@ qemudNetworkIfaceConnect(virConnectPtr conn, if (brname == NULL) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - brname = strdup(net->data.bridge.brname); + if (!(brname = strdup(net->data.bridge.brname))) { + virReportOOMError(conn); + return -1; + } } else { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), net->type); @@ -1276,12 +1288,15 @@ qemuAssignNetNames(virDomainDefPtr def, if (virAsprintf(&nic_name, "%s.%d", net->model ? net->model : "nic", - nic_index) < 0) + nic_index) < 0) { + virReportOOMError(NULL); return -1; + } if (virAsprintf(&hostnet_name, "%s.%d", qemuNetTypeToHostNet(net->type), hostnet_index) < 0) { + virReportOOMError(NULL); VIR_FREE(nic_name); return -1; } @@ -2434,7 +2449,7 @@ int qemudBuildCommandLine(virConnectPtr conn, hostdev->source.subsys.u.pci.function); if (ret < 0) { pcidev = NULL; - goto error; + goto no_memory; } ADD_ARG_LIT("-pcidevice"); ADD_ARG_LIT(pcidev); @@ -2589,6 +2604,7 @@ no_memory: for (i = 0 ; i < argcount ; i++) VIR_FREE(arglist[i]); VIR_FREE(arglist); + virReportOOMError(NULL); return -1; } @@ -3177,12 +3193,19 @@ qemuParseCommandLineChr(virConnectPtr conn, def->data.udp.connectHost = strndup(val, svc1-val); else def->data.udp.connectHost = strdup(val); + + if (!def->data.udp.connectHost) + goto no_memory; + if (svc1) { svc1++; if (host2) def->data.udp.connectService = strndup(svc1, host2-svc1); else def->data.udp.connectService = strdup(svc1); + + if (!def->data.udp.connectService) + goto no_memory; } if (host2) { @@ -3191,10 +3214,15 @@ qemuParseCommandLineChr(virConnectPtr conn, def->data.udp.bindHost = strndup(host2, svc2-host2); else def->data.udp.bindHost = strdup(host2); + + if (!def->data.udp.bindHost) + goto no_memory; } if (svc2) { svc2++; def->data.udp.bindService = strdup(svc2); + if (!def->data.udp.bindService) + goto no_memory; } } else if (STRPREFIX(val, "tcp:") || STRPREFIX(val, "telnet:")) { @@ -3217,12 +3245,16 @@ qemuParseCommandLineChr(virConnectPtr conn, def->data.tcp.listen = 1; def->data.tcp.host = strndup(val, svc-val); + if (!def->data.tcp.host) + goto no_memory; svc++; if (opt) { def->data.tcp.service = strndup(svc, opt-svc); } else { def->data.tcp.service = strdup(svc); } + if (!def->data.tcp.service) + goto no_memory; } else if (STRPREFIX(val, "unix:")) { const char *opt; val += strlen("unix:"); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5463951..f022f89 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -364,17 +364,13 @@ qemudSecurityCapsInit(virSecurityDriverPtr secdrv, caps->host.secModel.model = strdup(model); if (!caps->host.secModel.model) { - char ebuf[1024]; - VIR_ERROR(_("Failed to copy secModel model: %s"), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportOOMError(NULL); return -1; } caps->host.secModel.doi = strdup(doi); if (!caps->host.secModel.doi) { - char ebuf[1024]; - VIR_ERROR(_("Failed to copy secModel DOI: %s"), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportOOMError(NULL); return -1; } @@ -5851,8 +5847,10 @@ static struct qemuStreamMigFile *qemuStreamMigOpen(virStreamPtr st, int timeout = 3; int ret; - if (VIR_ALLOC(qemust) < 0) + if (VIR_ALLOC(qemust) < 0) { + virReportOOMError(st->conn); return NULL; + } qemust->fd = socket(AF_UNIX, SOCK_STREAM, 0); if (qemust->fd < 0) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 66526dc..35cd330 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -201,8 +201,10 @@ qemuMonitorSend(const virDomainObjPtr vm, size_t len; int ret = -1; - if (virAsprintf(&full, "%s\r", cmd) < 0) + if (virAsprintf(&full, "%s\r", cmd) < 0) { + virReportOOMError(NULL); return -1; + } len = strlen(full); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ee7a046..c866111 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -870,12 +870,12 @@ doRemoteOpen (virConnectPtr conn, } if(VIR_ALLOC(priv->callbackList)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating callbacks list")); + virReportOOMError(conn); goto failed; } if(VIR_ALLOC(priv->domainEvents)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents")); + virReportOOMError(conn); goto failed; } @@ -2751,9 +2751,18 @@ remoteListDefinedDomains (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -3086,7 +3095,7 @@ remoteDomainSetSchedulerParameters (virDomainPtr domain, /* Serialise the scheduler parameters. */ args.params.params_len = nparams; if (VIR_ALLOC_N(args.params.params_val, nparams) < 0) { - error (domain->conn, VIR_ERR_RPC, _("out of memory allocating array")); + virReportOOMError(domain->conn); goto done; } @@ -3432,9 +3441,18 @@ remoteListNetworks (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -3505,9 +3523,18 @@ remoteListDefinedNetworks (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -3921,9 +3948,18 @@ remoteListInterfaces (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -3993,9 +4029,18 @@ remoteListDefinedInterfaces (virConnectPtr conn, char **const names, int maxname * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -4314,9 +4359,18 @@ remoteListStoragePools (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -4383,9 +4437,18 @@ remoteListDefinedStoragePools (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -4890,9 +4953,18 @@ remoteStoragePoolListVolumes (virStoragePoolPtr pool, char **const names, int ma * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(pool->conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -5290,9 +5362,18 @@ static int remoteNodeListDevices(virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -5443,9 +5524,18 @@ static int remoteNodeDeviceListCaps(virNodeDevicePtr dev, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]); + if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(dev->conn); + goto cleanup; + } + } + rv = ret.names.names_len; cleanup: @@ -6496,9 +6586,18 @@ remoteSecretListSecrets (virConnectPtr conn, char **uuids, int maxuuids) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.uuids.uuids_len; ++i) + for (i = 0; i < ret.uuids.uuids_len; ++i) { uuids[i] = strdup (ret.uuids.uuids_val[i]); + if (uuids[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(uuids[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.uuids.uuids_len; cleanup: @@ -6707,8 +6806,10 @@ remoteStreamOpen(virStreamPtr st, struct private_data *priv = st->conn->privateData; struct private_stream_data *stpriv; - if (VIR_ALLOC(stpriv) < 0) + if (VIR_ALLOC(stpriv) < 0) { + virReportOOMError(st->conn); return NULL; + } /* Initialize call object used to receive replies */ stpriv->proc_nr = proc_nr; @@ -7106,8 +7207,10 @@ prepareCall(virConnectPtr conn, struct remote_message_header hdr; struct remote_thread_call *rv; - if (VIR_ALLOC(rv) < 0) + if (VIR_ALLOC(rv) < 0) { + virReportOOMError(conn); return NULL; + } if (virCondInit(&rv->cond) < 0) { VIR_FREE(rv); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 1d5b4f7..1eef468 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -449,8 +449,10 @@ secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver, if (secretLoadValidateUUID(conn, def, xml_basename) < 0) goto cleanup; - if (VIR_ALLOC(secret) < 0) + if (VIR_ALLOC(secret) < 0) { + virReportOOMError(conn); goto cleanup; + } secret->def = def; def = NULL; @@ -578,8 +580,10 @@ secretListSecrets(virConnectPtr conn, char **uuids, int maxuuids) char *uuidstr; if (i == maxuuids) break; - if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) { + virReportOOMError(conn); goto cleanup; + } virUUIDFormat(secret->def->uuid, uuidstr); uuids[i] = uuidstr; i++; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 6db51cd..6c2dce5 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -56,13 +56,16 @@ profile_status(const char *str, const int check_enforcing) int rc = -1; /* create string that is '<str> \0' for accurate matching */ - if (virAsprintf(&tmp, "%s ", str) == -1) + if (virAsprintf(&tmp, "%s ", str) == -1) { + virReportOOMError(NULL); return rc; + } if (check_enforcing != 0) { /* create string that is '<str> (enforce)\0' for accurate matching */ if (virAsprintf(&etmp, "%s (enforce)", str) == -1) { VIR_FREE(tmp); + virReportOOMError(NULL); return rc; } } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c70b1ed..4b181d7 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -326,6 +326,14 @@ getNewStyleBlockDevice(virConnectPtr conn, } *block_device = strdup(block_dirent->d_name); + + if (*block_device == NULL) { + virReportOOMError(conn); + closedir(block_dir); + retval = -1; + goto out; + } + VIR_DEBUG(_("Block device is '%s'"), *block_device); break; @@ -360,9 +368,16 @@ getOldStyleBlockDevice(virConnectPtr conn, blockp++; *block_device = strdup(blockp); + if (*block_device == NULL) { + virReportOOMError(conn); + retval = -1; + goto out; + } + VIR_DEBUG(_("Block device is '%s'"), *block_device); } +out: return retval; } diff --git a/src/util/conf.c b/src/util/conf.c index 2aa4a55..8126f69 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -388,6 +388,10 @@ virConfParseString(virConfParserCtxtPtr ctxt) return(NULL); } ret = strndup(base, ctxt->cur - base); + if (ret == NULL) { + virReportOOMError(NULL); + return NULL; + } NEXT; } else if ((ctxt->cur + 6 < ctxt->end) && (ctxt->cur[0] == '"') && (ctxt->cur[1] == '"') && (ctxt->cur[2] == '"')) { @@ -404,6 +408,10 @@ virConfParseString(virConfParserCtxtPtr ctxt) return(NULL); } ret = strndup(base, ctxt->cur - base); + if (ret == NULL) { + virReportOOMError(NULL); + return NULL; + } ctxt->cur += 3; } else if (CUR == '"') { NEXT; @@ -415,6 +423,10 @@ virConfParseString(virConfParserCtxtPtr ctxt) return(NULL); } ret = strndup(base, ctxt->cur - base); + if (ret == NULL) { + virReportOOMError(NULL); + return NULL; + } NEXT; } return(ret); @@ -857,11 +869,13 @@ virConfSetValue (virConfPtr conf, if (!cur) { if (VIR_ALLOC(cur) < 0) { + virReportOOMError(NULL); virConfFreeValue(value); return (-1); } cur->comment = NULL; if (!(cur->name = strdup(setting))) { + virReportOOMError(NULL); virConfFreeValue(value); VIR_FREE(cur); return (-1); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index d29e424..0957809 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1333,13 +1333,18 @@ cleanup: return ret; } -static char *vboxDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED) { +static char *vboxDomainGetOSType(virDomainPtr dom) { /* Returning "hvm" always as suggested on list, cause * this functions seems to be badly named and it * is supposed to pass the ABI name and not the domain * operating system driver as I had imagined ;) */ - return strdup("hvm"); + char *osType = strdup("hvm"); + + if (osType == NULL) + virReportOOMError(dom->conn); + + return osType; } static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) { @@ -1881,7 +1886,7 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { if (valueDisplayUtf8) sdlDisplay = strdup(valueDisplayUtf8); if (sdlDisplay == NULL) { - vboxError(dom->conn, VIR_ERR_SYSTEM_ERROR, "%s", "strdup failed"); + virReportOOMError(dom->conn); /* just don't go to cleanup yet as it is ok to have * sdlDisplay as NULL and we check it below if it * exist and then only use it there @@ -1895,7 +1900,7 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { if (valueDisplayUtf8) guiDisplay = strdup(valueDisplayUtf8); if (guiDisplay == NULL) { - vboxError(dom->conn, VIR_ERR_SYSTEM_ERROR, "%s", "strdup failed"); + virReportOOMError(dom->conn); /* just don't go to cleanup yet as it is ok to have * guiDisplay as NULL and we check it below if it * exist and then only use it there @@ -1932,7 +1937,7 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; def->graphics[def->ngraphics]->data.desktop.display = strdup(getenv("DISPLAY")); if (def->graphics[def->ngraphics]->data.desktop.display == NULL) { - vboxError(dom->conn, VIR_ERR_SYSTEM_ERROR, "%s", "strdup failed"); + virReportOOMError(dom->conn); /* just don't go to cleanup yet as it is ok to have * display as NULL */ @@ -3983,6 +3988,11 @@ static int vboxDomainAttachDevice(virDomainPtr dom, const char *xml) { def->os.type = strdup("hvm"); + if (def->os.type == NULL) { + virReportOOMError(dom->conn); + goto cleanup; + } + dev = virDomainDeviceDefParse(dom->conn, data->caps, def, xml, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) { @@ -4176,6 +4186,11 @@ static int vboxDomainDetachDevice(virDomainPtr dom, const char *xml) { def->os.type = strdup("hvm"); + if (def->os.type == NULL) { + virReportOOMError(dom->conn); + goto cleanup; + } + dev = virDomainDeviceDefParse(dom->conn, data->caps, def, xml, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 479db10..a83b764 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -138,14 +138,20 @@ xenDomainUsedCpus(virDomainPtr dom) if (xenUnifiedNodeGetInfo(dom->conn, &nodeinfo) < 0) return(NULL); - if (VIR_ALLOC_N(cpulist, priv->nbNodeCpus) < 0) + if (VIR_ALLOC_N(cpulist, priv->nbNodeCpus) < 0) { + virReportOOMError(dom->conn); goto done; - if (VIR_ALLOC_N(cpuinfo, nb_vcpu) < 0) + } + if (VIR_ALLOC_N(cpuinfo, nb_vcpu) < 0) { + virReportOOMError(dom->conn); goto done; + } cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo)); if (xalloc_oversized(nb_vcpu, cpumaplen) || - VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) < 0) + VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) < 0) { + virReportOOMError(dom->conn); goto done; + } if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu, cpumap, cpumaplen)) >= 0) { diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index e107d1e..f25629a 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1114,11 +1114,15 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) switch (op.u.getschedulerid.sched_id){ case XEN_SCHEDULER_SEDF: schedulertype = strdup("sedf"); + if (schedulertype == NULL) + virReportOOMError(domain->conn); if (nparams) *nparams = 6; break; case XEN_SCHEDULER_CREDIT: schedulertype = strdup("credit"); + if (schedulertype == NULL) + virReportOOMError(domain->conn); if (nparams) *nparams = 2; break; @@ -2755,6 +2759,7 @@ xenHypervisorDomainGetOSType (virDomainPtr dom) { xenUnifiedPrivatePtr priv; xen_getdomaininfo dominfo; + char *ostype = NULL; priv = (xenUnifiedPrivatePtr) dom->conn->privateData; if (priv->handle < 0) @@ -2774,8 +2779,14 @@ xenHypervisorDomainGetOSType (virDomainPtr dom) return (NULL); if (XEN_GETDOMAININFO_FLAGS(dominfo) & DOMFLAGS_HVM) - return strdup("hvm"); - return strdup("linux"); + ostype = strdup("hvm"); + else + ostype = strdup("linux"); + + if (ostype == NULL) + virReportOOMError(dom->conn); + + return ostype; } virDomainPtr diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index aa3893a..a41711d 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -107,6 +107,7 @@ xenInotifyXenCacheLookup(virConnectPtr conn, if (!*name) { DEBUG0("Error getting dom from def"); + virReportOOMError(conn); return -1; } return 0; @@ -145,8 +146,7 @@ xenInotifyXendDomainsDirLookup(virConnectPtr conn, const char *filename, if (!memcmp(rawuuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) { *name = strdup(priv->configInfoList->doms[i]->name); if (!*name) { - virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, - _("finding dom for %s"), uuid_str); + virReportOOMError(conn); return -1; } memcpy(uuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN); @@ -159,8 +159,10 @@ xenInotifyXendDomainsDirLookup(virConnectPtr conn, const char *filename, return -1; } - if (!(*name = strdup(dom->name))) + if (!(*name = strdup(dom->name))) { + virReportOOMError(conn); return -1; + } memcpy(uuid, dom->uuid, VIR_UUID_BUFLEN); virDomainFree(dom); /* succeeded too find domain by uuid */ @@ -380,7 +382,7 @@ cleanup: * Returns 0 or -1 in case of error. */ virDrvOpenStatus -xenInotifyOpen(virConnectPtr conn ATTRIBUTE_UNUSED, +xenInotifyOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { @@ -397,8 +399,7 @@ xenInotifyOpen(virConnectPtr conn ATTRIBUTE_UNUSED, priv->useXenConfigCache = 0; if (VIR_ALLOC(priv->configInfoList) < 0) { - virXenInotifyError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to allocate configInfoList")); + virReportOOMError(conn); return -1; } diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index f86e022..3c660be 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1055,8 +1055,13 @@ xenDaemonDomainLookupByID(virConnectPtr xend, "%s", _("domain information incomplete, missing name")); goto error; } - if (domname) + if (domname) { *domname = strdup(name); + if (*domname == NULL) { + virReportOOMError(xend); + goto error; + } + } if (sexpr_uuid(uuid, root, "domain/uuid") < 0) { virXendError(xend, VIR_ERR_INTERNAL_ERROR, @@ -2946,8 +2951,10 @@ xenDaemonOpen(virConnectPtr conn, goto failed; } else if (STRCASEEQ (conn->uri->scheme, "http")) { if (conn->uri->port && - virAsprintf(&port, "%d", conn->uri->port) == -1) + virAsprintf(&port, "%d", conn->uri->port) == -1) { + virReportOOMError(conn); goto failed; + } if (xenDaemonOpen_tcp(conn, conn->uri->server ? conn->uri->server : "localhost", @@ -3164,6 +3171,9 @@ xenDaemonDomainGetOSType(virDomainPtr domain) type = strdup("linux"); } + if (type == NULL) + virReportOOMError(domain->conn); + sexpr_free(root); return(type); @@ -3959,6 +3969,10 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (id >= 0) { if (!memcmp(uuid, ident, VIR_UUID_BUFLEN)) { name = strdup(*tmp); + + if (name == NULL) + virReportOOMError(conn); + break; } } @@ -3979,7 +3993,14 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) id = sexpr_int(root, "domain/domid"); else id = -1; - name = domname ? strdup(domname) : NULL; + + if (domname) { + name = strdup(domname); + + if (name == NULL) + virReportOOMError(conn); + } + sexpr_free(root); } @@ -4308,8 +4329,7 @@ xenDaemonDomainSetAutostart(virDomainPtr domain, autonode->u.s.car->u.value = (autostart ? strdup("start") : strdup("ignore")); if (!(autonode->u.s.car->u.value)) { - virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("no memory")); + virReportOOMError(domain->conn); goto error; } @@ -4628,7 +4648,7 @@ error: static int xenDaemonListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) { struct sexpr *root = NULL; - int ret = -1; + int i, ret = -1; struct sexpr *_for_i, *node; xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData; @@ -4651,12 +4671,19 @@ xenDaemonListDefinedDomains(virConnectPtr conn, char **const names, int maxnames if (node->kind != SEXPR_VALUE) continue; - names[ret++] = strdup(node->u.value); + if ((names[ret++] = strdup(node->u.value)) == NULL) { + virReportOOMError(conn); + goto error; + } + if (ret >= maxnames) break; } error: + for (i = 0; i < ret; ++i) + VIR_FREE(names[i]); + sexpr_free(root); return(ret); } @@ -4708,14 +4735,14 @@ xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) if (STREQ (ret, "credit")) { schedulertype = strdup("credit"); if (schedulertype == NULL){ - virXendError(domain->conn, VIR_ERR_SYSTEM_ERROR, "%s", _("strdup failed")); + virReportOOMError(domain->conn); goto error; } *nparams = XEN_SCHED_CRED_NPARAM; } else if (STREQ (ret, "sedf")) { schedulertype = strdup("sedf"); if (schedulertype == NULL){ - virXendError(domain->conn, VIR_ERR_SYSTEM_ERROR, "%s", _("strdup failed")); + virReportOOMError(domain->conn); goto error; } *nparams = XEN_SCHED_SEDF_NPARAM; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 5e8931e..5878ba1 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -689,8 +689,10 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { int i; const char *defaultArch, *defaultMachine; - if (VIR_ALLOC(def) < 0) + if (VIR_ALLOC(def) < 0) { + virReportOOMError(conn); return NULL; + } def->virtType = VIR_DOMAIN_VIRT_XEN; def->id = -1; @@ -1100,7 +1102,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { } if (VIR_ALLOC(net) < 0) - goto cleanup; + goto no_memory; if (mac[0]) { unsigned int rawmac[6]; @@ -1234,7 +1236,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { goto skippci; if (VIR_ALLOC(hostdev) < 0) - goto cleanup; + goto no_memory; hostdev->managed = 0; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; @@ -1922,8 +1924,10 @@ static int xenXMConfigSetInt(virConfPtr conf, const char *setting, long l) { virConfValuePtr value = NULL; - if (VIR_ALLOC(value) < 0) + if (VIR_ALLOC(value) < 0) { + virReportOOMError(NULL); return -1; + } value->type = VIR_CONF_LONG; value->next = NULL; @@ -1937,13 +1941,16 @@ static int xenXMConfigSetString(virConfPtr conf, const char *setting, const char *str) { virConfValuePtr value = NULL; - if (VIR_ALLOC(value) < 0) + if (VIR_ALLOC(value) < 0) { + virReportOOMError(NULL); return -1; + } value->type = VIR_CONF_STRING; value->next = NULL; if (!(value->str = strdup(str))) { VIR_FREE(value); + virReportOOMError(NULL); return -1; } @@ -2133,8 +2140,10 @@ xenXMDomainConfigFormatPCI(virConnectPtr conn, if (!hasPCI) return 0; - if (VIR_ALLOC(pciVal) < 0) + if (VIR_ALLOC(pciVal) < 0) { + virReportOOMError(conn); return -1; + } pciVal->type = VIR_CONF_LIST; pciVal->list = NULL; @@ -2149,8 +2158,10 @@ xenXMDomainConfigFormatPCI(virConnectPtr conn, def->hostdevs[i]->source.subsys.u.pci.domain, def->hostdevs[i]->source.subsys.u.pci.bus, def->hostdevs[i]->source.subsys.u.pci.slot, - def->hostdevs[i]->source.subsys.u.pci.function) < 0) + def->hostdevs[i]->source.subsys.u.pci.function) < 0) { + virReportOOMError(conn); goto error; + } if (VIR_ALLOC(val) < 0) { VIR_FREE(buf); @@ -2748,6 +2759,7 @@ cleanup: struct xenXMListIteratorContext { virConnectPtr conn; + int oom; int max; int count; char ** names; @@ -2757,13 +2769,18 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, struct xenXMListIteratorContext *ctx = data; virDomainPtr dom = NULL; + if (ctx->oom) + return; + if (ctx->count == ctx->max) return; dom = xenDaemonLookupByName(ctx->conn, name); if (!dom) { - ctx->names[ctx->count] = strdup(name); - ctx->count++; + if (!(ctx->names[ctx->count] = strdup(name))) + ctx->oom = 1; + else + ctx->count++; } else { virDomainFree(dom); } @@ -2777,7 +2794,7 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) { xenUnifiedPrivatePtr priv; struct xenXMListIteratorContext ctx; - int ret = -1; + int i, ret = -1; if (!VIR_IS_CONNECT(conn)) { xenXMError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__); @@ -2794,11 +2811,21 @@ int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames maxnames = virHashSize(priv->configCache); ctx.conn = conn; + ctx.oom = 0; ctx.count = 0; ctx.max = maxnames; ctx.names = names; virHashForEach(priv->nameConfigMap, xenXMListIterator, &ctx); + + if (ctx.oom) { + for (i = 0; i < ctx.count; i++) + VIR_FREE(ctx.names[i]); + + virReportOOMError(conn); + goto cleanup; + } + ret = ctx.count; cleanup: diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 0fabcf8..f8ef00b 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -305,17 +305,15 @@ xenStoreOpen(virConnectPtr conn, #ifndef PROXY /* Init activeDomainList */ if (VIR_ALLOC(priv->activeDomainList) < 0) { - virXenStoreError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to allocate activeDomainList")); + virReportOOMError(conn); return -1; } /* Init watch list before filling in domInfoList, so we can know if it is the first time through when the callback fires */ - if ( VIR_ALLOC(priv->xsWatchList) < 0 ) { - virXenStoreError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to allocate xsWatchList")); + if (VIR_ALLOC(priv->xsWatchList) < 0) { + virReportOOMError(conn); return -1; } @@ -892,7 +890,8 @@ xenStoreDomainGetOSTypeID(virConnectPtr conn, int id) { } if (str == NULL) str = strdup("linux"); - + if (str == NULL) + virReportOOMError(conn); return (str); } @@ -944,6 +943,10 @@ xenStoreDomainGetNetworkID(virConnectPtr conn, int id, const char *mac) { if (match) { ret = strdup(list[i]); + + if (ret == NULL) + virReportOOMError(conn); + break; } } @@ -995,15 +998,19 @@ xenStoreDomainGetDiskID(virConnectPtr conn, int id, const char *dev) { if (val == NULL) break; if ((devlen != len) || memcmp(val, dev, len)) { - free (val); + VIR_FREE (val); } else { ret = strdup(list[i]); - free (val); - free (list); + + if (ret == NULL) + virReportOOMError(conn); + + VIR_FREE (val); + VIR_FREE (list); return (ret); } } - free (list); + VIR_FREE (list); } snprintf(dir, sizeof(dir), "/local/domain/0/backend/tap/%d", id); list = xs_directory(priv->xshandle, 0, dir, &num); @@ -1014,15 +1021,19 @@ xenStoreDomainGetDiskID(virConnectPtr conn, int id, const char *dev) { if (val == NULL) break; if ((devlen != len) || memcmp(val, dev, len)) { - free (val); + VIR_FREE (val); } else { ret = strdup(list[i]); - free (val); - free (list); + + if (ret == NULL) + virReportOOMError(conn); + + VIR_FREE (val); + VIR_FREE (list); return (ret); } } - free (list); + VIR_FREE (list); } return (NULL); } @@ -1100,7 +1111,7 @@ int xenStoreAddWatch(virConnectPtr conn, xenStoreWatchCallback cb, void *opaque) { - xenStoreWatchPtr watch; + xenStoreWatchPtr watch = NULL; int n; xenStoreWatchListPtr list; xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData; @@ -1123,25 +1134,38 @@ int xenStoreAddWatch(virConnectPtr conn, } if (VIR_ALLOC(watch) < 0) - return -1; + goto no_memory; + watch->path = strdup(path); watch->token = strdup(token); watch->cb = cb; watch->opaque = opaque; + if (watch->path == NULL || watch->token == NULL) { + goto no_memory; + } + /* Make space on list */ n = list->count; if (VIR_REALLOC_N(list->watches, n + 1) < 0) { - virXenStoreError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("reallocating list")); - VIR_FREE(watch); - return -1; + goto no_memory; } list->watches[n] = watch; list->count++; return xs_watch(priv->xshandle, watch->path, watch->token); + + no_memory: + if (watch) { + VIR_FREE(watch->path); + VIR_FREE(watch->token); + VIR_FREE(watch); + } + + virReportOOMError(conn); + + return -1; } /* -- 1.6.0.4

On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote:
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c2c5a44..c5083cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create /* Extract device name */ if (create == EXISTING_DEVICE) { def->name = virXPathString(conn, "string(./name[1])", ctxt); + + if (!def->name) { + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); + goto error; + } } else { def->name = strdup("new device"); - }
- if (!def->name) { - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); - goto error; + if (!def->name) { + virReportOOMError(conn); + goto error; + } }
/* Extract device parent, if any */
I disagree with this one. The XPath string(./name[1]) can fail without this being an allocation error, it mays just be that there is no name element at the current node, and current behaviour looks better to me. Moreover if virXPathString() returns NULL because of a string allocation error it will already raise an OOMError [...]
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ee7a046..c866111 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -870,12 +870,12 @@ doRemoteOpen (virConnectPtr conn, }
if(VIR_ALLOC(priv->callbackList)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating callbacks list")); + virReportOOMError(conn); goto failed; }
if(VIR_ALLOC(priv->domainEvents)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents")); + virReportOOMError(conn); goto failed; }
@@ -2751,9 +2751,18 @@ remoteListDefinedDomains (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3086,7 +3095,7 @@ remoteDomainSetSchedulerParameters (virDomainPtr domain, /* Serialise the scheduler parameters. */ args.params.params_len = nparams; if (VIR_ALLOC_N(args.params.params_val, nparams) < 0) { - error (domain->conn, VIR_ERR_RPC, _("out of memory allocating array")); + virReportOOMError(domain->conn); goto done; }
@@ -3432,9 +3441,18 @@ remoteListNetworks (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3505,9 +3523,18 @@ remoteListDefinedNetworks (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3921,9 +3948,18 @@ remoteListInterfaces (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3993,9 +4029,18 @@ remoteListDefinedInterfaces (virConnectPtr conn, char **const names, int maxname * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4314,9 +4359,18 @@ remoteListStoragePools (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4383,9 +4437,18 @@ remoteListDefinedStoragePools (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4890,9 +4953,18 @@ remoteStoragePoolListVolumes (virStoragePoolPtr pool, char **const names, int ma * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(pool->conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -5290,9 +5362,18 @@ static int remoteNodeListDevices(virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -5443,9 +5524,18 @@ static int remoteNodeDeviceListCaps(virNodeDevicePtr dev, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(dev->conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -6496,9 +6586,18 @@ remoteSecretListSecrets (virConnectPtr conn, char **uuids, int maxuuids) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.uuids.uuids_len; ++i) + for (i = 0; i < ret.uuids.uuids_len; ++i) { uuids[i] = strdup (ret.uuids.uuids_val[i]);
+ if (uuids[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(uuids[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.uuids.uuids_len;
cleanup: @@ -6707,8 +6806,10 @@ remoteStreamOpen(virStreamPtr st, struct private_data *priv = st->conn->privateData; struct private_stream_data *stpriv;
- if (VIR_ALLOC(stpriv) < 0) + if (VIR_ALLOC(stpriv) < 0) { + virReportOOMError(st->conn); return NULL; + }
/* Initialize call object used to receive replies */ stpriv->proc_nr = proc_nr; @@ -7106,8 +7207,10 @@ prepareCall(virConnectPtr conn, struct remote_message_header hdr; struct remote_thread_call *rv;
- if (VIR_ALLOC(rv) < 0) + if (VIR_ALLOC(rv) < 0) { + virReportOOMError(conn); return NULL; + }
if (virCondInit(&rv->cond) < 0) { VIR_FREE(rv); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 1d5b4f7..1eef468 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -449,8 +449,10 @@ secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver, if (secretLoadValidateUUID(conn, def, xml_basename) < 0) goto cleanup;
- if (VIR_ALLOC(secret) < 0) + if (VIR_ALLOC(secret) < 0) { + virReportOOMError(conn); goto cleanup; + } secret->def = def; def = NULL;
@@ -578,8 +580,10 @@ secretListSecrets(virConnectPtr conn, char **uuids, int maxuuids) char *uuidstr; if (i == maxuuids) break; - if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) { + virReportOOMError(conn); goto cleanup; + } virUUIDFormat(secret->def->uuid, uuidstr); uuids[i] = uuidstr; i++;
Okay, I was just a bit worried about virReportOOMError() in the context of a remote driver entry point but apparently there are previous uses in that context, so nice :-) [...]
@@ -2748,6 +2759,7 @@ cleanup:
struct xenXMListIteratorContext { virConnectPtr conn; + int oom; int max; int count; char ** names; @@ -2757,13 +2769,18 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, struct xenXMListIteratorContext *ctx = data; virDomainPtr dom = NULL;
+ if (ctx->oom) + return; + if (ctx->count == ctx->max) return;
dom = xenDaemonLookupByName(ctx->conn, name); if (!dom) { - ctx->names[ctx->count] = strdup(name); - ctx->count++; + if (!(ctx->names[ctx->count] = strdup(name))) + ctx->oom = 1; + else + ctx->count++; } else { virDomainFree(dom); } @@ -2777,7 +2794,7 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) { xenUnifiedPrivatePtr priv; struct xenXMListIteratorContext ctx; - int ret = -1; + int i, ret = -1;
if (!VIR_IS_CONNECT(conn)) { xenXMError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__); @@ -2794,11 +2811,21 @@ int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames maxnames = virHashSize(priv->configCache);
ctx.conn = conn; + ctx.oom = 0; ctx.count = 0; ctx.max = maxnames; ctx.names = names;
virHashForEach(priv->nameConfigMap, xenXMListIterator, &ctx); + + if (ctx.oom) { + for (i = 0; i < ctx.count; i++) + VIR_FREE(ctx.names[i]); + + virReportOOMError(conn); + goto cleanup; + } + ret = ctx.count;
cleanup:
So you had to add a filed in the iterator structure to report OOMs while running the iterator, nice work ! ACK except for the one in virNodeDeviceDefParseXML(), very good job you must have spent a lot of time, thanks a lot ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

2009/11/9 Daniel Veillard <veillard@redhat.com>:
On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote:
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c2c5a44..c5083cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create /* Extract device name */ if (create == EXISTING_DEVICE) { def->name = virXPathString(conn, "string(./name[1])", ctxt); + + if (!def->name) { + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); + goto error; + } } else { def->name = strdup("new device"); - }
- if (!def->name) { - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); - goto error; + if (!def->name) { + virReportOOMError(conn); + goto error; + } }
/* Extract device parent, if any */
I disagree with this one. The XPath string(./name[1]) can fail without this being an allocation error, it mays just be that there is no name element at the current node, and current behaviour looks better to me. Moreover if virXPathString() returns NULL because of a string allocation error it will already raise an OOMError
I think you misread this one. The original code assigns the result of virXPathString() or strdup() to def->name. After that it checks def->name for NULL and reports an no-name error even if the NULL was returned by strdup(), indicating an OOM error. I moved the no-name error report into the virXPathString() case and added an OOM error in the strdup() case.
[...]
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ee7a046..c866111 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -870,12 +870,12 @@ doRemoteOpen (virConnectPtr conn, }
if(VIR_ALLOC(priv->callbackList)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating callbacks list")); + virReportOOMError(conn); goto failed; }
if(VIR_ALLOC(priv->domainEvents)<0) { - error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents")); + virReportOOMError(conn); goto failed; }
@@ -2751,9 +2751,18 @@ remoteListDefinedDomains (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3086,7 +3095,7 @@ remoteDomainSetSchedulerParameters (virDomainPtr domain, /* Serialise the scheduler parameters. */ args.params.params_len = nparams; if (VIR_ALLOC_N(args.params.params_val, nparams) < 0) { - error (domain->conn, VIR_ERR_RPC, _("out of memory allocating array")); + virReportOOMError(domain->conn); goto done; }
@@ -3432,9 +3441,18 @@ remoteListNetworks (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3505,9 +3523,18 @@ remoteListDefinedNetworks (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3921,9 +3948,18 @@ remoteListInterfaces (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -3993,9 +4029,18 @@ remoteListDefinedInterfaces (virConnectPtr conn, char **const names, int maxname * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4314,9 +4359,18 @@ remoteListStoragePools (virConnectPtr conn, char **const names, int maxnames) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4383,9 +4437,18 @@ remoteListDefinedStoragePools (virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -4890,9 +4953,18 @@ remoteStoragePoolListVolumes (virStoragePoolPtr pool, char **const names, int ma * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(pool->conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -5290,9 +5362,18 @@ static int remoteNodeListDevices(virConnectPtr conn, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -5443,9 +5524,18 @@ static int remoteNodeDeviceListCaps(virNodeDevicePtr dev, * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.names.names_len; ++i) + for (i = 0; i < ret.names.names_len; ++i) { names[i] = strdup (ret.names.names_val[i]);
+ if (names[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(names[i]); + + virReportOOMError(dev->conn); + goto cleanup; + } + } + rv = ret.names.names_len;
cleanup: @@ -6496,9 +6586,18 @@ remoteSecretListSecrets (virConnectPtr conn, char **uuids, int maxuuids) * names and the list of pointers, so we have to strdup the * names here. */ - for (i = 0; i < ret.uuids.uuids_len; ++i) + for (i = 0; i < ret.uuids.uuids_len; ++i) { uuids[i] = strdup (ret.uuids.uuids_val[i]);
+ if (uuids[i] == NULL) { + for (--i; i >= 0; --i) + VIR_FREE(uuids[i]); + + virReportOOMError(conn); + goto cleanup; + } + } + rv = ret.uuids.uuids_len;
cleanup: @@ -6707,8 +6806,10 @@ remoteStreamOpen(virStreamPtr st, struct private_data *priv = st->conn->privateData; struct private_stream_data *stpriv;
- if (VIR_ALLOC(stpriv) < 0) + if (VIR_ALLOC(stpriv) < 0) { + virReportOOMError(st->conn); return NULL; + }
/* Initialize call object used to receive replies */ stpriv->proc_nr = proc_nr; @@ -7106,8 +7207,10 @@ prepareCall(virConnectPtr conn, struct remote_message_header hdr; struct remote_thread_call *rv;
- if (VIR_ALLOC(rv) < 0) + if (VIR_ALLOC(rv) < 0) { + virReportOOMError(conn); return NULL; + }
if (virCondInit(&rv->cond) < 0) { VIR_FREE(rv); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 1d5b4f7..1eef468 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -449,8 +449,10 @@ secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver, if (secretLoadValidateUUID(conn, def, xml_basename) < 0) goto cleanup;
- if (VIR_ALLOC(secret) < 0) + if (VIR_ALLOC(secret) < 0) { + virReportOOMError(conn); goto cleanup; + } secret->def = def; def = NULL;
@@ -578,8 +580,10 @@ secretListSecrets(virConnectPtr conn, char **uuids, int maxuuids) char *uuidstr; if (i == maxuuids) break; - if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) { + virReportOOMError(conn); goto cleanup; + } virUUIDFormat(secret->def->uuid, uuidstr); uuids[i] = uuidstr; i++;
Okay, I was just a bit worried about virReportOOMError() in the context of a remote driver entry point but apparently there are previous uses in that context, so nice :-)
[...]
@@ -2748,6 +2759,7 @@ cleanup:
struct xenXMListIteratorContext { virConnectPtr conn; + int oom; int max; int count; char ** names; @@ -2757,13 +2769,18 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, struct xenXMListIteratorContext *ctx = data; virDomainPtr dom = NULL;
+ if (ctx->oom) + return; + if (ctx->count == ctx->max) return;
dom = xenDaemonLookupByName(ctx->conn, name); if (!dom) { - ctx->names[ctx->count] = strdup(name); - ctx->count++; + if (!(ctx->names[ctx->count] = strdup(name))) + ctx->oom = 1; + else + ctx->count++; } else { virDomainFree(dom); } @@ -2777,7 +2794,7 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) { xenUnifiedPrivatePtr priv; struct xenXMListIteratorContext ctx; - int ret = -1; + int i, ret = -1;
if (!VIR_IS_CONNECT(conn)) { xenXMError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__); @@ -2794,11 +2811,21 @@ int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames maxnames = virHashSize(priv->configCache);
ctx.conn = conn; + ctx.oom = 0; ctx.count = 0; ctx.max = maxnames; ctx.names = names;
virHashForEach(priv->nameConfigMap, xenXMListIterator, &ctx); + + if (ctx.oom) { + for (i = 0; i < ctx.count; i++) + VIR_FREE(ctx.names[i]); + + virReportOOMError(conn); + goto cleanup; + } + ret = ctx.count;
cleanup:
So you had to add a filed in the iterator structure to report OOMs while running the iterator, nice work !
Well, DPB used this pattern in his "Convert virDomainObjListPtr to use a hash of domain objects" patch, I just applied it here too :-)
ACK except for the one in virNodeDeviceDefParseXML(), very good job you must have spent a lot of time, thanks a lot !
Daniel
It took some hours, but the task was simple: search and fix. Matthias

On Mon, Nov 09, 2009 at 12:07:40PM +0100, Matthias Bolte wrote:
2009/11/9 Daniel Veillard <veillard@redhat.com>:
On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote:
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c2c5a44..c5083cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create /* Extract device name */ if (create == EXISTING_DEVICE) { def->name = virXPathString(conn, "string(./name[1])", ctxt); + + if (!def->name) { + virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); + goto error; + } } else { def->name = strdup("new device"); - }
- if (!def->name) { - virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL); - goto error; + if (!def->name) { + virReportOOMError(conn); + goto error; + } }
/* Extract device parent, if any */
I disagree with this one. The XPath string(./name[1]) can fail without this being an allocation error, it mays just be that there is no name element at the current node, and current behaviour looks better to me. Moreover if virXPathString() returns NULL because of a string allocation error it will already raise an OOMError
I think you misread this one. The original code assigns the result of virXPathString() or strdup() to def->name. After that it checks def->name for NULL and reports an no-name error even if the NULL was returned by strdup(), indicating an OOM error.
Whoops, right :-) Rereading the patch it's fine !
I moved the no-name error report into the virXPathString() case and added an OOM error in the strdup() case. [...]
So you had to add a filed in the iterator structure to report OOMs while running the iterator, nice work !
Well, DPB used this pattern in his "Convert virDomainObjListPtr to use a hash of domain objects" patch, I just applied it here too :-)
ACK except for the one in virNodeDeviceDefParseXML(), very good job you must have spent a lot of time, thanks a lot !
Daniel
It took some hours, but the task was simple: search and fix.
Well thanks a lot ! and ACK :-) Danie -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard
-
Matthias Bolte