[libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing
When virCgroupEnableMissingControllers fails it's possible that *group is still set to NULL. Therefore let's add a guard and an attribute for this. [#0] virCgroupRemove(group=0x0) [#1] virCgroupNewMachineSystemd [#2] virCgroupNewMachine [#3] qemuInitCgroup [#4] qemuSetupCgroup [#5] qemuProcessLaunch [#6] qemuProcessStart [#7] qemuDomainObjStart [#8] qemuDomainCreateWithFlags [#9] qemuDomainCreate ... Fixes: 1602aa28f820ada66f707cef3e536e8572fbda1e Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/util/vircgroup.c | 3 ++- src/util/vircgroup.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) 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); int virCgroupKillRecursive(virCgroupPtr group, int signum); int virCgroupKillPainfully(virCgroupPtr group); -- 2.17.0
$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@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- 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);
On Thu, Sep 27, 2018 at 02:38 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
$subj:
util: Fix cgroup processing NULL pointer dereferencing
I’m fine with this change :)
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:".
Okay.
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...
Okay.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- 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.
Okay.
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.
Thanks for pointing this out! :) I thought it could help Coverity.
int virCgroupKillRecursive(virCgroupPtr group, int signum); int virCgroupKillPainfully(virCgroupPtr group);
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
John Ferlan -
Marc Hartmayer