[libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer
Jim Meyering
jim at meyering.net
Mon May 17 20:51:21 UTC 2010
Jim Meyering wrote:
> Jim Meyering wrote:
>> Eric Blake wrote:
>>> On 05/17/2010 11:22 AM, Jim Meyering wrote:
>>>> This addresses another coverity-spotted "flaw".
>>>> However, since "cgroup" is never NULL after that initial "if" stmt,
>>>> the only penalty is that the useless cleanup test would make a reviewer
>>>> try to figure out how cgroup could be NULL there.
>>>
>>> ACK.
>>
>> Thanks.
>>
>>>> cleanup:
>>>> - if (cgroup)
>>>> - virCgroupFree(&cgroup);
>>>> + virCgroupFree(&cgroup);
>>>
>>> Is this something that the useless-if-before-free test could have
>>> caught, rather than waiting for coverity?
>>
>> Very good idea.
>> Here's a patch to do that.
>> With this, "make sc_avoid_if_before_free" (part of make syntax-check)
>> will now prevent any new useless checks.
>> The above was the sole offender.
>
> Actually, this is just the tip of the iceberg.
> A couple minutes work have shown that there are many more:
>
> Running this shows there are potentially at least 100
> candidate functions:
>
> aid free|grep '^vi'
>
> Looking at the first few on the list, I've found that most are
> indeed free-like:
>
> N virBufferFreeAndReset
> y virCPUDefFree
> Y virCapabilitiesFree
> y virCapabilitiesFreeGuest
> y virCapabilitiesFreeGuestDomain
> y virCapabilitiesFreeGuestFeature
> y virCapabilitiesFreeGuestMachine
> y virCapabilitiesFreeHostNUMACell
> y virCapabilitiesFreeMachines
> N virCapabilitiesFreeNUMAInfo (should probably fix)
> y virCgroupFree
> N virConfFree (diagnoses the "error")
> y virConfFreeList
> y virConfFreeValue
>
> So far, most of the exceptions can be "fixed"
> to also be free-like.
>
> Using those few additions uncovered these:
>
> src/conf/storage_conf.c: if (pool->newDef)
> virStoragePoolDefFree(pool->newDef)
> src/security/virt-aa-helper.c: if (ctl->caps)
> virCapabilitiesFree(ctl->caps)
> src/util/conf.c: if (cur->value) {
> virConfFreeValue(cur->value);
> }
Here's a complete patch:
>From 59d7134bc925de86b807262896a76256b889c96a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Mon, 17 May 2010 22:38:59 +0200
Subject: [PATCH] maint: add more free-like functions to the list and deal with fallout
* cfg.mk (useless_free_options): Add many vir*Free* function names,
and then remove the useless if-before-free tests exposed by running
make syntax-check.
* src/conf/interface_conf.c (virInterfaceDefFree): Remove useless "if".
(virInterfaceAssignDef): Likewise.
* src/conf/network_conf.c (virNetworkAssignDef): Likewise.
* src/conf/storage_conf.c (virStoragePoolObjAssignDef): Likewise.
* src/node_device/node_device_hal.c (dev_create): Likewise.
* src/security/virt-aa-helper.c (vahDeinit): Likewise.
* src/test/test_driver.c (testNodeDeviceCreateXML): Likewise.
* src/util/conf.c (virConfSetValue): Likewise.
---
cfg.mk | 163 +++++++++++++++++++++++++++++++++++--
src/conf/interface_conf.c | 13 +--
src/conf/network_conf.c | 3 +-
src/conf/storage_conf.c | 3 +-
src/node_device/node_device_hal.c | 3 +-
src/security/virt-aa-helper.c | 3 +-
src/test/test_driver.c | 3 +-
src/util/conf.c | 4 +-
8 files changed, 167 insertions(+), 28 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 0b281a5..96d6953 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -64,15 +64,164 @@ local-checks-to-skip = \
sc_makefile_check \
sc_useless_cpp_parens
-useless_free_options = \
- --name=sexpr_free \
- --name=virCgroupFree \
- --name=VIR_FREE \
- --name=xmlFree \
- --name=xmlXPathFreeContext \
- --name=virDomainDefFree \
+useless_free_options = \
+ --name=VIR_FREE \
+ --name=sexpr_free \
+ --name=virCPUDefFree \
+ --name=virCapabilitiesFree \
+ --name=virCapabilitiesFreeGuest \
+ --name=virCapabilitiesFreeGuestDomain \
+ --name=virCapabilitiesFreeGuestFeature \
+ --name=virCapabilitiesFreeGuestMachine \
+ --name=virCapabilitiesFreeHostNUMACell \
+ --name=virCapabilitiesFreeMachines \
+ --name=virCgroupFree \
+ --name=virConfFreeList \
+ --name=virConfFreeValue \
+ --name=virDomainChrDefFree \
+ --name=virDomainControllerDefFree \
+ --name=virDomainDefFree \
+ --name=virDomainDeviceDefFree \
+ --name=virDomainDiskDefFree \
+ --name=virDomainEventCallbackListFree \
+ --name=virDomainEventFree \
+ --name=virDomainEventQueueFree \
+ --name=virDomainFSDefFree \
+ --name=virDomainGraphicsDefFree \
+ --name=virDomainHostdevDefFree \
+ --name=virDomainInputDefFree \
+ --name=virDomainNetDefFree \
+ --name=virDomainObjFree \
+ --name=virDomainSnapshotDefFree \
+ --name=virDomainSnapshotObjFree \
+ --name=virDomainSoundDefFree \
+ --name=virDomainVideoDefFree \
+ --name=virDomainWatchdogDefFree \
+ --name=virInterfaceDefFree \
+ --name=virInterfaceIpDefFree \
+ --name=virInterfaceObjFree \
+ --name=virInterfaceProtocolDefFree \
+ --name=virJSONValueFree \
+ --name=virLastErrFreeData \
+ --name=virNWFilterDefFree \
+ --name=virNWFilterEntryFree \
+ --name=virNWFilterHashTableFree \
+ --name=virNWFilterIPAddrLearnReqFree \
+ --name=virNWFilterIncludeDefFree \
+ --name=virNWFilterPoolObjFree \
+ --name=virNWFilterRuleDefFree \
+ --name=virNWFilterRuleInstFree \
+ --name=virNetworkDefFree \
+ --name=virNetworkObjFree \
+ --name=virNodeDeviceDefFree \
+ --name=virNodeDeviceObjFree \
+ --name=virSecretDefFree \
+ --name=virStorageEncryptionFree \
+ --name=virStorageEncryptionSecretFree \
+ --name=virStoragePoolDefFree \
+ --name=virStoragePoolObjFree \
+ --name=virStoragePoolSourceFree \
+ --name=virStorageVolDefFree \
+ --name=xmlFree \
+ --name=xmlXPathFreeContext \
--name=xmlXPathFreeObject
+# The following template was generated by this command:
+# make ID && aid free|grep '^vi'|sed 's/ .*//;s/^/# /'
+# N virBufferFreeAndReset
+# y virCPUDefFree
+# y virCapabilitiesFree
+# y virCapabilitiesFreeGuest
+# y virCapabilitiesFreeGuestDomain
+# y virCapabilitiesFreeGuestFeature
+# y virCapabilitiesFreeGuestMachine
+# y virCapabilitiesFreeHostNUMACell
+# y virCapabilitiesFreeMachines
+# N virCapabilitiesFreeNUMAInfo FIXME
+# y virCgroupFree
+# N virConfFree (diagnoses the "error")
+# y virConfFreeList
+# y virConfFreeValue
+# y virDomainChrDefFree
+# y virDomainControllerDefFree
+# y virDomainDefFree
+# y virDomainDeviceDefFree
+# y virDomainDiskDefFree
+# y virDomainEventCallbackListFree
+# y virDomainEventFree
+# y virDomainEventQueueFree
+# y virDomainFSDefFree
+# n virDomainFree
+# n virDomainFreeName (can't fix -- returns int)
+# y virDomainGraphicsDefFree
+# y virDomainHostdevDefFree
+# y virDomainInputDefFree
+# y virDomainNetDefFree
+# y virDomainObjFree
+# y virDomainSnapshotDefFree
+# n virDomainSnapshotFree (returns int)
+# n virDomainSnapshotFreeName (returns int)
+# y virDomainSnapshotObjFree
+# y virDomainSoundDefFree
+# y virDomainVideoDefFree
+# y virDomainWatchdogDefFree
+# n virDrvNodeGetCellsFreeMemory (returns int)
+# n virDrvNodeGetFreeMemory (returns long long)
+# n virFree (dereferences param)
+# n virFreeError
+# n virHashFree (takes 2 args)
+# y virInterfaceDefFree
+# n virInterfaceFree (returns int)
+# n virInterfaceFreeName
+# y virInterfaceIpDefFree
+# y virInterfaceObjFree
+# n virInterfaceObjListFree
+# y virInterfaceProtocolDefFree
+# y virJSONValueFree
+# y virLastErrFreeData
+# y virNWFilterDefFree
+# y virNWFilterEntryFree
+# n virNWFilterFree (returns int)
+# y virNWFilterHashTableFree
+# y virNWFilterIPAddrLearnReqFree
+# y virNWFilterIncludeDefFree
+# n virNWFilterPoolFreeName (returns int)
+# y virNWFilterPoolObjFree
+# n virNWFilterPoolObjListFree FIXME
+# y virNWFilterRuleDefFree
+# n virNWFilterRuleFreeInstanceData (typedef)
+# y virNWFilterRuleInstFree
+# y virNetworkDefFree
+# n virNetworkFree (returns int)
+# n virNetworkFreeName (returns int)
+# y virNetworkObjFree
+# n virNetworkObjListFree FIXME
+# n virNodeDevCapsDefFree FIXME
+# y virNodeDeviceDefFree
+# n virNodeDeviceFree (returns int)
+# y virNodeDeviceObjFree
+# n virNodeDeviceObjListFree FIXME
+# n virNodeGetCellsFreeMemory (returns int)
+# n virNodeGetFreeMemory (returns non-void)
+# y virSecretDefFree
+# n virSecretFree (returns non-void)
+# n virSecretFreeName (2 args)
+# n virSecurityLabelDefFree FIXME
+# n virStorageBackendDiskMakeFreeExtent (returns non-void)
+# y virStorageEncryptionFree
+# y virStorageEncryptionSecretFree
+# n virStorageFreeType (enum)
+# y virStoragePoolDefFree
+# n virStoragePoolFree (returns non-void)
+# n virStoragePoolFreeName (returns non-void)
+# y virStoragePoolObjFree
+# n virStoragePoolObjListFree FIXME
+# y virStoragePoolSourceFree
+# y virStorageVolDefFree
+# n virStorageVolFree (returns non-void)
+# n virStorageVolFreeName (returns non-void)
+# n virStreamFree
+
# Avoid uses of write(2). Either switch to streams (fwrite), or use
# the safewrite wrapper.
sc_avoid_write:
diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c
index 920b090..6430f7a 100644
--- a/src/conf/interface_conf.c
+++ b/src/conf/interface_conf.c
@@ -85,20 +85,18 @@ void virInterfaceDefFree(virInterfaceDefPtr def)
switch (def->type) {
case VIR_INTERFACE_TYPE_BRIDGE:
for (i = 0;i < def->data.bridge.nbItf;i++) {
- if (def->data.bridge.itf[i] != NULL)
- virInterfaceDefFree(def->data.bridge.itf[i]);
- else
+ if (def->data.bridge.itf[i] == NULL)
break; /* to cope with half parsed data on errors */
+ virInterfaceDefFree(def->data.bridge.itf[i]);
}
VIR_FREE(def->data.bridge.itf);
break;
case VIR_INTERFACE_TYPE_BOND:
VIR_FREE(def->data.bond.target);
for (i = 0;i < def->data.bond.nbItf;i++) {
- if (def->data.bond.itf[i] != NULL)
- virInterfaceDefFree(def->data.bond.itf[i]);
- else
+ if (def->data.bond.itf[i] == NULL)
break; /* to cope with half parsed data on errors */
+ virInterfaceDefFree(def->data.bond.itf[i]);
}
VIR_FREE(def->data.bond.itf);
break;
@@ -1227,8 +1225,7 @@ virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces,
virInterfaceObjPtr iface;
if ((iface = virInterfaceFindByName(interfaces, def->name))) {
- if (iface->def)
- virInterfaceDefFree(iface->def);
+ virInterfaceDefFree(iface->def);
iface->def = def;
return iface;
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 1f3a44c..06537d1 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -157,8 +157,7 @@ virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
virNetworkDefFree(network->def);
network->def = def;
} else {
- if (network->newDef)
- virNetworkDefFree(network->newDef);
+ virNetworkDefFree(network->newDef);
network->newDef = def;
}
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 6218e02..778b909 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1332,8 +1332,7 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
virStoragePoolDefFree(pool->def);
pool->def = def;
} else {
- if (pool->newDef)
- virStoragePoolDefFree(pool->newDef);
+ virStoragePoolDefFree(pool->newDef);
pool->newDef = def;
}
return pool;
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
index 0174794..8113c55 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -486,8 +486,7 @@ static void dev_create(const char *udi)
DEBUG("FAILED TO ADD dev %s", name);
cleanup:
VIR_FREE(privData);
- if (def)
- virNodeDeviceDefFree(def);
+ virNodeDeviceDefFree(def);
nodeDeviceUnlock(driverState);
}
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index ae923e8..88cdc9d 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -63,8 +63,7 @@ vahDeinit(vahControl * ctl)
return -1;
VIR_FREE(ctl->def);
- if (ctl->caps)
- virCapabilitiesFree(ctl->caps);
+ virCapabilitiesFree(ctl->caps);
VIR_FREE(ctl->files);
VIR_FREE(ctl->hvm);
VIR_FREE(ctl->arch);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6706cba..395c8c9 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4983,8 +4983,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
def = NULL;
cleanup:
testDriverUnlock(driver);
- if (def)
- virNodeDeviceDefFree(def);
+ virNodeDeviceDefFree(def);
VIR_FREE(wwnn);
VIR_FREE(wwpn);
return dev;
diff --git a/src/util/conf.c b/src/util/conf.c
index 38eb163..8682f7b 100644
--- a/src/util/conf.c
+++ b/src/util/conf.c
@@ -902,9 +902,7 @@ virConfSetValue (virConfPtr conf,
conf->entries = cur;
}
} else {
- if (cur->value) {
- virConfFreeValue(cur->value);
- }
+ virConfFreeValue(cur->value);
cur->value = value;
}
return (0);
--
1.7.1.250.g7d1e8
More information about the libvir-list
mailing list