fix(fdosecrets): handle Secret Service requests during database load#13321
fix(fdosecrets): handle Secret Service requests during database load#13321ddidderr wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the FDO Secrets integration where SearchItems could be invoked during a short-lived collection initialization window (backend unlocked, but exposed group not yet populated), leading to null dereference during search.
Changes:
- Treat collections without an exposed group as not searchable (return empty results), matching the “locked collection” behavior.
- Strengthen teardown/reload cleanup by clearing
m_exposedGroupandm_entryToItemincleanupConnections()to avoid stale state during transitions.
| if (backendLocked() || !m_exposedGroup) { | ||
| // searchItems should work, whether `this` is locked or not. | ||
| // however, we can't search items the same way as in gnome-keying, | ||
| // because there's no database at all when locked. | ||
| // However, we can't search items the same way as in gnome-keyring, | ||
| // because there may be no database or exposed group available yet. | ||
| return {}; |
|
If this is a short-lived state, it may be better to wait for it to sync. "Exposed groups" are KPXC internal implementation detail that the public API doesn't care about. The API needs to always show a collection list that's consistent with the locked/unlocked state. Otherwise it can break client expectations. |
Possible, but in my opinion the current fix can still land as is because from a pure secret service client's perspective it is consistent behaviour. This is already a crazy edge case that I only noticed because I was fast enough to open a terminal and type an alias that uses secret-tool under the hood, while my database was still unlocking a 2nd AutoOpen db. The case you make sounds like a client that somehow gets an event (or polls) when KeePassXC is unlocked and immediately runs a secret-tool command... Theoretically possible, but unlikely. And again, could be improved later, this still fixes a real segfault.
Right and its never violated, or is it? A client sees:
This is perfectly fine from a client's perspective.
See my first paragraph above. |
|
Might be better to place the null check in the |
|
I see now, a secret service client could see the collection as unlocked but still get an empty result for the search which would be inconsistent. I'll try to improve this tomorrow. |
Most client apps talk to Secret Service via DBus, so as soon as they get a DBus reply, they can immediately send the next request. This is typically inside a single c++ function.
The 2nd step is the problematic one. It should be either
Exactly. This can cause a client to misbehave, because it would think the thing it's looking for just isn't there. |
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>
ddidderr
changed the title
|
PR is updated: see the new commit message. The split: Tested with two databases, DB1 normal and DB2 auto-opened by DB1:
Net effect: now stays true until my_collection and its auto-opened children have all finished populating. Before this PR it flipped to false as soon as my_collection's own backend was unlocked, even while the children were still loading, which is the inconsistency the PR addresses. What's your opinion? |
|
Update LGTM, though not sure about the unlock prompt in the middle of unlocking. If the exposed group is in the DB that's already unlocking, then I suppose it would be redundant. Also depends if it shows the partly-unlocked DB as one of the options, and whether trying to unlock (again) in that state could cause some problems. But I think that can be left to a follow-up PR.
That's because the child database is fully locked, so |
DB1 |
|
Ok, so actually DB2 is irrelevant to Secret Service in that case (unless it also has an exposed group), and I guess it just extends that "half locked" state. But even without DB2, the fix here makes things more robust from the public API point of view. Yea, so if the exposed group is in DB1, then from the user perspective, that unlock dialog isn't needed if that DB is already unlocking. And it might cause other problems too, in that state. But I'd leave that to a separate PR, as it might need more extensive refactoring to fix. |
varjolintu
added
the
pr: ai-assisted
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:
WITH_XC_FDOSECRETS=ON and WITH_TESTS=ON.
SearchItems calls during the load window no longer crash, no empty
unlock dialog appears, and a retry once populate completes returns
real data.
Co-Authored-By: Codex GPT-5.5 (xhigh) noreply@openai.com
Co-Authored-By: Claude Opus 4.7 (1M context) (xhigh) noreply@anthropic.com
AI: The debugging with gdb was done by Codex, the initial fix done by Codex, the improvement of the PR was done together with Claude.
Testing strategy
I tested (and reproduced) this manually:
secret-tool lookup label "label-of-your-test-entry"b) (with fix) secret-tool immediately fails to look up the entry as long as test2 is being unlocked. Once everything is unlocked, the secret-tool lookup works as expected.
Type of change
GDB Logs
This was a 2nd debugging run for show by Codex so you can see what was happening under the hood:
fdosecrets-search-crash-gdb.log