[PATCH v2 4/4] secret: Handle object list removal and deletion properly

[John Ferlan <jferlan@xxxxxxxxxx>]

Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
after the call to virSecretObjListRemove, be more explicit by calling
virObjectUnref and setting @obj to NULL for secretUndefine and in
the error path of secretDefineXML. Calling EndAPI will end up calling
Unlock on an already unlocked object which has indeteriminate results
(usually an ignored error).

The virSecretObjEndAPI will both Unref and Unlock the object; however,
the virSecretObjListRemove would have already Unlock'd the object so
calling Unlock again is incorrect. Once the virSecretObjListRemove
is called all that's left is to Unref our interest since that's the
corrollary to the virSecretObjListAdd which returned our ref interest
plus references for each hash table in which the object resides. In math
terms, after an Add there's 2 refs on the object (1 for the object and
1 for the list). After calling Remove there's just 1 ref on the object.
For the Add callers, calling EndAPI removes the ref for the object and
unlocks it, but since it's in a list the other 1 remains.

This also fixes a leak during virSecretLoad if the virSecretLoadValue
fails the code jumps to cleanup without setting @ret = obj, thus calling
virSecretObjListRemove which only accounts for the object reference
related to adding the object to the list during virSecretObjListAdd,
but does not account for the reference to the object itself as the
return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
on the object recently added thus reducing the refcnt to zero. Thus
cleaning up the virSecretLoadValue error path to make it clearer what
needs to be done on failure.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
 src/conf/virsecretobj.c    | 14 ++++++--------
 src/secret/secret_driver.c |  9 +++++++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index dd36ce6..0e7675e 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -914,7 +914,6 @@ virSecretLoad(virSecretObjListPtr secrets,
 {
     virSecretDefPtr def = NULL;
     virSecretObjPtr obj = NULL;
-    virSecretObjPtr ret = NULL;
 
     if (!(def = virSecretDefParseFile(path)))
         goto cleanup;
@@ -926,16 +925,15 @@ virSecretLoad(virSecretObjListPtr secrets,
         goto cleanup;
     def = NULL;
 
-    if (virSecretLoadValue(obj) < 0)
-        goto cleanup;
-
-    ret = obj;
-    obj = NULL;
+    if (virSecretLoadValue(obj) < 0) {
+        virSecretObjListRemove(secrets, obj);
+        virObjectUnref(obj);
+        obj = NULL;
+    }
 
  cleanup:
-    virSecretObjListRemove(secrets, obj);
     virSecretDefFree(def);
-    return ret;
+    return obj;
 }
 
 
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 77351d8..19ba6fd 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -267,10 +267,13 @@ secretDefineXML(virConnectPtr conn,
      * the backup. The current def will be Free'd below.
      * Otherwise, this is a new secret, thus remove it.
      */
-    if (backup)
+    if (backup) {
         virSecretObjSetDef(obj, backup);
-    else
+    } else {
         virSecretObjListRemove(driver->secrets, obj);
+        virObjectUnref(obj);
+        obj = NULL;
+    }
 
  cleanup:
     virSecretDefFree(def);
@@ -410,6 +413,8 @@ secretUndefine(virSecretPtr secret)
     virSecretObjDeleteData(obj);
 
     virSecretObjListRemove(driver->secrets, obj);
+    virObjectUnref(obj);
+    obj = NULL;
 
     ret = 0;
 
-- 
2.9.4

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