[libvirt] [PATCH 0/6] More coverity issues
REVERSE_INULL, SIZEOF_MISMATCH and some uninitialized variables. Ján Tomko (6): conf: fix NULL check in virNetDevBandwidthParse util: fix virBitmap allocation in virProcessInfoGetAffinity virsh: use correct sizeof when allocating cpumap virsh: do timing even for unusable connections rpc: don't destroy xdr before creating it in virNetMessageEncodeHeader conf: fix uninitialized variable in virDomainListSnapshots src/conf/netdev_bandwidth_conf.c | 4 +++- src/conf/snapshot_conf.c | 2 +- src/rpc/virnetmessage.c | 2 +- src/util/processinfo.c | 2 +- tools/virsh-domain.c | 4 ++-- tools/virsh.c | 12 ++++++------ 6 files changed, 14 insertions(+), 12 deletions(-) -- 1.7.8.6
Found by coverity: Error: REVERSE_INULL (CWE-476): libvirt-0.10.2/src/conf/netdev_bandwidth_conf.c:99: deref_ptr: Directly dereferencing pointer "node". libvirt-0.10.2/src/conf/netdev_bandwidth_conf.c:107: check_after_deref: Null-checking "node" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. --- src/conf/netdev_bandwidth_conf.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 261718f..5802eba 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -96,7 +96,7 @@ virNetDevBandwidthPtr virNetDevBandwidthParse(xmlNodePtr node) { virNetDevBandwidthPtr def = NULL; - xmlNodePtr cur = node->children; + xmlNodePtr cur; xmlNodePtr in = NULL, out = NULL; if (VIR_ALLOC(def) < 0) { @@ -110,6 +110,8 @@ virNetDevBandwidthParse(xmlNodePtr node) goto error; } + cur = node->children; + while (cur) { if (cur->type == XML_ELEMENT_NODE) { if (xmlStrEqual(cur->name, BAD_CAST "inbound")) { -- 1.7.8.6
Found by coverity: Error: REVERSE_INULL (CWE-476): libvirt-0.10.2/src/conf/netdev_bandwidth_conf.c:99: deref_ptr: Directly dereferencing pointer "node". libvirt-0.10.2/src/conf/netdev_bandwidth_conf.c:107: check_after_deref: Null-checking "node" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. --- src/conf/netdev_bandwidth_conf.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
ACK.
Found by coverity: Error: REVERSE_INULL (CWE-476): libvirt-0.10.2/src/util/processinfo.c:141: deref_ptr: Directly dereferencing pointer "map". libvirt-0.10.2/src/util/processinfo.c:142: check_after_deref: Null-checking "map" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. --- src/util/processinfo.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b8c60eb..b1db049 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -140,7 +140,7 @@ realloc: } *map = virBitmapNew(maxcpu); - if (!map) { + if (!*map) { virReportOOMError(); return -1; } -- 1.7.8.6
Found by coverity: Error: REVERSE_INULL (CWE-476): libvirt-0.10.2/src/util/processinfo.c:141: deref_ptr: Directly dereferencing pointer "map". libvirt-0.10.2/src/util/processinfo.c:142: check_after_deref: Null-checking "map" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. --- src/util/processinfo.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK.
Found by coverity: Error: SIZEOF_MISMATCH (CWE-569): libvirt-0.10.2/tools/virsh-domain.c:4754: suspicious_sizeof: Passing argument "8UL /* sizeof (cpumap) */" to function "_vshCalloc(vshControl *, size_t, size_t, char const *, int)" and then casting the return value to "unsigned char *" is suspicious. Error: SIZEOF_MISMATCH (CWE-569): libvirt-0.10.2/tools/virsh-domain.c:4942: suspicious_sizeof: Passing argument "8UL /* sizeof (cpumap) */" to function "_vshCalloc(vshControl *, size_t, size_t, char const *, int)" and then casting the return value to "unsigned char *" is suspicious. --- tools/virsh-domain.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6d5a0ec..12da00d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4768,7 +4768,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) /* Pin mode: pinning specified vcpu to specified physical cpus*/ - cpumap = vshCalloc(ctl, cpumaplen, sizeof(cpumap)); + cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap)); /* Parse cpulist */ cur = cpulist; if (*cur == 0) { @@ -4954,7 +4954,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) /* Pin mode: pinning emulator threads to specified physical cpus*/ - cpumap = vshCalloc(ctl, cpumaplen, sizeof(cpumap)); + cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap)); /* Parse cpulist */ cur = cpulist; if (*cur == 0) { -- 1.7.8.6
----- Original Message -----
Found by coverity: Error: SIZEOF_MISMATCH (CWE-569): libvirt-0.10.2/tools/virsh-domain.c:4754: suspicious_sizeof: Passing argument "8UL /* sizeof (cpumap) */" to function "_vshCalloc(vshControl *, size_t, size_t, char const *, int)" and then casting the return value to "unsigned char *" is suspicious.
Error: SIZEOF_MISMATCH (CWE-569): libvirt-0.10.2/tools/virsh-domain.c:4942: suspicious_sizeof: Passing argument "8UL /* sizeof (cpumap) */" to function "_vshCalloc(vshControl *, size_t, size_t, char const *, int)" and then casting the return value to "unsigned char *" is suspicious. --- tools/virsh-domain.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
ACK.
- cpumap = vshCalloc(ctl, cpumaplen, sizeof(cpumap)); + cpumap = vshCalloc(ctl, cpumaplen, sizeof(*cpumap));
Bugs like this show why we prefer VIR_ALLOC_N. Maybe we should fix vshCalloc to be a thin wrapper around VIR_ALLOC_N, but that's a bigger task for another day.
Time values were uninitialized if the connection wasn't usable. --- tools/virsh.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 6372177..dea3f82 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1562,20 +1562,20 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd) !(cmd->def->flags & VSH_CMD_FLAG_NOCONNECT)) vshReconnect(ctl); + if (enable_timing) + GETTIMEOFDAY(&before); + if ((cmd->def->flags & VSH_CMD_FLAG_NOCONNECT) || vshConnectionUsability(ctl, ctl->conn)) { - if (enable_timing) - GETTIMEOFDAY(&before); - ret = cmd->def->handler(ctl, cmd); - - if (enable_timing) - GETTIMEOFDAY(&after); } else { /* connection is not usable, return error */ ret = false; } + if (enable_timing) + GETTIMEOFDAY(&after); + /* try to automatically catch disconnections */ if (!ret && ((last_error != NULL) && -- 1.7.8.6
On OOM, xdr_destroy got called on xdr even though it wasn't created yet. Found by coverity: Error: UNINIT (CWE-457): libvirt-0.10.2/src/rpc/virnetmessage.c:214: var_decl: Declaring variable "xdr" without initializer. libvirt-0.10.2/src/rpc/virnetmessage.c:219: cond_true: Condition "virReallocN(&msg->buffer, 1UL /* sizeof (*msg->buffer) */, msg->bufferLength) < 0", taking true branch libvirt-0.10.2/src/rpc/virnetmessage.c:221: goto: Jumping to label "cleanup" libvirt-0.10.2/src/rpc/virnetmessage.c:257: label: Reached label "cleanup" libvirt-0.10.2/src/rpc/virnetmessage.c:258: uninit_use: Using uninitialized value "xdr.x_ops". --- src/rpc/virnetmessage.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index ce5f9d8..2fbd603 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -218,7 +218,7 @@ int virNetMessageEncodeHeader(virNetMessagePtr msg) msg->bufferLength = VIR_NET_MESSAGE_MAX + VIR_NET_MESSAGE_LEN_MAX; if (VIR_REALLOC_N(msg->buffer, msg->bufferLength) < 0) { virReportOOMError(); - goto cleanup; + return ret; } msg->bufferOffset = 0; -- 1.7.8.6
On OOM, xdr_destroy got called on xdr even though it wasn't created yet.
Found by coverity: Error: UNINIT (CWE-457): libvirt-0.10.2/src/rpc/virnetmessage.c:214: var_decl: Declaring variable "xdr" without initializer. libvirt-0.10.2/src/rpc/virnetmessage.c:219: cond_true: Condition "virReallocN(&msg->buffer, 1UL /* sizeof (*msg->buffer) */, msg->bufferLength) < 0", taking true branch libvirt-0.10.2/src/rpc/virnetmessage.c:221: goto: Jumping to label "cleanup" libvirt-0.10.2/src/rpc/virnetmessage.c:257: label: Reached label "cleanup" libvirt-0.10.2/src/rpc/virnetmessage.c:258: uninit_use: Using uninitialized value "xdr.x_ops". --- src/rpc/virnetmessage.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK.
If allocation of names fails, list is uninitialized. --- src/conf/snapshot_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 06be34d..bba1bb7 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1021,7 +1021,7 @@ virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, unsigned int flags) { int count = virDomainSnapshotObjListNum(snapshots, from, flags); - virDomainSnapshotPtr *list; + virDomainSnapshotPtr *list = NULL; char **names; int ret = -1; int i; -- 1.7.8.6
If allocation of names fails, list is uninitialized. --- src/conf/snapshot_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK. And looks like this one was introduced fairly recently, by commit 0361917.
participants (2)
-
Eric Blake -
Ján Tomko