From 98ff9f1e77fea574f25003e87342a280001697d3 Mon Sep 17 00:00:00 2001
From: Aetf <aetf@unlimitedcodeworks.xyz>
Date: Wed, 11 Dec 2019 16:17:39 -0500
Subject: [PATCH] FdoSecrets: cleanup all connections when database is replaced
 due to locking, fix #4004

---
 src/fdosecrets/objects/Collection.cpp | 27 ++++++++++++++++++---------
 src/fdosecrets/objects/DBusReturn.h   | 12 ++++++++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp
index bce4579bd..82d3513b8 100644
--- a/src/fdosecrets/objects/Collection.cpp
+++ b/src/fdosecrets/objects/Collection.cpp
@@ -469,14 +469,19 @@ namespace FdoSecrets
 
         // Attach signal to update exposed group settings if the group was removed.
         //
-        // The lifetime of the connection is bound to the database object, because
-        // in Database::~Database, groups are also deleted as children, but we don't
-        // want to trigger this.
-        // This works because the fact that QObject disconnects signals BEFORE deleting
-        // children.
+        // When the group object is normally deleted due to ~Database, the databaseReplaced
+        // signal should be first emitted, and we will clean up connection in reloadDatabase,
+        // so this handler won't be triggered.
         QPointer<Database> db = m_backend->database().data();
-        connect(m_exposedGroup.data(), &Group::groupAboutToRemove, db, [db](Group* toBeRemoved) {
-            if (!db) {
+        connect(m_exposedGroup.data(), &Group::groupAboutToRemove, this, [this](Group* toBeRemoved) {
+            if (backendLocked()) {
+                return;
+            }
+            auto db = m_backend->database();
+            if (toBeRemoved->database() != db) {
+                // should not happen, but anyway.
+                // somehow our current database has been changed, and the old group is being deleted
+                // possibly logic changes in replaceDatabase.
                 return;
             }
             auto uuid = FdoSecrets::settings()->exposedGroup(db);
@@ -496,7 +501,7 @@ namespace FdoSecrets
 
         // Monitor exposed group settings
         connect(m_backend->database()->metadata()->customData(), &CustomData::customDataModified, this, [this]() {
-            if (!m_exposedGroup || !m_backend) {
+            if (!m_exposedGroup || backendLocked()) {
                 return;
             }
             if (m_exposedGroup->uuid() == FdoSecrets::settings()->exposedGroup(m_backend->database())) {
@@ -615,9 +620,13 @@ namespace FdoSecrets
 
     void Collection::cleanupConnections()
     {
+        m_backend->database()->metadata()->customData()->disconnect(this);
         if (m_exposedGroup) {
-            m_exposedGroup->disconnect(this);
+            for (const auto group : m_exposedGroup->groupsRecursive(true)) {
+                group->disconnect(this);
+            }
         }
+
         m_items.clear();
     }
 
diff --git a/src/fdosecrets/objects/DBusReturn.h b/src/fdosecrets/objects/DBusReturn.h
index 6c94eab18..889b8e11c 100644
--- a/src/fdosecrets/objects/DBusReturn.h
+++ b/src/fdosecrets/objects/DBusReturn.h
@@ -158,6 +158,12 @@ namespace FdoSecrets
             return std::move(m_value);
         }
 
+        /**
+         * Get value or handle the error by the passed in dbus object
+         * @tparam P
+         * @param p
+         * @return
+         */
         template <typename P> T valueOrHandle(P* p) const&
         {
             if (isError()) {
@@ -169,6 +175,12 @@ namespace FdoSecrets
             return m_value;
         }
 
+        /**
+         * Get value or handle the error by the passed in dbus object
+         * @tparam P
+         * @param p
+         * @return
+         */
         template <typename P> T&& valueOrHandle(P* p) &&
         {
             if (isError()) {