Re: [libvirt PATCH 03/10] util: vircgroup: change virCgroupFree to take only virCgroupPtr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux