On Thu, Oct 08, 2020 at 11:36:56AM -0500, Jonathon Jongsma wrote:
> On Thu, 8 Oct 2020 16:26:56 +0200
> Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
>
> > As preparation for g_autoptr() we need to change the function to take
> > only virCgroupPtr.
> >
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > 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 | 11 +++---
> > src/qemu/qemu_cgroup.c | 12 +++---
> > src/qemu/qemu_domain.c | 3 +-
> > src/qemu/qemu_driver.c | 32 +++++++--------
> > src/qemu/qemu_process.c | 2 +-
> > src/util/vircgroup.c | 65
> > +++++++++++++++++-------------- src/util/vircgroup.h |
> > 2 +- src/util/vircgroupv1.c | 2 +-
> > tests/vircgrouptest.c | 48 +++++++++++------------
> > tools/virt-host-validate-common.c | 2 +-
> > 15 files changed, 102 insertions(+), 91 deletions(-)
> >
> > diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
> > index 25f1cfc5f7..73daf123f0 100644
> > --- a/src/libvirt-lxc.c
> > +++ b/src/libvirt-lxc.c
> > @@ -307,12 +307,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain,
> > if (virCgroupAddProcess(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 d13f2adde5..b80a8911f9 100644
> > --- a/src/lxc/lxc_cgroup.c
> > +++ b/src/lxc/lxc_cgroup.c
> > @@ -168,7 +168,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr
> > meminfo)
> > ret = 0;
> > cleanup:
> > - virCgroupFree(&cgroup);
> > + virCgroupFree(cgroup);
> > return ret;
> > }
> >
> > @@ -417,7 +417,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);
> > return NULL;
> > }
> > }
> > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> > index d1aa622be4..913f4de26a 100644
> > --- a/src/lxc/lxc_container.c
> > +++ b/src/lxc/lxc_container.c
> > @@ -1668,7 +1668,7 @@ static int
> > lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, ret = 0;
> >
> > cleanup:
> > - virCgroupFree(&cgroup);
> > + virCgroupFree(cgroup);
> > return ret;
> > }
> >
> > diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> > index f70731bc64..e6dee85ec7 100644
> > --- a/src/lxc/lxc_controller.c
> > +++ b/src/lxc/lxc_controller.c
> > @@ -310,7 +310,7 @@ static void
> > virLXCControllerFree(virLXCControllerPtr ctrl) g_free(ctrl->nbdpids);
> >
> > g_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 d8aebe06d9..df60519fca 100644
> > --- a/src/lxc/lxc_domain.c
> > +++ b/src/lxc/lxc_domain.c
> > @@ -168,7 +168,7 @@ virLXCDomainObjPrivateFree(void *data)
> > {
> > virLXCDomainObjPrivatePtr priv = data;
> >
> > - virCgroupFree(&priv->cgroup);
> > + virCgroupFree(priv->cgroup);
> > virLXCDomainObjFreeJob(priv);
> > g_free(priv);
> > }
> > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> > index 16969dbf33..a98a090893 100644
> > --- a/src/lxc/lxc_process.c
> > +++ b/src/lxc/lxc_process.c
> > @@ -236,7 +236,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr
> > driver,
> > if (priv->cgroup) {
> > virCgroupRemove(priv->cgroup);
> > - virCgroupFree(&priv->cgroup);
> > + virCgroupFree(priv->cgroup);
> > + priv->cgroup = NULL;
> > }
> >
> > /* Get machined to terminate the machine as it may not have
> > cleaned it @@ -1202,26 +1203,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 9017753053..473ca8a414 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -921,7 +921,8 @@ qemuInitCgroup(virDomainObjPtr vm,
> > if (!virCgroupAvailable())
> > return 0;
> >
> > - virCgroupFree(&priv->cgroup);
> > + virCgroupFree(priv->cgroup);
> > + priv->cgroup = NULL;
> >
> > if (!vm->def->resource) {
> > virDomainResourceDefPtr res;
> > @@ -983,7 +984,7 @@ qemuRestoreCgroupThread(virCgroupPtr cgroup,
> >
> > ret = 0;
> > cleanup:
> > - virCgroupFree(&cgroup_temp);
> > + virCgroupFree(cgroup_temp);
> > return ret;
> > }
> >
> > @@ -1054,7 +1055,8 @@ qemuConnectCgroup(virDomainObjPtr vm)
> > if (!virCgroupAvailable())
> > return 0;
> >
> > - virCgroupFree(&priv->cgroup);
> > + virCgroupFree(priv->cgroup);
> > + priv->cgroup = NULL;
> >
> > if (virCgroupNewDetectMachine(vm->def->name,
> > "qemu",
> > @@ -1150,7 +1152,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm,
> > ret = qemuExtDevicesSetupCgroup(driver, vm, cgroup_temp);
> >
> > cleanup:
> > - virCgroupFree(&cgroup_temp);
> > + virCgroupFree(cgroup_temp);
> >
> > return ret;
> > }
> > @@ -1221,7 +1223,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 9623123d3c..2d7f61f5e9 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -1725,7 +1725,8 @@
> > qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
> > g_strfreev(priv->qemuDevices); priv->qemuDevices = NULL;
> >
> > - virCgroupFree(&priv->cgroup);
> > + virCgroupFree(priv->cgroup);
> > + priv->cgroup = NULL;
> >
> > virPerfFree(priv->perf);
> > priv->perf = NULL;
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 8ef812cd94..60e043115f 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4583,7 +4583,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
> >
> > cleanup:
> > virBitmapFree(tmpmap);
> > - virCgroupFree(&cgroup_vcpu);
> > + virCgroupFree(cgroup_vcpu);
> > virObjectEventStateQueue(driver->domainEventState, event);
> > return ret;
> > }
> > @@ -4809,7 +4809,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
> >
> > cleanup:
> > if (cgroup_emulator)
> > - virCgroupFree(&cgroup_emulator);
> > + virCgroupFree(cgroup_emulator);
> > virObjectEventStateQueue(driver->domainEventState, event);
> > virBitmapFree(pcpumap);
> > virDomainObjEndAPI(&vm);
> > @@ -5287,7 +5287,7 @@ qemuDomainPinIOThread(virDomainPtr dom,
> >
> > cleanup:
> > if (cgroup_iothread)
> > - virCgroupFree(&cgroup_iothread);
> > + virCgroupFree(cgroup_iothread);
> > virObjectEventStateQueue(driver->domainEventState, event);
> > virBitmapFree(pcpumap);
> > virDomainObjEndAPI(&vm);
> > @@ -8717,7 +8717,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);
> > @@ -8729,7 +8729,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++) {
> > @@ -8738,7 +8738,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
> > false, &cgroup_temp) < 0 ||
> > virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0)
> > goto cleanup;
> > - virCgroupFree(&cgroup_temp);
> > + virCgroupFree(cgroup_temp);
> > }
> >
> > /* set nodeset for root cgroup */
> > @@ -8747,7 +8747,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
> >
> > ret = 0;
> > cleanup:
> > - virCgroupFree(&cgroup_temp);
> > + virCgroupFree(cgroup_temp);
>
> I realize that this particular issue only exists until you switch to
> using autoptr in patch 7, but you're technically introducing a bug
> here. virCgroupFree() no longer clears the ptr to NULL, so when you
> free cgroup_temp up above within this function, it is now a dangling
> pointer, and then we try to free it again in the cleanup section.
> Previously this cleanup call to virCgroupFree() was a no-op for all
> non-error paths.
>
> There are several other examples within this patch.
Nice catch! I'll fix it before pushing.
Pavel
Attachment:
signature.asc
Description: PGP signature
