[libvirt] [PATCH 0/7] Fixes to various different issues
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
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@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
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@redhat.com> --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
Yet another case of not needing virGetLastError processing Signed-off-by: John Ferlan <jferlan@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
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@redhat.com> --- tests/securityselinuxlabeltest.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
ACK
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@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
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@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
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@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
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@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
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@redhat.com> --- src/util/virlog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
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@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
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@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
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@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
Since it's only used in loop - just go direct. Signed-off-by: John Ferlan <jferlan@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
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@redhat.com> ---
ACK in this case, but we need to be careful.
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@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
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@redhat.com> --- tests/qemumonitorjsontest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
participants (2)
-
John Ferlan -
Peter Krempa