FdoSecrets: cleanup all connections when database is replaced due to …

…locking, fix #4004
keepassxreboot · Dec 13, 2019 · cca0e1d · cca0e1d
1 parent 9362c3e
commit cca0e1d
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 14 deletions.
diff --git 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) {
+        auto c = 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);
@@ -486,17 +491,19 @@ namespace FdoSecrets
                 FdoSecrets::settings()->setExposedGroup(db, {});
             }
         });
+        m_connections.append(c);
         // Another possibility is the group being moved to recycle bin.
-        connect(m_exposedGroup.data(), &Group::groupModified, this, [this]() {
+        c = connect(m_exposedGroup.data(), &Group::groupModified, this, [this]() {
             if (inRecycleBin(m_exposedGroup->parentGroup())) {
                 // reset the exposed group to none
                 FdoSecrets::settings()->setExposedGroup(m_backend->database().data(), {});
             }
         });
+        m_connections.append(c);
 
         // Monitor exposed group settings
-        connect(m_backend->database()->metadata()->customData(), &CustomData::customDataModified, this, [this]() {
-            if (!m_exposedGroup || !m_backend) {
+        c = connect(m_backend->database()->metadata()->customData(), &CustomData::customDataModified, this, [this]() {
+            if (!m_exposedGroup || backendLocked()) {
                 return;
             }
             if (m_exposedGroup->uuid() == FdoSecrets::settings()->exposedGroup(m_backend->database())) {
@@ -505,6 +512,7 @@ namespace FdoSecrets
             }
             onDatabaseExposedGroupChanged();
         });
+        m_connections.append(c);
 
         // Add items for existing entry
         const auto entries = m_exposedGroup->entriesRecursive(false);
@@ -565,8 +573,10 @@ namespace FdoSecrets
             return;
         }
 
-        connect(group, &Group::groupModified, this, &Collection::collectionChanged);
-        connect(group, &Group::entryAdded, this, [this](Entry* entry) { onEntryAdded(entry, true); });
+        auto c = connect(group, &Group::groupModified, this, &Collection::collectionChanged);
+        m_connections.append(c);
+        c = connect(group, &Group::entryAdded, this, [this](Entry* entry) { onEntryAdded(entry, true); });
+        m_connections.append(c);
 
         const auto children = group->children();
         for (const auto& cg : children) {
@@ -615,9 +625,10 @@ namespace FdoSecrets
 
     void Collection::cleanupConnections()
     {
-        if (m_exposedGroup) {
-            m_exposedGroup->disconnect(this);
+        while (!m_connections.isEmpty()) {
+            disconnect(m_connections.takeFirst());
         }
+
         m_items.clear();
     }
 

diff --git a/src/fdosecrets/objects/Collection.h b/src/fdosecrets/objects/Collection.h
@@ -152,6 +152,9 @@ namespace FdoSecrets
         QSet<QString> m_aliases;
         QList<Item*> m_items;
         QMap<const Entry*, Item*> m_entryToItem;
+        // any connections to/from the internal Database object
+        // must be saved so we can disconnect later
+        QList<QMetaObject::Connection> m_connections;
 
         bool m_registered;
     };

diff --git 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()) {