[libvirt] [PATCH] qemu: avoid dereference of null pointer
* src/qemu/qemu_driver.c: avoid dereference of null pointer. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/qemu/qemu_driver.c | 27 ++++++++++++++++++--------- 1 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..28ffff7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5697,7 +5697,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, continue; } - persistentDef->blkio.weight = params[i].value.ui; + if (params[i].value.ul) + persistentDef->blkio.weight = params[i].value.ui; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5837,7 +5838,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, "%s", _("Field blkio weight too long for destination")); goto cleanup; } - param->value.ui = persistentDef->blkio.weight; + if (persistentDef->blkio.weight) + param->value.ui = persistentDef->blkio.weight; break; default: @@ -5946,7 +5948,8 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.hard_limit = params[i].value.ul; + if (params[i].value.ul) + persistentDef->mem.hard_limit = params[i].value.ul; } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) { int rc; @@ -5967,7 +5970,8 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.soft_limit = params[i].value.ul; + if (params[i].value.ul) + persistentDef->mem.soft_limit = params[i].value.ul; } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) { int rc; @@ -5987,7 +5991,8 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, } } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.swap_hard_limit = params[i].value.ul; + if (params[i].value.ul) + persistentDef->mem.swap_hard_limit = params[i].value.ul; } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) { qemuReportError(VIR_ERR_INVALID_ARG, @@ -6107,7 +6112,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, "%s", _("Field memory hard limit too long for destination")); goto cleanup; } - param->value.ul = persistentDef->mem.hard_limit; + if (persistentDef->mem.hard_limit) + param->value.ul = persistentDef->mem.hard_limit; break; case 1: /* fill memory soft limit here */ @@ -6404,7 +6410,8 @@ static int qemuSetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - vm->def->cputune.shares = params[i].value.ul; + if (params[i].value.ul) + vm->def->cputune.shares = params[i].value.ul; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -6428,7 +6435,8 @@ static int qemuSetSchedulerParametersFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.period = params[i].value.ul; + if (params[i].value.ul) + vmdef->cputune.period = params[i].value.ul; } } else if (STREQ(param->field, "vcpu_quota")) { if (param->type != VIR_TYPED_PARAM_LLONG) { @@ -6448,7 +6456,8 @@ static int qemuSetSchedulerParametersFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.quota = params[i].value.l; + if (params[i].value.ul) + vmdef->cputune.quota = params[i].value.l; } } else { qemuReportError(VIR_ERR_INVALID_ARG, -- 1.7.5.1
On 08/04/2011 09:51 AM, Alex Jia wrote:
* src/qemu/qemu_driver.c: avoid dereference of null pointer.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/qemu/qemu_driver.c | 27 ++++++++++++++++++--------- 1 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..28ffff7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5697,7 +5697,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, continue; }
- persistentDef->blkio.weight = params[i].value.ui; + if (params[i].value.ul) + persistentDef->blkio.weight = params[i].value.ui;
Why check ul, but then use ui? That seems broken.
if (flags& VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.quota = params[i].value.l; + if (params[i].value.ul) + vmdef->cputune.quota = params[i].value.l;
Why check ul, but then use l? Also, how does checking for a non-zero union value prevent a null dereference? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/04/2011 09:51 AM, Alex Jia wrote:
* src/qemu/qemu_driver.c: avoid dereference of null pointer.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/qemu/qemu_driver.c | 27 ++++++++++++++++++--------- 1 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..28ffff7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5697,7 +5697,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, continue; }
- persistentDef->blkio.weight = params[i].value.ui; + if (params[i].value.ul) + persistentDef->blkio.weight = params[i].value.ui;
Why check ul, but then use ui? That seems broken. Ah, correct me, this is a typing error.
if (flags& VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.quota = params[i].value.l; + if (params[i].value.ul) + vmdef->cputune.quota = params[i].value.l;
Why check ul, but then use l?
On 08/05/2011 12:18 AM, Eric Blake wrote: this is also a typing error.
Also, how does checking for a non-zero union value prevent a null dereference?
To be honest, I'm not sure this, however, it's okay for ccc-analyzer if I add these judgements, of course, I'm probably making a mistake, if so, please correct me. Thanks, Alex
On 08/04/2011 11:14 AM, Alex Jia wrote:
Also, how does checking for a non-zero union value prevent a null dereference?
To be honest, I'm not sure this, however, it's okay for ccc-analyzer if I add these judgements,
Is ccc-analyzer different from clang? If so, how can I set it up, to reproduce the problem you saw? I previously saw a false positive in one of these functions (qemudDomainGetMemoryParameters) when using Coverity, but that was fixed by commit f768b4c3, but Coverity was silent for the other 4 functions you touched. I'm now trying to do a clang run to see if that differs from Coverity. The Coverity false positive was that our logic confused the static analyzers: type var; // uninit if (flags & _CONFIG) var = something if (flags & _LIVE) do something else if (flags & _CONFIG) use var this pattern was enough to make the analyzers think that var could be used uninitialized, or initialized to NULL, in a setting where it must not be NULL; but once you see that it is merely a case of the analyzer getting it wrong (var is _only_ used under the same conditions where it was previously assigned earlier on), the solution is to add sa_assert() hints to the analyzers. NACK to this patch; we need to get to the real root of why the analyzers are complaining, and fix the real bug if there is one (but I didn't see one in my manual inspection), or more likely add sa_assert() hints to silence the analyzer. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/05/2011 03:47 AM, Eric Blake wrote:
On 08/04/2011 11:14 AM, Alex Jia wrote:
Also, how does checking for a non-zero union value prevent a null dereference?
To be honest, I'm not sure this, however, it's okay for ccc-analyzer if I add these judgements,
Is ccc-analyzer different from clang? If so, how can I set it up, to reproduce the problem you saw? It may be different, I will tidy up a docs or resource and then sending to you.
Regards, Alex
I previously saw a false positive in one of these functions (qemudDomainGetMemoryParameters) when using Coverity, but that was fixed by commit f768b4c3, but Coverity was silent for the other 4 functions you touched. I'm now trying to do a clang run to see if that differs from Coverity. The Coverity false positive was that our logic confused the static analyzers:
type var; // uninit if (flags & _CONFIG) var = something if (flags & _LIVE) do something else if (flags & _CONFIG) use var
this pattern was enough to make the analyzers think that var could be used uninitialized, or initialized to NULL, in a setting where it must not be NULL; but once you see that it is merely a case of the analyzer getting it wrong (var is _only_ used under the same conditions where it was previously assigned earlier on), the solution is to add sa_assert() hints to the analyzers.
NACK to this patch; we need to get to the real root of why the analyzers are complaining, and fix the real bug if there is one (but I didn't see one in my manual inspection), or more likely add sa_assert() hints to silence the analyzer.
Our logic throws off analyzer tools: ptr var = NULL; if (flags == 0) flags = live ? _LIVE : _CONFIG; if (flags & _LIVE) do stuff if (flags & _CONFIG) var = non-null; if (flags & _LIVE) do more stuff else if (flags & _CONFIG) use var the tools keep thinking that var can still be NULL in the last if clause, adding the hint shuts them up. * src/qemu/qemu_driver.c (qemuDomainSetBlkioParameters): Add a static analysis hint. --- Off-list, I confirmed with Alex that ccc-analyzer and clang are both built on the same llvm compiler front end, and thus give the same analysis. And I reproduced the static analyzer false positive; this is the proper fix. src/qemu/qemu_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..a8e4b78 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5679,6 +5679,9 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } } } else if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + /* Clang can't see that if we get here, persistentDef was set. */ + sa_assert(persistentDef); + for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; -- 1.7.4.4
On 08/05/2011 11:14 AM, Eric Blake wrote:
Our logic throws off analyzer tools:
ptr var = NULL; if (flags == 0) flags = live ? _LIVE : _CONFIG; if (flags& _LIVE) do stuff if (flags& _CONFIG) var = non-null; if (flags& _LIVE) do more stuff else if (flags& _CONFIG) use var
the tools keep thinking that var can still be NULL in the last if clause, adding the hint shuts them up.
* src/qemu/qemu_driver.c (qemuDomainSetBlkioParameters): Add a static analysis hint. ---
Off-list, I confirmed with Alex that ccc-analyzer and clang are both built on the same llvm compiler front end, and thus give the same analysis. And I reproduced the static analyzer false positive; this is the proper fix.
src/qemu/qemu_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce19be7..a8e4b78 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5679,6 +5679,9 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } } } else if (flags& VIR_DOMAIN_AFFECT_CONFIG) { + /* Clang can't see that if we get here, persistentDef was set. */ + sa_assert(persistentDef); + for (i = 0; i< nparams; i++) { virTypedParameterPtr param =¶ms[i];
Was this ever ACKed? (I don't see it). If not, then ACK. :-)
On 08/11/2011 08:51 AM, Laine Stump wrote:
On 08/05/2011 11:14 AM, Eric Blake wrote:
} } else if (flags& VIR_DOMAIN_AFFECT_CONFIG) { + /* Clang can't see that if we get here, persistentDef was set. */ + sa_assert(persistentDef); + for (i = 0; i< nparams; i++) { virTypedParameterPtr param =¶ms[i];
Was this ever ACKed? (I don't see it). If not, then ACK. :-)
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Laine Stump