10:51 a.m.
* src/qemu/qemu_driver.c: avoid dereference of null pointer.
Signed-off-by: Alex Jia <ajia(a)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
11:18 a.m.
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:14 p.m.
On 08/05/2011 12:18 AM, Eric Blake wrote:
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(a)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?
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
2:47 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:11 p.m.
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.
10:14 a.m.
New subject: [libvirt] [PATCH] qemu: silence clang false positive
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
9:51 a.m.
New subject: [libvirt] [PATCH] qemu: silence clang false positive
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. :-)
2:06 p.m.
New subject: [libvirt] [PATCH] qemu: silence clang false positive
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4852
days inactive
4859
days old
7 comments
4 participants
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Laine Stump