[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]