diff mbox

[2/4] nwfilter: Fix potential locking problems on ObjLoad failure

Message ID db344097e23b4b84ef76a565da1f8bf63fa3a882.1461540078.git.crobinso@redhat.com
State Accepted
Commit ab05abdbc395a77bd21ea475f42e78e2c479ea60
Headers show

Commit Message

Cole Robinson April 24, 2016, 11:22 p.m. UTC 
In virNWFilterObjLoad we can still fail after virNWFilterObjAssignDef,
but we don't unlock and free the created virNWFilterObjPtr in the
cleanup path.

The bit we are trying to do after AssignDef is just STRDUP in the
configFile path. However caching the configFile in the NWFilterObj
is largely redundant and doesn't follow the same pattern we use
for domain and network objects.

So just remove all the configFile caching which fixes the latent
bug as a side effect.
---
 src/conf/nwfilter_conf.c       | 62 ++++++++++++++++++++----------------------
 src/conf/nwfilter_conf.h       |  5 ++--
 src/nwfilter/nwfilter_driver.c |  6 ++--
 3 files changed, 34 insertions(+), 39 deletions(-)

-- 
2.7.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index ced8eb8..d02bbff 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -353,8 +353,6 @@  virNWFilterObjFree(virNWFilterObjPtr obj)
     virNWFilterDefFree(obj->def);
     virNWFilterDefFree(obj->newDef);
 
-    VIR_FREE(obj->configFile);
-
     virMutexDestroy(&obj->lock);
 
     VIR_FREE(obj);
@@ -3181,12 +3179,6 @@  virNWFilterObjLoad(virNWFilterObjListPtr nwfilters,
         return NULL;
     }
 
-    VIR_FREE(nwfilter->configFile); /* for driver reload */
-    if (VIR_STRDUP(nwfilter->configFile, path) < 0) {
-        virNWFilterDefFree(def);
-        return NULL;
-    }
-
     return nwfilter;
 }
 
@@ -3234,60 +3226,66 @@  virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters,
 
 int
 virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver,
-                      virNWFilterObjPtr nwfilter,
                       virNWFilterDefPtr def)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     char *xml;
-    int ret;
-
-    if (!nwfilter->configFile) {
-        if (virFileMakePath(driver->configDir) < 0) {
-            virReportSystemError(errno,
-                                 _("cannot create config directory %s"),
-                                 driver->configDir);
-            return -1;
-        }
+    int ret = -1;
+    char *configFile = NULL;
 
-        if (!(nwfilter->configFile = virFileBuildPath(driver->configDir,
-                                                      def->name, ".xml"))) {
-            return -1;
-        }
+    if (virFileMakePath(driver->configDir) < 0) {
+        virReportSystemError(errno,
+                             _("cannot create config directory %s"),
+                             driver->configDir);
+        goto error;
+    }
+
+    if (!(configFile = virFileBuildPath(driver->configDir,
+                                        def->name, ".xml"))) {
+        goto error;
     }
 
     if (!(xml = virNWFilterDefFormat(def))) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        "%s", _("failed to generate XML"));
-        return -1;
+        goto error;
     }
 
     virUUIDFormat(def->uuid, uuidstr);
-    ret = virXMLSaveFile(nwfilter->configFile,
+    ret = virXMLSaveFile(configFile,
                          virXMLPickShellSafeComment(def->name, uuidstr),
                          "nwfilter-edit", xml);
     VIR_FREE(xml);
 
+ error:
+    VIR_FREE(configFile);
     return ret;
 }
 
 
 int
-virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter)
+virNWFilterObjDeleteDef(const char *configDir,
+                        virNWFilterObjPtr nwfilter)
 {
-    if (!nwfilter->configFile) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("no config file for %s"), nwfilter->def->name);
-        return -1;
+    int ret = -1;
+    char *configFile = NULL;
+
+    if (!(configFile = virFileBuildPath(configDir,
+                                        nwfilter->def->name, ".xml"))) {
+        goto error;
     }
 
-    if (unlink(nwfilter->configFile) < 0) {
+    if (unlink(configFile) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("cannot remove config for %s"),
                        nwfilter->def->name);
-        return -1;
+        goto error;
     }
 
-    return 0;
+    ret = 0;
+ error:
+    VIR_FREE(configFile);
+    return ret;
 }
 
 
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index 0211861..823cfa4 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -551,7 +551,6 @@  typedef virNWFilterObj *virNWFilterObjPtr;
 struct _virNWFilterObj {
     virMutex lock;
 
-    char *configFile;
     int active;
     int wantRemoved;
 
@@ -612,10 +611,10 @@  virNWFilterObjPtr virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters,
 
 
 int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver,
-                          virNWFilterObjPtr nwfilter,
                           virNWFilterDefPtr def);
 
-int virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter);
+int virNWFilterObjDeleteDef(const char *configDir,
+                            virNWFilterObjPtr nwfilter);
 
 virNWFilterObjPtr virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
                                           virNWFilterDefPtr def);
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 1a868b6..2828b28 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -554,7 +554,7 @@  nwfilterDefineXML(virConnectPtr conn,
     if (!(nwfilter = virNWFilterObjAssignDef(&driver->nwfilters, def)))
         goto cleanup;
 
-    if (virNWFilterObjSaveDef(driver, nwfilter, def) < 0) {
+    if (virNWFilterObjSaveDef(driver, def) < 0) {
         virNWFilterObjRemove(&driver->nwfilters, nwfilter);
         def = NULL;
         goto cleanup;
@@ -602,11 +602,9 @@  nwfilterUndefine(virNWFilterPtr obj)
         goto cleanup;
     }
 
-    if (virNWFilterObjDeleteDef(nwfilter) < 0)
+    if (virNWFilterObjDeleteDef(driver->configDir, nwfilter) < 0)
         goto cleanup;
 
-    VIR_FREE(nwfilter->configFile);
-
     virNWFilterObjRemove(&driver->nwfilters, nwfilter);
     nwfilter = NULL;
     ret = 0;