Re: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer

Jim Meyering wrote: Here's a complete patch: From: Jim Meyering <meyering(a)redhat.com&gt; 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