Re: [libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing

$subj: util: Fix cgroup processing NULL pointer dereferencing

On 9/26/18 11:53 AM, Marc Hartmayer wrote: > When virCgroupEnableMissingControllers fails it's possible that *group > is still set to NULL. Therefore let's add a guard and an attribute for > this. Prefix paragraph with rather than at the bottom "Fixes:".

Introduced by commit 1602aa28f, > > [#0] virCgroupRemove(group=0x0) > [#1] virCgroupNewMachineSystemd > [#2] virCgroupNewMachine > [#3] qemuInitCgroup > [#4] qemuSetupCgroup > [#5] qemuProcessLaunch > [#6] qemuProcessStart > [#7] qemuDomainObjStart > [#8] qemuDomainCreateWithFlags > [#9] qemuDomainCreate > ... > > Fixes: 1602aa28f820ada66f707cef3e536e8572fbda1e I think it's safe to remove the stack trace...

> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > --- > src/util/vircgroup.c | 3 ++- > src/util/vircgroup.h | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) While this patch is correct to remove the NULL deref, there "may" be a problem with the patch that introduced this. Rather than usurp this thread, I'll respond to the other one and see where it takes us.

John > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 23957c82c7fa..06e1d158febb 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -1104,7 +1104,8 @@ virCgroupNewMachineSystemd(const char *name, > > error: > saved = virSaveLastError(); > - virCgroupRemove(*group); > + if (*group) > + virCgroupRemove(*group); > virCgroupFree(group); > if (saved) { > virSetError(saved); > diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h > index 1f676f21c380..9e1ae3706b1e 100644 > --- a/src/util/vircgroup.h > +++ b/src/util/vircgroup.h > @@ -268,7 +268,8 @@ int virCgroupGetCpusetMemoryMigrate(virCgroupPtr group, bool *migrate); > int virCgroupSetCpusetCpus(virCgroupPtr group, const char *cpus); > int virCgroupGetCpusetCpus(virCgroupPtr group, char **cpus); > > -int virCgroupRemove(virCgroupPtr group); > +int virCgroupRemove(virCgroupPtr group) > + ATTRIBUTE_NONNULL(1); FWIW: This only helps if someone tried to call virCgroupRemove(NULL); it doesn't help if the parameter itself is NULL. One could add a "if (!group) return 0" to virCgroupRemove to avoid.

> > int virCgroupKillRecursive(virCgroupPtr group, int signum); > int virCgroupKillPainfully(virCgroupPtr group); >