10:42 a.m.
All found by Coverity....
Not being so diligent lately... There are a few more, but they're all
false positives so I'll keep those local.
John Ferlan (7):
conf: Remove incorrect check when encoding shmem audit message
tests: Prefer virGetLastErrorMessage in testSELinuxLabeling
qemu: Remove possibility of NULL dereference
util: Resolve memory leaks in virLogParse{Output|Filter}
vsh: Fix some issues in auto completion code
util: Remove need for local 'nelems'
tests: Need to initialize data
src/conf/domain_audit.c | 4 ++--
src/qemu/qemu_capabilities.c | 7 +++++--
src/util/virlog.c | 4 ++--
src/util/virqemu.c | 3 +--
tests/qemumonitorjsontest.c | 2 +-
tests/securityselinuxlabeltest.c | 6 ++----
tools/vsh.c | 7 ++++---
7 files changed, 17 insertions(+), 16 deletions(-)
--
2.7.4
10:42 a.m.
New subject: [libvirt] [PATCH 1/7] conf: Remove incorrect check when encoding shmem audit message
Remove the !size check since size is initialized to NULL and thus
causing the condition to always be true
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_audit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index fd20ace..2423f34 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -983,7 +983,7 @@ virDomainAuditShmem(virDomainObjPtr vm,
virUUIDFormat(vm->def->uuid, uuidstr);
- if (!vmname || !src || !size || !shmem ||
+ if (!vmname || !src || !shmem ||
virAsprintfQuiet(&size, "%llu", def->size) < 0) {
VIR_WARN("OOM while encoding audit message");
goto cleanup;
@@ -997,7 +997,7 @@ virDomainAuditShmem(virDomainObjPtr vm,
VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
"virt=%s resrc=shmem reason=%s %s uuid=%s size=%s %s %s",
- virt, reason, vmname, uuidstr, size ?: "?", shmem, src);
+ virt, reason, vmname, uuidstr, size, shmem, src);
cleanup:
VIR_FREE(vmname);
--
2.7.4
11:21 a.m.
New subject: [libvirt] [PATCH 1/7] conf: Remove incorrect check when encoding shmem audit message
On Mon, Oct 10, 2016 at 11:42:12 -0400, John Ferlan wrote:
Remove the !size check since size is initialized to NULL and thus
causing the condition to always be true
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_audit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
ACK
10:42 a.m.
New subject: [libvirt] [PATCH 2/7] tests: Prefer virGetLastErrorMessage in testSELinuxLabeling
Yet another case of not needing virGetLastError processing
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/securityselinuxlabeltest.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index e98d9bb..1178acf 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -331,10 +331,8 @@ testSELinuxLabeling(const void *opaque)
VIR_FREE(files[i].context);
}
VIR_FREE(files);
- if (ret < 0) {
- virErrorPtr err = virGetLastError();
- VIR_TEST_VERBOSE("%s\n", err ? err->message :
"<unknown>");
- }
+ if (ret < 0)
+ VIR_TEST_VERBOSE("%s\n", virGetLastErrorMessage());
return ret;
}
--
2.7.4
11:20 a.m.
New subject: [libvirt] [PATCH 2/7] tests: Prefer virGetLastErrorMessage in testSELinuxLabeling
On Mon, Oct 10, 2016 at 11:42:13 -0400, John Ferlan wrote:
Yet another case of not needing virGetLastError processing
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/securityselinuxlabeltest.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
ACK
10:42 a.m.
New subject: [libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference
If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps and
dereferencing to get the nmachineTypes will cause a core. Rework the code
slightly to avoid the issue and return immediately if !qemubinCaps or
!nmachineTypes
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index da8f3d1..ee3e50f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
size_t i;
*machines = NULL;
+ *nmachines = 0;
+
+ if (!qemuCaps || !qemuCaps->nmachineTypes)
+ return 0;
*nmachines = qemuCaps->nmachineTypes;
- if (*nmachines &&
- VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
+ if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
goto error;
for (i = 0; i < qemuCaps->nmachineTypes; i++) {
--
2.7.4
11:19 a.m.
New subject: [libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference
On Mon, Oct 10, 2016 at 11:42:14 -0400, John Ferlan wrote:
If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps
and
^^^^^
This symbol name refers to the name in the caller. The function uses a
different name.
dereferencing to get the nmachineTypes will cause a core. Rework the
code
It will cause a crash, not a core. A core dump is caused by the crash
and can be turned off.
slightly to avoid the issue and return immediately if !qemubinCaps
or
!nmachineTypes
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index da8f3d1..ee3e50f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
size_t i;
*machines = NULL;
+ *nmachines = 0;
+
+ if (!qemuCaps || !qemuCaps->nmachineTypes)
+ return 0;
*nmachines = qemuCaps->nmachineTypes;
- if (*nmachines &&
- VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
+ if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
goto error;
This is a false positive, the caller checks that 'binary' is set and
does not call this function at all. Coverity probably doesn't see it as
it depends on the state of 'binary' rather than 'qemubinCaps' in the
caller.
As of such I disagree with this fix. The only acceptable way is to make
the caller better introspectable. This fix obscures what is actually
happening.
Peter
2:26 p.m.
New subject: [libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference
On 10/10/2016 12:19 PM, Peter Krempa wrote:
On Mon, Oct 10, 2016 at 11:42:14 -0400, John Ferlan wrote:
> If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps and
^^^^^
This symbol name refers to the name in the caller. The function uses a
different name.
> dereferencing to get the nmachineTypes will cause a core. Rework the code
It will cause a crash, not a core. A core dump is caused by the crash
and can be turned off.
Ok - semantics...
> slightly to avoid the issue and return immediately if
!qemubinCaps or
> !nmachineTypes
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_capabilities.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index da8f3d1..ee3e50f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
> size_t i;
>
> *machines = NULL;
> + *nmachines = 0;
> +
> + if (!qemuCaps || !qemuCaps->nmachineTypes)
> + return 0;
> *nmachines = qemuCaps->nmachineTypes;
>
> - if (*nmachines &&
> - VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
> + if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
> goto error;
This is a false positive, the caller checks that 'binary' is set and
does not call this function at all. Coverity probably doesn't see it as
it depends on the state of 'binary' rather than 'qemubinCaps' in the
caller.
Hmm... true. Coverity generally has an issue with similar conditions
where there's two arguments that are related and only one is checked
while the other is used. This one's not so obvious. Interesting that the
subsequent call wasn't flagged, but that's perhaps because it's preceded
by "kvmbin &&".
In any case, I've dropped this one and tagged it in my personal coverity
branch as a false positive.
John
10:42 a.m.
New subject: [libvirt] [PATCH 4/7] util: Resolve memory leaks in virLogParse{Output|Filter}
In both virLogParseOutput and virLogParseFilter, rather than returning
NULL, goto cleanup since it's possible that for each the first condition
passes, but the || condition doesn't and thus we leak memory.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virlog.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 14ee701..52b0eea 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1466,7 +1466,7 @@ virLogParseOutput(const char *src)
if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count <
2) {
virReportError(VIR_ERR_INVALID_ARG,
_("Malformed format for output '%s'"), src);
- return NULL;
+ goto cleanup;
}
if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 ||
@@ -1575,7 +1575,7 @@ virLogParseFilter(const char *src)
if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count != 2)
{
virReportError(VIR_ERR_INVALID_ARG,
_("Malformed format for filter '%s'"), src);
- return NULL;
+ goto cleanup;
}
if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 ||
--
2.7.4
11:05 a.m.
New subject: [libvirt] [PATCH 4/7] util: Resolve memory leaks in virLogParse{Output|Filter}
On Mon, Oct 10, 2016 at 11:42:15 -0400, John Ferlan wrote:
In both virLogParseOutput and virLogParseFilter, rather than
returning
NULL, goto cleanup since it's possible that for each the first condition
passes, but the || condition doesn't and thus we leak memory.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virlog.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
ACK
10:42 a.m.
New subject: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code
1. Move the declaration of const vshCmdDef *help - it should be at the
top of the "if" rather than in the middle.
2. Change a comparison from && to || - without doing so we could core
on commands like 'virsh list' which would allow completion of some
non -- option based on whatever was found in the current working
directory and then as soon as that was completed, the next <tab>
would core since "opt" would be returned as NULL, but the check
was dereferencing "&& opt->type"
3. Before dereferencing opt->completer, be sure opt isn't NULL.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/vsh.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 420eb79..9558dad 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
/* if we encountered --help, replace parsed command with
* 'help <cmdname>' */
for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
+ const vshCmdDef *help;
if (STRNEQ(tmpopt->def->name, "help"))
continue;
- const vshCmdDef *help = vshCmddefSearch("help");
+ help = vshCmddefSearch("help");
vshCommandOptFree(first);
first = vshMalloc(ctl, sizeof(vshCmdOpt));
first->def = help->opts;
@@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
* Try to find the default option.
*/
if (!(opt = vshCmddefGetData(cmd, &opts_need_arg, &opts_seen))
- && opt->type == VSH_OT_BOOL)
+ || opt->type == VSH_OT_BOOL)
goto error;
opt_exists = true;
opts_need_arg = const_opts_need_arg;
@@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
res = vshReadlineCommandGenerator(sanitized_text, state);
} else if (opts_filled && !non_bool_opt_exists) {
res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
- } else if (non_bool_opt_exists && data_complete && opt->completer)
{
+ } else if (non_bool_opt_exists && data_complete && opt &&
opt->completer) {
if (!completed_list)
completed_list = opt->completer(autoCompleteOpaque,
opt->completer_flags);
--
2.7.4
11:33 a.m.
New subject: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code
On Mon, Oct 10, 2016 at 11:42:16 -0400, John Ferlan wrote:
1. Move the declaration of const vshCmdDef *help - it should be at
the
top of the "if" rather than in the middle.
2. Change a comparison from && to || - without doing so we could core
Same comment as in 3/7 on 'core' not being the appropriate term.
on commands like 'virsh list' which would allow completion
of some
non -- option based on whatever was found in the current working
directory and then as soon as that was completed, the next <tab>
would core since "opt" would be returned as NULL, but the check
aaand again.
was dereferencing "&& opt->type"
3. Before dereferencing opt->completer, be sure opt isn't NULL.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/vsh.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 420eb79..9558dad 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
/* if we encountered --help, replace parsed command with
* 'help <cmdname>' */
for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
+ const vshCmdDef *help;
if (STRNEQ(tmpopt->def->name, "help"))
continue;
- const vshCmdDef *help = vshCmddefSearch("help");
+ help = vshCmddefSearch("help");
vshCommandOptFree(first);
first = vshMalloc(ctl, sizeof(vshCmdOpt));
first->def = help->opts;
@@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
* Try to find the default option.
*/
if (!(opt = vshCmddefGetData(cmd, &opts_need_arg, &opts_seen))
- && opt->type == VSH_OT_BOOL)
+ || opt->type == VSH_OT_BOOL)
goto error;
opt_exists = true;
opts_need_arg = const_opts_need_arg;
@@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
res = vshReadlineCommandGenerator(sanitized_text, state);
} else if (opts_filled && !non_bool_opt_exists) {
res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
- } else if (non_bool_opt_exists && data_complete &&
opt->completer) {
+ } else if (non_bool_opt_exists && data_complete && opt &&
opt->completer) {
if (!completed_list)
completed_list = opt->completer(autoCompleteOpaque,
opt->completer_flags);
This function makes my eyes bleed.
ACK
2:35 p.m.
New subject: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code
On 10/10/2016 12:33 PM, Peter Krempa wrote:
On Mon, Oct 10, 2016 at 11:42:16 -0400, John Ferlan wrote:
> 1. Move the declaration of const vshCmdDef *help - it should be at the
> top of the "if" rather than in the middle.
>
> 2. Change a comparison from && to || - without doing so we could core
Same comment as in 3/7 on 'core' not being the appropriate term.
> on commands like 'virsh list' which would allow completion of some
> non -- option based on whatever was found in the current working
> directory and then as soon as that was completed, the next <tab>
> would core since "opt" would be returned as NULL, but the check
aaand again.
OK changed them to crash.
> was dereferencing "&& opt->type"
>
> 3. Before dereferencing opt->completer, be sure opt isn't NULL.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> tools/vsh.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 420eb79..9558dad 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser)
> /* if we encountered --help, replace parsed command with
> * 'help <cmdname>' */
> for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
> + const vshCmdDef *help;
> if (STRNEQ(tmpopt->def->name, "help"))
> continue;
>
> - const vshCmdDef *help = vshCmddefSearch("help");
> + help = vshCmddefSearch("help");
> vshCommandOptFree(first);
> first = vshMalloc(ctl, sizeof(vshCmdOpt));
> first->def = help->opts;
> @@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
> * Try to find the default option.
> */
> if (!(opt = vshCmddefGetData(cmd, &opts_need_arg,
&opts_seen))
> - && opt->type == VSH_OT_BOOL)
> + || opt->type == VSH_OT_BOOL)
> goto error;
> opt_exists = true;
> opts_need_arg = const_opts_need_arg;
> @@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
> res = vshReadlineCommandGenerator(sanitized_text, state);
> } else if (opts_filled && !non_bool_opt_exists) {
> res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
> - } else if (non_bool_opt_exists && data_complete &&
opt->completer) {
> + } else if (non_bool_opt_exists && data_complete && opt
&& opt->completer) {
> if (!completed_list)
> completed_list = opt->completer(autoCompleteOpaque,
> opt->completer_flags);
This function makes my eyes bleed.
Yep - couple shots of tequila or whatever that Borovicka stuff is we had
at KVM Forum was could help ;-)
John
10:42 a.m.
New subject: [libvirt] [PATCH 6/7] util: Remove need for local 'nelems'
Since it's only used in loop - just go direct.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virqemu.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 0b516fc..7849d8b 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -85,12 +85,11 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key,
virBufferPtr buf)
{
const virJSONValue *member;
- ssize_t nelems = virJSONValueArraySize(array);
char *prefix = NULL;
size_t i;
int ret = 0;
- for (i = 0; i < nelems; i++) {
+ for (i = 0; i < virJSONValueArraySize(array); i++) {
member = virJSONValueArrayGet((virJSONValuePtr) array, i);
if (virAsprintf(&prefix, "%s.%zu", key, i) < 0)
--
2.7.4
11:03 a.m.
New subject: [libvirt] [PATCH 6/7] util: Remove need for local 'nelems'
On Mon, Oct 10, 2016 at 11:42:17 -0400, John Ferlan wrote:
Since it's only used in loop - just go direct.
Not sure how deep coverity sees stuff, but in some cases this would
increase the complexity of the loop from O(n) to O(n^2) since the
function gets evaluated on every single loop, but the returned value
never changes.
In this case it's okay, since virJSONValueArraySize does not do any
iteration over the list.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
ACK in this case, but we need to be careful.
10:42 a.m.
New subject: [libvirt] [PATCH 7/7] tests: Need to initialize data
If not initialized and the virAsprintf to jsonreply or fulllablel fails,
then the call to qemuMonitorTestFree will take stack data.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/qemumonitorjsontest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0574f8c..993a373 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -753,7 +753,7 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt,
bool fail)
{
- struct qemuMonitorJSONTestAttachChardevData data;
+ struct qemuMonitorJSONTestAttachChardevData data = {0};
char *jsonreply = NULL;
char *fulllabel = NULL;
int ret = -1;
--
2.7.4
11:04 a.m.
New subject: [libvirt] [PATCH 7/7] tests: Need to initialize data
On Mon, Oct 10, 2016 at 11:42:18 -0400, John Ferlan wrote:
If not initialized and the virAsprintf to jsonreply or fulllablel
fails,
then the call to qemuMonitorTestFree will take stack data.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/qemumonitorjsontest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
2969
days inactive
2969
days old
16 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa