[libvirt] [PATCH 1/2] nwfilter: address coverity findings
This patch addresses the following coverity findings: /libvirt/src/conf/nwfilter_params.c:157: deref_parm: Directly dereferencing parameter "val". /libvirt/src/conf/nwfilter_params.c:473: negative_returns: Using variable "iterIndex" as an index to array "res->iter". /libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2891: unchecked_value: No check of the return value of "virAsprintf(&protostr, "-d 01:80:c2:00:00:00 ")". /libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2894: unchecked_value: No check of the return value of "virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr)". /libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:3590: var_deref_op: Dereferencing null variable "inst". --- src/conf/nwfilter_params.c | 5 ++++- src/nwfilter/nwfilter_ebiptables_driver.c | 10 +++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -154,6 +154,9 @@ virNWFilterVarValueGetNthValue(virNWFilt { const char *res = NULL; + if (!val) + return NULL; + switch (val->valType) { case NWFILTER_VALUE_TYPE_SIMPLE: if (idx == 0) @@ -467,7 +470,7 @@ virNWFilterVarCombIterCreate(virNWFilter res->nIter++; break; case VIR_NWFILTER_VAR_ACCESS_LAST: - break; + goto err_exit; } if (virNWFilterVarCombIterAddVariable(&res->iter[iterIndex], Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2878,6 +2878,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; char *protostr = NULL; + int r = 0; PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, @@ -2888,14 +2889,14 @@ ebtablesCreateTmpSubChain(ebiptablesRule protostr = strdup(""); break; case L2_PROTO_STP_IDX: - virAsprintf(&protostr, "-d " NWFILTER_MAC_BGA " "); + r = virAsprintf(&protostr, "-d " NWFILTER_MAC_BGA " "); break; default: - virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr); + r = virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr); break; } - if (!protostr) { + if (!protostr || r < 0) { virReportOOMError(); return -1; } @@ -3589,6 +3590,9 @@ ebiptablesApplyNewRules(const char *ifna int nEbtChains = 0; char *errmsg = NULL; + if (inst == NULL) + nruleInstances = 0; + if (!chains_in_set || !chains_out_set) { virReportOOMError(); goto exit_free_sets;
This patch addresses the following coverity findings: /libvirt/src/conf/nwfilter_params.c:390: var_assigned: Assigning: "varValue" = null return value from "virHashLookup". /libvirt/src/conf/nwfilter_params.c:392: dereference: Dereferencing a pointer that might be null "varValue" when calling "virNWFilterVarValueGetNthValue". /libvirt/src/conf/nwfilter_params.c:399: dereference: Dereferencing a pointer that might be null "tmp" when calling "virNWFilterVarValueGetNthValue". --- src/conf/nwfilter_params.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -30,7 +30,7 @@ #include "datatypes.h" #include "nwfilter_params.h" #include "domain_conf.h" - +#include "logging.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -391,14 +391,28 @@ virNWFilterVarCombIterEntryAreUniqueEntr const char *value; varValue = virHashLookup(hash->hashTable, cie->varNames[0]); + if (!varValue) { + /* caller's error */ + VIR_ERROR(_("%s: hash lookup resulted in NULL pointer"), __func__); + return true; + } value = virNWFilterVarValueGetNthValue(varValue, cie->curValue); + if (!value) { + VIR_ERROR(_("%s: Lookup of value at index %u resulted in a NULL " + "pointer"), __func__, cie->curValue); + return true; + } for (i = 0; i < cie->curValue; i++) { if (STREQ(value, virNWFilterVarValueGetNthValue(varValue, i))) { bool isSame = true; for (j = 1; j < cie->nVarNames; j++) { tmp = virHashLookup(hash->hashTable, cie->varNames[j]); + if (!tmp) { + /* should never occur to step on a NULL here */ + return true; + } if (!STREQ(virNWFilterVarValueGetNthValue(tmp, cie->curValue), virNWFilterVarValueGetNthValue(tmp, i))) { isSame = false;
On 04/26/2012 01:46 PM, Stefan Berger wrote:
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:390: var_assigned: Assigning: "varValue" = null return value from "virHashLookup".
/libvirt/src/conf/nwfilter_params.c:392: dereference: Dereferencing a pointer that might be null "varValue" when calling "virNWFilterVarValueGetNthValue".
/libvirt/src/conf/nwfilter_params.c:399: dereference: Dereferencing a pointer that might be null "tmp" when calling "virNWFilterVarValueGetNthValue".
--- src/conf/nwfilter_params.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
Nice to have tools that help us catch bugs.
@@ -391,14 +391,28 @@ virNWFilterVarCombIterEntryAreUniqueEntr const char *value;
varValue = virHashLookup(hash->hashTable, cie->varNames[0]); + if (!varValue) { + /* caller's error */ + VIR_ERROR(_("%s: hash lookup resulted in NULL pointer"), __func__);
VIR_ERROR already appends __func__ to the resulting message. This should be: VIR_ERROR("%s", _("hash lookup resulted in NULL pointer"));
value = virNWFilterVarValueGetNthValue(varValue, cie->curValue); + if (!value) { + VIR_ERROR(_("%s: Lookup of value at index %u resulted in a NULL " + "pointer"), __func__, cie->curValue);
And again, this should be: VIR_ERROR(_("Lookup of value at index %u resulted in a NULL pointer"), cie->curValue); ACK with those fixes; I'm okay if you push without posting a v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/26/2012 04:35 PM, Eric Blake wrote:
And again, this should be:
VIR_ERROR(_("Lookup of value at index %u resulted in a NULL pointer"), cie->curValue);
ACK with those fixes; I'm okay if you push without posting a v2.
Pushed this series.
On 04/26/2012 01:17 PM, Stefan Berger wrote:
This patch addresses the following coverity findings:
/libvirt/src/conf/nwfilter_params.c:157: deref_parm: Directly dereferencing parameter "val".
/libvirt/src/conf/nwfilter_params.c:473: negative_returns: Using variable "iterIndex" as an index to array "res->iter".
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2891: unchecked_value: No check of the return value of "virAsprintf(&protostr, "-d 01:80:c2:00:00:00 ")".
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:2894: unchecked_value: No check of the return value of "virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr)".
/libvirt/src/nwfilter/nwfilter_ebiptables_driver.c:3590: var_deref_op: Dereferencing null variable "inst".
--- src/conf/nwfilter_params.c | 5 ++++- src/nwfilter/nwfilter_ebiptables_driver.c | 10 +++++++--- 2 files changed, 11 insertions(+), 4 deletions(-)
Nice little grab-bag of patches.
if (virNWFilterVarCombIterAddVariable(&res->iter[iterIndex], Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2878,6 +2878,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; char *protostr = NULL; + int r = 0;
No need for r.
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, @@ -2888,14 +2889,14 @@ ebtablesCreateTmpSubChain(ebiptablesRule protostr = strdup(""); break; case L2_PROTO_STP_IDX: - virAsprintf(&protostr, "-d " NWFILTER_MAC_BGA " "); + r = virAsprintf(&protostr, "-d " NWFILTER_MAC_BGA " ");
Here, I'd just write: ignore_value(virAsprintf(...));
break; default: - virAsprintf(&protostr, "-p 0x%04x ", l3_protocols[protoidx].attr); + r = virAsprintf(&protostr, "-p 0x%04x ",
and here,
l3_protocols[protoidx].attr); break; }
- if (!protostr) {
because virAsprintf guarantees that if it returned < 0, then protostr is NULL and you have an OOM situation. In other words, our code is already correct, and we just need the ignore_value() wrapper to shut up the static analyzer. The other hunks were right. ACK with the cleanup mentioned, I'm okay if you push without posting a v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Stefan Berger