Skip to content

Commit dce1059

Browse files
ddidderrcodexclaude
committed
fix(fdosecrets): handle Secret Service requests during database load
A SearchItems call arriving while a database is still being opened crashed the application. Reading the KDBX4 payload spins a nested event loop, and Service::onDatabaseTabOpened has inserted the new Collection into m_collections before Collection::reloadBackend has finished populating it. In that window the backend database is not locked but m_exposedGroup is still null, so Collection::searchItems dereferences a null group via the UUID/path fast paths or EntrySearcher (Group::groupsRecursive with this == nullptr). The half-loaded state was also observable as inconsistent state to clients: the Locked property reported false while SearchItems returned empty, which a client interprets as "the entry isn't there" and behaves accordingly. Introduce collectionLocked() as the predicate the public DBus API uses (Locked, ensureUnlocked, label, SearchItems, and the collectionLockChanged signal payload). It is true when the database is locked or while m_exposedGroup is not yet established, so during the populate window clients consistently see Locked=true and SearchItems empty rather than the inconsistent mix. backendLocked() keeps its prior meaning (database locked, uninitialised, or absent) and is used by the bootstrap path in reloadBackend and by signal-handler guards that run only when m_exposedGroup is already set. cleanupConnections() now also clears m_exposedGroup and m_entryToItem. Without that, after a database reload the collection would still appear searchable through stale signal wiring, and the UUID/path shortcuts in searchItems would consult a map pointing at removed Item objects. KeePassXC has an "unlock before search" setting that prompts the user to unlock a database whenever SearchItems finds no unlocked collection. With the consistency fix above, a half-loaded collection counts as locked, so the populate window now also satisfies that "no unlocked collection" condition. The prompt would then open for a database already on its way to being unlocked, with no actually-locked database to offer - an empty dialog the user cannot act on. Service::searchItems therefore also requires that at least one DatabaseWidget actually be locked before opening the prompt. During the populate window SearchItems returns empty results; populate completes on its own and a follow-up call from the client returns real data. Test Plan: - Build and run testfdosecrets and testentrysearcher with WITH_XC_FDOSECRETS=ON and WITH_TESTS=ON. - Manual: with unlockBeforeSearch=on and an AutoOpen child database, SearchItems calls during the load window no longer crash, no empty unlock dialog appears, and a retry once populate completes returns real data. - Original crash locally reproduced with gdb --args keepassxc. Co-Authored-By: Codex GPT-5.5 (xhigh) <noreply@openai.com> Co-Authored-By: Claude Opus 4.7 (1M context) (xhigh) <noreply@anthropic.com>
1 parent 7c7ca45 commit dce1059

3 files changed

Lines changed: 35 additions & 8 deletions

File tree

src/fdosecrets/objects/Collection.cpp

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ namespace FdoSecrets
7979
return false;
8080
}
8181

82-
// populate contents after expose on dbus, because items rely on parent's dbus object path
82+
// populate contents after expose on dbus, because items rely on parent's dbus object path.
8383
if (!backendLocked()) {
8484
populateContents();
8585
} else {
@@ -107,7 +107,7 @@ namespace FdoSecrets
107107

108108
DBusResult Collection::ensureUnlocked() const
109109
{
110-
if (backendLocked()) {
110+
if (collectionLocked()) {
111111
return DBusResult(DBUS_ERROR_SECRET_IS_LOCKED);
112112
}
113113
return {};
@@ -130,7 +130,7 @@ namespace FdoSecrets
130130
return ret;
131131
}
132132

133-
if (backendLocked()) {
133+
if (collectionLocked()) {
134134
label = name();
135135
} else {
136136
label = m_backend->database()->metadata()->name();
@@ -159,7 +159,7 @@ namespace FdoSecrets
159159
if (ret.err()) {
160160
return ret;
161161
}
162-
locked = backendLocked();
162+
locked = collectionLocked();
163163
return {};
164164
}
165165

@@ -219,9 +219,9 @@ namespace FdoSecrets
219219
return ret;
220220
}
221221

222-
if (backendLocked()) {
222+
if (collectionLocked()) {
223223
// searchItems should work, whether `this` is locked or not.
224-
// however, we can't search items the same way as in gnome-keying,
224+
// However, we can't search items the same way as in gnome-keyring,
225225
// because there's no database at all when locked.
226226
return {};
227227
}
@@ -395,7 +395,7 @@ namespace FdoSecrets
395395
removeFromDBus();
396396
return;
397397
}
398-
emit collectionLockChanged(backendLocked());
398+
emit collectionLockChanged(collectionLocked());
399399
}
400400

401401
void Collection::populateContents()
@@ -606,7 +606,9 @@ namespace FdoSecrets
606606
}
607607
}
608608

609+
m_exposedGroup = nullptr;
609610
m_items.clear();
611+
m_entryToItem.clear();
610612
}
611613

612614
QString Collection::backendFilePath() const
@@ -621,9 +623,19 @@ namespace FdoSecrets
621623

622624
bool Collection::backendLocked() const
623625
{
626+
// True when the underlying database is locked, uninitialised, or absent.
624627
return !m_backend || !m_backend->database()->isInitialized() || m_backend->isLocked();
625628
}
626629

630+
bool Collection::collectionLocked() const
631+
{
632+
// True when the collection appears locked to DBus clients.
633+
// The collection appears locked when the database is locked, and also while
634+
// m_exposedGroup is null - between the databaseUnlocked signal and the end of
635+
// populateContents().
636+
return backendLocked() || !m_exposedGroup;
637+
}
638+
627639
bool Collection::doDeleteEntry(Entry* entry)
628640
{
629641
// Confirm entry removal before moving forward

src/fdosecrets/objects/Collection.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ namespace FdoSecrets
145145
void cleanupConnections();
146146

147147
bool backendLocked() const;
148+
bool collectionLocked() const;
148149

149150
/**
150151
* Check if the backend is a valid object, send error reply if not.

src/fdosecrets/objects/Service.cpp

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,21 @@ namespace FdoSecrets
275275
return ret;
276276
}
277277

278-
while (unlockedColls.isEmpty() && settings()->unlockBeforeSearch()) {
278+
auto anyBackendLocked = [this]() {
279+
for (const auto& coll : asConst(m_collections)) {
280+
if (auto* db = coll->backend(); db && db->isLocked()) {
281+
return true;
282+
}
283+
}
284+
return false;
285+
};
286+
287+
// unlockedColls.isEmpty() can be true even when no DatabaseWidget is
288+
// locked: a collection that is still being populated counts as locked,
289+
// while its database is already on its way to being unlocked. Don't
290+
// open the unlock dialog in that case - populate finishes on its own
291+
// and the client's next call returns real data.
292+
while (unlockedColls.isEmpty() && settings()->unlockBeforeSearch() && anyBackendLocked()) {
279293
// enable compatibility mode by making sure at least one database is unlocked
280294
QEventLoop loop;
281295
bool wasAccepted = false;

0 commit comments

Comments
 (0)