[libvirt] [PATCH for 4.6.0 0/3] Revert VIR_AUTOPTR work in cgroup
Turns out, our code relies on virCgroupFree(&var) setting var = NULL. Otherwise a double free can occur: https://www.redhat.com/archives/libvir-list/2018-July/msg02004.html Rather than inserting var = NULL after each virCgroupFree() call lets revert the patches. The ideal solution would be to use virCgroupFree both directly and as attribute cleanup. Michal Prívozník (3): Revert "util: cgroup: use VIR_AUTOPTR for aggregate types" Revert "util: cgroup: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC" Revert "util: cgroup: modify virCgroupFree to take virCgroupPtr" src/libvirt-lxc.c | 4 +- src/lxc/lxc_cgroup.c | 4 +- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 10 +- src/qemu/qemu_cgroup.c | 16 ++-- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 34 +++---- src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 233 +++++++++++++++++++++++++++++------------------ src/util/vircgroup.h | 11 +-- src/util/vircgrouppriv.h | 2 +- tests/vircgrouptest.c | 42 ++++----- 14 files changed, 209 insertions(+), 157 deletions(-) -- 2.16.4
This reverts commit dd47145aaad780cde0f1d67cf6a85737c0292418. Turns out, our code relies on virCgroupFree(&var) setting var = NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroup.c | 200 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 127 insertions(+), 73 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 472a8167f5..6f7b5b40f7 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -836,21 +836,25 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, { VIR_AUTOFREE(char *) prefix = NULL; VIR_AUTOFREE(char *) str = NULL; - VIR_AUTOPTR(virString) lines = NULL; + char **lines = NULL; + int ret = -1; if (virCgroupGetValueStr(group, controller, key, &str) < 0) - return -1; + goto error; if (!(prefix = virCgroupGetBlockDevString(path))) - return -1; + goto error; if (!(lines = virStringSplit(str, "\n", -1))) - return -1; + goto error; if (VIR_STRDUP(*value, virStringListGetFirstWithPrefix(lines, prefix)) < 0) - return -1; + goto error; - return 0; + ret = 0; + error: + virStringListFree(lines); + return ret; } @@ -1213,11 +1217,12 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller) static int virCgroupSetPartitionSuffix(const char *path, char **res) { - VIR_AUTOPTR(virString) tokens = NULL; + char **tokens; size_t i; + int ret = -1; if (!(tokens = virStringSplit(path, "/", 0))) - return -1; + return ret; for (i = 0; tokens[i] != NULL; i++) { /* Whitelist the 3 top level fixed dirs @@ -1236,18 +1241,22 @@ virCgroupSetPartitionSuffix(const char *path, char **res) !strchr(tokens[i], '.')) { if (VIR_REALLOC_N(tokens[i], strlen(tokens[i]) + strlen(".partition") + 1) < 0) - return -1; + goto cleanup; strcat(tokens[i], ".partition"); } if (virCgroupPartitionEscape(&(tokens[i])) < 0) - return -1; + goto cleanup; } if (!(*res = virStringListJoin((const char **)tokens, "/"))) - return -1; + goto cleanup; - return 0; + ret = 0; + + cleanup: + virStringListFree(tokens); + return ret; } @@ -1268,10 +1277,10 @@ virCgroupNewPartition(const char *path, int controllers, virCgroupPtr *group) { + int ret = -1; VIR_AUTOFREE(char *) parentPath = NULL; VIR_AUTOFREE(char *) newPath = NULL; - VIR_AUTOPTR(virCgroup) parent = NULL; - VIR_AUTOPTR(virCgroup) tmpGroup = NULL; + virCgroupPtr parent = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -1283,31 +1292,35 @@ virCgroupNewPartition(const char *path, } if (virCgroupSetPartitionSuffix(path, &newPath) < 0) - return -1; + goto cleanup; - if (virCgroupNew(-1, newPath, NULL, controllers, &tmpGroup) < 0) - return -1; + if (virCgroupNew(-1, newPath, NULL, controllers, group) < 0) + goto cleanup; if (STRNEQ(newPath, "/")) { char *tmp; if (VIR_STRDUP(parentPath, newPath) < 0) - return -1; + goto cleanup; tmp = strrchr(parentPath, '/'); tmp++; *tmp = '\0'; if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0) - return -1; + goto cleanup; - if (virCgroupMakeGroup(parent, tmpGroup, create, VIR_CGROUP_NONE) < 0) { - virCgroupRemove(tmpGroup); - return -1; + if (virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE) < 0) { + virCgroupRemove(*group); + goto cleanup; } } - VIR_STEAL_PTR(*group, tmpGroup); - return 0; + ret = 0; + cleanup: + if (ret != 0) + virCgroupFree(*group); + virCgroupFree(parent); + return ret; } @@ -1489,9 +1502,9 @@ virCgroupNewMachineSystemd(const char *name, int controllers, virCgroupPtr *group) { + int ret = -1; int rv; - VIR_AUTOPTR(virCgroup) init = NULL; - VIR_AUTOPTR(virCgroup) parent = NULL; + virCgroupPtr init, parent = NULL; VIR_AUTOFREE(char *) path = NULL; char *offset; @@ -1518,10 +1531,12 @@ virCgroupNewMachineSystemd(const char *name, path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement; init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL; + virCgroupFree(init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller"); - return -2; + ret = -2; + goto cleanup; } offset = path; @@ -1531,7 +1546,7 @@ virCgroupNewMachineSystemd(const char *name, NULL, controllers, &parent) < 0) - return -1; + goto cleanup; for (;;) { @@ -1545,11 +1560,11 @@ virCgroupNewMachineSystemd(const char *name, parent, controllers, &tmp) < 0) - return -1; + goto cleanup; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) { virCgroupFree(tmp); - return -1; + goto cleanup; } if (t) { *t = '/'; @@ -1572,7 +1587,10 @@ virCgroupNewMachineSystemd(const char *name, } } - return 0; + ret = 0; + cleanup: + virCgroupFree(parent); + return ret; } @@ -1593,7 +1611,8 @@ virCgroupNewMachineManual(const char *name, int controllers, virCgroupPtr *group) { - VIR_AUTOPTR(virCgroup) parent = NULL; + virCgroupPtr parent = NULL; + int ret = -1; VIR_DEBUG("Fallback to non-systemd setup"); if (virCgroupNewPartition(partition, @@ -1601,9 +1620,9 @@ virCgroupNewMachineManual(const char *name, controllers, &parent) < 0) { if (virCgroupNewIgnoreError()) - return 0; + goto done; - return -1; + goto cleanup; } if (virCgroupNewDomainPartition(parent, @@ -1611,7 +1630,7 @@ virCgroupNewMachineManual(const char *name, name, true, group) < 0) - return -1; + goto cleanup; if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); @@ -1623,7 +1642,12 @@ virCgroupNewMachineManual(const char *name, } } - return 0; + done: + ret = 0; + + cleanup: + virCgroupFree(parent); + return ret; } @@ -2352,7 +2376,7 @@ static virOnceControl virCgroupMemoryOnce = VIR_ONCE_CONTROL_INITIALIZER; static void virCgroupMemoryOnceInit(void) { - VIR_AUTOPTR(virCgroup) group = NULL; + virCgroupPtr group; unsigned long long int mem_unlimited = 0ULL; if (virCgroupNew(-1, "/", NULL, -1, &group) < 0) @@ -2366,6 +2390,7 @@ virCgroupMemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: + virCgroupFree(group); virCgroupMemoryUnlimitedKB = mem_unlimited >> 10; } @@ -2966,21 +2991,22 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, size_t nsum, virBitmapPtr cpumap) { + int ret = -1; ssize_t i = -1; + virCgroupPtr group_vcpu = NULL; while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { VIR_AUTOFREE(char *) buf = NULL; - VIR_AUTOPTR(virCgroup) group_vcpu = NULL; char *pos; unsigned long long tmp; ssize_t j; if (virCgroupNewThread(group, VIR_CGROUP_THREAD_VCPU, i, false, &group_vcpu) < 0) - return -1; + goto cleanup; if (virCgroupGetCpuacctPercpuUsage(group_vcpu, &buf) < 0) - return -1; + goto cleanup; pos = buf; for (j = virBitmapNextSetBit(cpumap, -1); @@ -2989,13 +3015,18 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); - return -1; + goto cleanup; } sum_cpu_time[j] += tmp; } + + virCgroupFree(group_vcpu); } - return 0; + ret = 0; + cleanup: + virCgroupFree(group_vcpu); + return ret; } @@ -3027,6 +3058,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int ncpus, virBitmapPtr guestvcpus) { + int ret = -1; size_t i; int need_cpus, total_cpus; char *pos; @@ -3035,7 +3067,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, virTypedParameterPtr ent; int param_idx; unsigned long long cpu_time; - VIR_AUTOPTR(virBitmap) cpumap = NULL; + virBitmapPtr cpumap = NULL; /* return the number of supported params */ if (nparams == 0 && ncpus != 0) { @@ -3052,19 +3084,21 @@ virCgroupGetPercpuStats(virCgroupPtr group, total_cpus = virBitmapSize(cpumap); /* return total number of cpus */ - if (ncpus == 0) - return total_cpus; + if (ncpus == 0) { + ret = total_cpus; + goto cleanup; + } if (start_cpu >= total_cpus) { virReportError(VIR_ERR_INVALID_ARG, _("start_cpu %d larger than maximum of %d"), start_cpu, total_cpus - 1); - return -1; + goto cleanup; } /* we get percpu cputime accounting info. */ if (virCgroupGetCpuacctPercpuUsage(group, &buf)) - return -1; + goto cleanup; pos = buf; /* return percpu cputime in index 0 */ @@ -3079,14 +3113,14 @@ virCgroupGetPercpuStats(virCgroupPtr group, } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); - return -1; + goto cleanup; } if (i < start_cpu) continue; ent = ¶ms[(i - start_cpu) * nparams + param_idx]; if (virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, VIR_TYPED_PARAM_ULLONG, cpu_time) < 0) - return -1; + goto cleanup; } /* return percpu vcputime in index 1 */ @@ -3094,10 +3128,10 @@ virCgroupGetPercpuStats(virCgroupPtr group, if (guestvcpus && param_idx < nparams) { if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) - return -1; + goto cleanup; if (virCgroupGetPercpuVcpuSum(group, guestvcpus, sum_cpu_time, need_cpus, cpumap) < 0) - return -1; + goto cleanup; for (i = start_cpu; i < need_cpus; i++) { if (virTypedParameterAssign(¶ms[(i - start_cpu) * nparams + @@ -3105,13 +3139,17 @@ virCgroupGetPercpuStats(virCgroupPtr group, VIR_DOMAIN_CPU_STATS_VCPUTIME, VIR_TYPED_PARAM_ULLONG, sum_cpu_time[i]) < 0) - return -1; + goto cleanup; } param_idx++; } - return param_idx; + ret = param_idx; + + cleanup: + virBitmapFree(cpumap); + return ret; } @@ -3467,18 +3505,23 @@ int virCgroupKill(virCgroupPtr group, int signum) { VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); + int ret; /* The 'tasks' file in cgroups can contain duplicated * pids, so we use a hash to track which we've already * killed. */ - VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, - NULL, - virCgroupPidCode, - virCgroupPidEqual, - virCgroupPidCopy, - NULL); + virHashTablePtr pids = virHashCreateFull(100, + NULL, + virCgroupPidCode, + virCgroupPidEqual, + virCgroupPidCopy, + NULL); - return virCgroupKillInternal(group, signum, pids); + ret = virCgroupKillInternal(group, signum, pids); + + virHashFree(pids); + + return ret; } @@ -3493,6 +3536,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, bool killedAny = false; VIR_AUTOFREE(char *) keypath = NULL; DIR *dp = NULL; + virCgroupPtr subgroup = NULL; struct dirent *ent; int direrr; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", @@ -3517,8 +3561,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, } while ((direrr = virDirRead(dp, &ent, keypath)) > 0) { - VIR_AUTOPTR(virCgroup) subgroup = NULL; - if (ent->d_type != DT_DIR) continue; @@ -3535,6 +3577,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); + + virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -3543,6 +3587,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: + virCgroupFree(subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -3551,15 +3596,20 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int virCgroupKillRecursive(virCgroupPtr group, int signum) { + int ret; VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, - NULL, - virCgroupPidCode, - virCgroupPidEqual, - virCgroupPidCopy, - NULL); + virHashTablePtr pids = virHashCreateFull(100, + NULL, + virCgroupPidCode, + virCgroupPidEqual, + virCgroupPidCopy, + NULL); - return virCgroupKillRecursiveInternal(group, signum, pids, false); + ret = virCgroupKillRecursiveInternal(group, signum, pids, false); + + virHashFree(pids); + + return ret; } @@ -3894,12 +3944,15 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) bool virCgroupControllerAvailable(int controller) { - VIR_AUTOPTR(virCgroup) cgroup = NULL; + virCgroupPtr cgroup; + bool ret = false; if (virCgroupNewSelf(&cgroup) < 0) - return false; + return ret; - return virCgroupHasController(cgroup, controller); + ret = virCgroupHasController(cgroup, controller); + virCgroupFree(cgroup); + return ret; } #else /* !VIR_CGROUP_SUPPORTED */ @@ -4687,7 +4740,7 @@ virCgroupDelThread(virCgroupPtr cgroup, virCgroupThreadName nameval, int idx) { - VIR_AUTOPTR(virCgroup) new_cgroup = NULL; + virCgroupPtr new_cgroup = NULL; if (cgroup) { if (virCgroupNewThread(cgroup, nameval, idx, false, &new_cgroup) < 0) @@ -4695,6 +4748,7 @@ virCgroupDelThread(virCgroupPtr cgroup, /* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); + virCgroupFree(new_cgroup); } return 0; -- 2.16.4
This reverts commit 4da4a9fe0c0956feefe3d592b4ba2b92b2a9a2f9. Turns out, our code relies on virCgroupFree(&var) setting var = NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroup.c | 1 + src/util/vircgroup.h | 9 ++------- src/util/vircgrouppriv.h | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6f7b5b40f7..4e34bf5885 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -50,6 +50,7 @@ #include "vircgrouppriv.h" #include "virutil.h" +#include "viralloc.h" #include "virerror.h" #include "virlog.h" #include "virfile.h" diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 065861d700..e4ffd57b6b 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -27,11 +27,9 @@ # include "virutil.h" # include "virbitmap.h" -# include "viralloc.h" -struct _virCgroup; -typedef struct _virCgroup virCgroup; -typedef virCgroup *virCgroupPtr; +struct virCgroup; +typedef struct virCgroup *virCgroupPtr; enum { VIR_CGROUP_CONTROLLER_CPU, @@ -299,7 +297,4 @@ int virCgroupSetOwner(virCgroupPtr cgroup, int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); bool virCgroupControllerAvailable(int controller); - -VIR_DEFINE_AUTOPTR_FUNC(virCgroup, virCgroupFree) - #endif /* __VIR_CGROUP_H__ */ diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index a72bee1ef2..722863e5b6 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -42,7 +42,7 @@ struct virCgroupController { char *placement; }; -struct _virCgroup { +struct virCgroup { char *path; struct virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST]; -- 2.16.4
This reverts commit 0f80c71822d82465d558d697d3be9af2d21e3675. Turns out, our code relies on virCgroupFree(&var) setting var = NULL. Conflicts: src/util/vircgroup.c: context because 94f1855f099445d is not reverted. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-lxc.c | 4 ++-- src/lxc/lxc_cgroup.c | 4 ++-- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 10 ++++----- src/qemu/qemu_cgroup.c | 16 +++++++------- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 34 +++++++++++++++-------------- src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 56 ++++++++++++++++++++++++------------------------ src/util/vircgroup.h | 2 +- tests/vircgrouptest.c | 42 ++++++++++++++++++------------------ 13 files changed, 90 insertions(+), 88 deletions(-) diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c index 12be89398e..c9f2146487 100644 --- a/src/libvirt-lxc.c +++ b/src/libvirt-lxc.c @@ -309,12 +309,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain, if (virCgroupAddTask(cgroup, getpid()) < 0) goto error; - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return 0; error: virDispatchError(NULL); - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return -1; } diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 873c843542..8e937ec389 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -306,7 +306,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo) ret = 0; cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -515,7 +515,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, def->idmap.uidmap[0].target, def->idmap.gidmap[0].target, (1 << VIR_CGROUP_CONTROLLER_SYSTEMD)) < 0) { - virCgroupFree(cgroup); + virCgroupFree(&cgroup); cgroup = NULL; goto cleanup; } diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 407214fced..3a1b2d6819 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1815,7 +1815,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, cleanup: VIR_FREE(stateDir); - virCgroupFree(cgroup); + virCgroupFree(&cgroup); VIR_FREE(sec_mount_options); return ret; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7be45f84ce..4e84391bf5 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -296,7 +296,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) VIR_FREE(ctrl->nbdpids); VIR_FREE(ctrl->nsFDs); - virCgroupFree(ctrl->cgroup); + virCgroupFree(&ctrl->cgroup); /* This must always be the last thing to be closed */ VIR_FORCE_CLOSE(ctrl->handshakeFd); diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index eb0071d07a..b197f9dfc2 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -172,7 +172,7 @@ virLXCDomainObjPrivateFree(void *data) { virLXCDomainObjPrivatePtr priv = data; - virCgroupFree(priv->cgroup); + virCgroupFree(&priv->cgroup); virLXCDomainObjFreeJob(priv); VIR_FREE(priv); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 5d8fa63c67..33c806630b 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -220,7 +220,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, if (priv->cgroup) { virCgroupRemove(priv->cgroup); - virCgroupFree(priv->cgroup); + virCgroupFree(&priv->cgroup); } /* Get machined to terminate the machine as it may not have cleaned it @@ -1201,26 +1201,26 @@ int virLXCProcessStart(virConnectPtr conn, if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) { - virCgroupFree(selfcgroup); + virCgroupFree(&selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'cpuacct' cgroups controller mount")); return -1; } if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { - virCgroupFree(selfcgroup); + virCgroupFree(&selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'devices' cgroups controller mount")); return -1; } if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { - virCgroupFree(selfcgroup); + virCgroupFree(&selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'memory' cgroups controller mount")); return -1; } - virCgroupFree(selfcgroup); + virCgroupFree(&selfcgroup); if (vm->def->nconsoles == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 8a00ffcd45..43e17d786e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -841,7 +841,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm) ret = 0; cleanup: VIR_FREE(mem_mask); - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); return ret; } @@ -920,7 +920,7 @@ qemuInitCgroup(virDomainObjPtr vm, if (!virCgroupAvailable()) goto done; - virCgroupFree(priv->cgroup); + virCgroupFree(&priv->cgroup); if (!vm->def->resource) { virDomainResourceDefPtr res; @@ -1008,7 +1008,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) goto cleanup; VIR_FREE(nodeset); - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); } for (i = 0; i < vm->def->niothreadids; i++) { @@ -1021,7 +1021,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) goto cleanup; VIR_FREE(nodeset); - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); } if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, @@ -1035,7 +1035,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) VIR_FREE(mem_mask); VIR_FREE(nodeset); virBitmapFree(all_nodes); - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); return; error: @@ -1057,7 +1057,7 @@ qemuConnectCgroup(virDomainObjPtr vm) if (!virCgroupAvailable()) goto done; - virCgroupFree(priv->cgroup); + virCgroupFree(&priv->cgroup); if (virCgroupNewDetectMachine(vm->def->name, "qemu", @@ -1203,7 +1203,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm, ret = qemuExtDevicesSetupCgroup(driver, vm->def, cgroup_temp); cleanup: - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); return ret; } @@ -1281,7 +1281,7 @@ qemuCgroupEmulatorAllNodesDataFree(qemuCgroupEmulatorAllNodesDataPtr data) if (!data) return; - virCgroupFree(data->emulatorCgroup); + virCgroupFree(&data->emulatorCgroup); VIR_FREE(data->emulatorMemMask); VIR_FREE(data); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bda53814a3..de056272e8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1920,7 +1920,7 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) virStringListFree(priv->qemuDevices); priv->qemuDevices = NULL; - virCgroupFree(priv->cgroup); + virCgroupFree(&priv->cgroup); virPerfFree(priv->perf); priv->perf = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7018076c4..fb0d4a8c7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5078,7 +5078,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, cleanup: virBitmapFree(tmpmap); - virCgroupFree(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); VIR_FREE(str); virObjectEventStateQueue(driver->domainEventState, event); return ret; @@ -5315,7 +5315,8 @@ qemuDomainPinEmulator(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - virCgroupFree(cgroup_emulator); + if (cgroup_emulator) + virCgroupFree(&cgroup_emulator); virObjectEventStateQueue(driver->domainEventState, event); VIR_FREE(str); virBitmapFree(pcpumap); @@ -5796,7 +5797,8 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - virCgroupFree(cgroup_iothread); + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); virObjectEventStateQueue(driver->domainEventState, event); VIR_FREE(str); virBitmapFree(pcpumap); @@ -9850,7 +9852,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); @@ -9862,7 +9864,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); } for (i = 0; i < vm->def->niothreadids; i++) { @@ -9871,13 +9873,13 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); } ret = 0; cleanup: VIR_FREE(nodeset_str); - virCgroupFree(cgroup_temp); + virCgroupFree(&cgroup_temp); return ret; } @@ -10299,13 +10301,13 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) goto cleanup; - virCgroupFree(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); } return 0; cleanup: - virCgroupFree(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); return -1; } @@ -10326,11 +10328,11 @@ qemuSetEmulatorBandwidthLive(virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_emulator, period, quota) < 0) goto cleanup; - virCgroupFree(cgroup_emulator); + virCgroupFree(&cgroup_emulator); return 0; cleanup: - virCgroupFree(cgroup_emulator); + virCgroupFree(&cgroup_emulator); return -1; } @@ -10357,13 +10359,13 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) goto cleanup; - virCgroupFree(cgroup_iothread); + virCgroupFree(&cgroup_iothread); } return 0; cleanup: - virCgroupFree(cgroup_iothread); + virCgroupFree(&cgroup_iothread); return -1; } @@ -10749,7 +10751,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm, ret = 0; cleanup: - virCgroupFree(cgroup_vcpu); + virCgroupFree(&cgroup_vcpu); return ret; } @@ -10772,7 +10774,7 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup, ret = 0; cleanup: - virCgroupFree(cgroup_emulator); + virCgroupFree(&cgroup_emulator); return ret; } @@ -10808,7 +10810,7 @@ qemuGetIOThreadsBWLive(virDomainObjPtr vm, ret = 0; cleanup: - virCgroupFree(cgroup_iothread); + virCgroupFree(&cgroup_iothread); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2f0b4c1d80..c4e33723d1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2532,7 +2532,7 @@ qemuProcessSetupPid(virDomainObjPtr vm, if (cgroup) { if (ret < 0) virCgroupRemove(cgroup); - virCgroupFree(cgroup); + virCgroupFree(&cgroup); } return ret; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 4e34bf5885..6bf4e88da1 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1118,7 +1118,7 @@ virCgroupNew(pid_t pid, return 0; error: - virCgroupFree(*group); + virCgroupFree(group); *group = NULL; return -1; @@ -1319,8 +1319,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(*group); - virCgroupFree(parent); + virCgroupFree(group); + virCgroupFree(&parent); return ret; } @@ -1384,7 +1384,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition, if (virCgroupMakeGroup(partition, *group, create, VIR_CGROUP_MEM_HIERACHY) < 0) { virCgroupRemove(*group); - virCgroupFree(*group); + virCgroupFree(group); return -1; } @@ -1441,7 +1441,7 @@ virCgroupNewThread(virCgroupPtr domain, if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { virCgroupRemove(*group); - virCgroupFree(*group); + virCgroupFree(group); return -1; } @@ -1479,7 +1479,7 @@ virCgroupNewDetectMachine(const char *name, true, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); - virCgroupFree(*group); + virCgroupFree(group); return 0; } @@ -1532,7 +1532,7 @@ virCgroupNewMachineSystemd(const char *name, path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement; init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL; - virCgroupFree(init); + virCgroupFree(&init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller"); @@ -1564,13 +1564,13 @@ virCgroupNewMachineSystemd(const char *name, goto cleanup; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) { - virCgroupFree(tmp); + virCgroupFree(&tmp); goto cleanup; } if (t) { *t = '/'; offset = t; - virCgroupFree(parent); + virCgroupFree(&parent); parent = tmp; } else { *group = tmp; @@ -1581,7 +1581,7 @@ virCgroupNewMachineSystemd(const char *name, if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); virCgroupRemove(*group); - virCgroupFree(*group); + virCgroupFree(group); if (saved) { virSetError(saved); virFreeError(saved); @@ -1590,7 +1590,7 @@ virCgroupNewMachineSystemd(const char *name, ret = 0; cleanup: - virCgroupFree(parent); + virCgroupFree(&parent); return ret; } @@ -1636,7 +1636,7 @@ virCgroupNewMachineManual(const char *name, if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); virCgroupRemove(*group); - virCgroupFree(*group); + virCgroupFree(group); if (saved) { virSetError(saved); virFreeError(saved); @@ -1647,7 +1647,7 @@ virCgroupNewMachineManual(const char *name, ret = 0; cleanup: - virCgroupFree(parent); + virCgroupFree(&parent); return ret; } @@ -1714,21 +1714,21 @@ virCgroupNewIgnoreError(void) * @group: The group structure to free */ void -virCgroupFree(virCgroupPtr group) +virCgroupFree(virCgroupPtr *group) { size_t i; - if (!group) + if (*group == NULL) return; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - VIR_FREE(group->controllers[i].mountPoint); - VIR_FREE(group->controllers[i].linkPoint); - VIR_FREE(group->controllers[i].placement); + VIR_FREE((*group)->controllers[i].mountPoint); + VIR_FREE((*group)->controllers[i].linkPoint); + VIR_FREE((*group)->controllers[i].placement); } - VIR_FREE(group->path); - VIR_FREE(group); + VIR_FREE((*group)->path); + VIR_FREE(*group); } @@ -2391,7 +2391,7 @@ virCgroupMemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: - virCgroupFree(group); + virCgroupFree(&group); virCgroupMemoryUnlimitedKB = mem_unlimited >> 10; } @@ -3021,12 +3021,12 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, sum_cpu_time[j] += tmp; } - virCgroupFree(group_vcpu); + virCgroupFree(&group_vcpu); } ret = 0; cleanup: - virCgroupFree(group_vcpu); + virCgroupFree(&group_vcpu); return ret; } @@ -3579,7 +3579,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); - virCgroupFree(subgroup); + virCgroupFree(&subgroup); } if (direrr < 0) goto cleanup; @@ -3588,7 +3588,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - virCgroupFree(subgroup); + virCgroupFree(&subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -3952,7 +3952,7 @@ virCgroupControllerAvailable(int controller) return ret; ret = virCgroupHasController(cgroup, controller); - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -4084,7 +4084,7 @@ virCgroupNewIgnoreError(void) void -virCgroupFree(virCgroupPtr group ATTRIBUTE_UNUSED) +virCgroupFree(virCgroupPtr *group ATTRIBUTE_UNUSED) { virReportSystemError(ENXIO, "%s", _("Control groups not supported on this platform")); @@ -4749,7 +4749,7 @@ virCgroupDelThread(virCgroupPtr cgroup, /* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); - virCgroupFree(new_cgroup); + virCgroupFree(&new_cgroup); } return 0; diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index e4ffd57b6b..d833927678 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -122,7 +122,7 @@ int virCgroupTerminateMachine(const char *name) bool virCgroupNewIgnoreError(void); -void virCgroupFree(virCgroupPtr group); +void virCgroupFree(virCgroupPtr *group); bool virCgroupHasController(virCgroupPtr cgroup, int controller); int virCgroupPathOfController(virCgroupPtr group, diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index e5190e3b34..be50f3e73c 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -198,7 +198,7 @@ testCgroupDetectMounts(const void *args) cleanup: VIR_FREE(mounts); VIR_FREE(parsed); - virCgroupFree(group); + virCgroupFree(&group); virBufferFreeAndReset(&buf); return result; } @@ -227,7 +227,7 @@ static int testCgroupNewForSelf(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsFull, links, placement); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -304,7 +304,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED) goto cleanup; } ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall); - virCgroupFree(cgroup); + virCgroupFree(&cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); @@ -313,7 +313,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -353,7 +353,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_UNUSED) } /* Should now work */ - virCgroupFree(cgroup); + virCgroupFree(&cgroup); if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /deployment/production cgroup: %d\n", -rv); goto cleanup; @@ -363,7 +363,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_UNUSED) mountsFull, links, placementFull); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -402,14 +402,14 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_UNUSED goto cleanup; } - virCgroupFree(cgroup); + virCgroupFree(&cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv); goto cleanup; } /* Should now work */ - virCgroupFree(cgroup); + virCgroupFree(&cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user/production cgroup: %d\n", -rv); goto cleanup; @@ -419,7 +419,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_UNUSED mountsFull, links, placementFull); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -455,8 +455,8 @@ static int testCgroupNewForPartitionDomain(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement); cleanup: - virCgroupFree(partitioncgroup); - virCgroupFree(domaincgroup); + virCgroupFree(&partitioncgroup); + virCgroupFree(&domaincgroup); return ret; } @@ -506,10 +506,10 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args ATTRIBUTE_UNU ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement); cleanup: - virCgroupFree(partitioncgroup3); - virCgroupFree(partitioncgroup2); - virCgroupFree(partitioncgroup1); - virCgroupFree(domaincgroup); + virCgroupFree(&partitioncgroup3); + virCgroupFree(&partitioncgroup2); + virCgroupFree(&partitioncgroup1); + virCgroupFree(&domaincgroup); return ret; } @@ -535,7 +535,7 @@ static int testCgroupNewForSelfAllInOne(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -563,7 +563,7 @@ static int testCgroupNewForSelfLogind(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsLogind, linksLogind, placement); cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -690,7 +690,7 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); VIR_FREE(params); return ret; } @@ -723,7 +723,7 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -773,7 +773,7 @@ static int testCgroupGetBlkioIoServiced(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } @@ -846,7 +846,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(cgroup); + virCgroupFree(&cgroup); return ret; } -- 2.16.4
On Mon, Jul 30, 2018 at 11:19:32AM +0200, Michal Privoznik wrote:
Turns out, our code relies on virCgroupFree(&var) setting var = NULL. Otherwise a double free can occur:
https://www.redhat.com/archives/libvir-list/2018-July/msg02004.html
Rather than inserting var = NULL after each virCgroupFree() call lets revert the patches. The ideal solution would be to use virCgroupFree both directly and as attribute cleanup.
Michal Prívozník (3): Revert "util: cgroup: use VIR_AUTOPTR for aggregate types" Revert "util: cgroup: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC" Revert "util: cgroup: modify virCgroupFree to take virCgroupPtr"
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Michal Privoznik -
Pavel Hrdina