Re: [libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing
On Thu, Sep 27, 2018 at 02:38 PM +0200, John Ferlan 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
>> Reviewed-by: Bjoern Walk
>> Signed-off-by: Marc Hartmayer
>> ---
>> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
> Reviewed-by: Bjoern Walk
> Signed-off-by: Marc Hartmayer
> ---
> 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);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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
Reviewed-by: Bjoern Walk
Signed-off-by: Marc Hartmayer
---
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
