Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Secret service crash when searching with null baseGroup #5279

Closed
droidmonkey opened this issue Aug 16, 2020 · 10 comments · Fixed by #5660
Closed

Secret service crash when searching with null baseGroup #5279

droidmonkey opened this issue Aug 16, 2020 · 10 comments · Fixed by #5660
Assignees
@Aetf
Labels
crash 💥 feature: Secret Service more info needed
Milestone
v2.7.0

Comments

@droidmonkey
Copy link
Member

I experience the same on fresh install on Arch Linux. From gdb, I got:

ASSERT: "baseGroup" in file /home/invidian/repos/keepassxc/src/core/EntrySearcher.cpp, line 41

#0  0x00007ffff62a2355 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff628b853 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff6a429ac in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/libQt5Core.so.5
#3  0x00007ffff6a41d59 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#4  0x0000555555792888 in EntrySearcher::search (this=0x7fffffffd5a0, searchTerms=..., baseGroup=0x0, forceSearch=false) at /home/invidian/repos/keepassxc/src/core/EntrySearcher.cpp:41
#5  0x00005555557fd8a9 in FdoSecrets::Collection::searchItems (this=0x555558571320, attributes=...) at /home/invidian/repos/keepassxc/src/fdosecrets/objects/Collection.cpp:254
#6  0x00005555557f20ca in FdoSecrets::Service::searchItems (this=0x55555667f3f0, attributes=..., locked=...) at /home/invidian/repos/keepassxc/src/fdosecrets/objects/Service.cpp:267
#7  0x000055555580ae7c in FdoSecrets::ServiceAdaptor::SearchItems (this=0x55555667f730, attributes=..., locked=...) at /home/invidian/repos/keepassxc/src/fdosecrets/objects/adaptors/ServiceAdaptor.cpp:71
#8  0x00005555557de07a in FdoSecrets::ServiceAdaptor::qt_static_metacall (_o=0x55555667f730, _c=QMetaObject::InvokeMetaMethod, _id=5, _a=0x7fffffffd8c0)
    at /home/invidian/repos/keepassxc/build/src/fdosecrets/fdosecrets_autogen/ZPMS5EPCTP/moc_ServiceAdaptor.cpp:155
#9  0x00005555557de76b in FdoSecrets::ServiceAdaptor::qt_metacall (this=0x55555667f730, _c=QMetaObject::InvokeMetaMethod, _id=5, _a=0x7fffffffd8c0)
    at /home/invidian/repos/keepassxc/build/src/fdosecrets/fdosecrets_autogen/ZPMS5EPCTP/moc_ServiceAdaptor.cpp:313
#10 0x00007ffff6f25fd6 in ?? () from /usr/lib/libQt5DBus.so.5
#11 0x00007ffff6f295cb in ?? () from /usr/lib/libQt5DBus.so.5
#12 0x00007ffff6f29eba in ?? () from /usr/lib/libQt5DBus.so.5
#13 0x00007ffff6f2c388 in ?? () from /usr/lib/libQt5DBus.so.5
#14 0x00007ffff6c911d2 in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#15 0x00007ffff77ee702 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#16 0x00007ffff6c647ba in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/libQt5Core.so.5
#17 0x00007ffff6c672a3 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/libQt5Core.so.5
#18 0x00007ffff6cbdcf4 in ?? () from /usr/lib/libQt5Core.so.5
#19 0x00007ffff544a43c in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#20 0x00007ffff54981d9 in ?? () from /usr/lib/libglib-2.0.so.0
#21 0x00007ffff5449221 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#22 0x00007ffff6cbd331 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#23 0x00007ffff6c6313c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#24 0x00007ffff6c6b5c4 in QCoreApplication::exec() () from /usr/lib/libQt5Core.so.5
#25 0x00005555555c5919 in main (argc=1, argv=0x7fffffffe258) at /home/invidian/repos/keepassxc/src/main.cpp:160

When trying to execute the following command:

secret-tool lookup UserName invidian

Version: 71b05db.

Originally posted by @invidian in #5199 (comment)
@droidmonkey droidmonkey added this to the v2.6.2 milestone Aug 16, 2020
@Aetf
Copy link
Contributor

Aetf commented Sep 25, 2020

I took a brief look: m_exposedGroup should never be nullptr as long as the Collection object exists. This is done by connecting to the following signals:

  • groupAboutToRemove signal to reset the exposed group in db metadata
  • groupModified signal if moved to recycle bin to reset the exposed group in db metadata
  • db metadata change signal to repopulate the db
  • the group object is deleted in the db object destructor. This should be handled by databaseReplaced and the databaseLocked signal.

Now given the assert failure, something must go wrong with these. Maybe the group was deleted in another situation and not handled by the above. So it's set to nullptr by the QPointer mechanism.

I'm short on time at the moment so can't investigate further.

@droidmonkey droidmonkey linked a pull request Oct 13, 2020 that will close this issue
Final push for 2.6.2 #5542
Merged
@droidmonkey droidmonkey removed a link to a pull request Oct 13, 2020
Final push for 2.6.2 #5542
Merged
@droidmonkey droidmonkey modified the milestones: v2.6.2, v2.7.0 Oct 13, 2020
@Aetf
Copy link
Contributor

Aetf commented Nov 2, 2020

While the stacktrace shows the exact place crashed, it's still unclear to me why that would happen... I tried to reproduce the crash using the latest build of the develop branch with debug build but had no success.

@invidian Could you give more details about how the crash happened? Like the exact steps you did to cause the crash, and what are the settings of the secret service plugin in both app settings and database settings.

@Aetf Aetf self-assigned this Nov 2, 2020
@invidian
Copy link

invidian commented Nov 2, 2020

@Aetf I can still reproduce it with v2.6.2. Steps to reproduce:

  • Enable SSI
  • Close KeepassXC
  • Open KeepassXC
  • Open database
  • Run secret-tool lookup UserName invidian

When running keepassxc from the console, I can see the following logs:

$ keepassxc
QDBusConnection: Could not emit signal org.freedesktop.Secret.Service.CollectionCreated: Marshalling failed: Invalid object path passed in arguments
QDBusConnection: error: could not send reply message to service "": Marshalling failed: Invalid object path passed in arguments
QDBusConnection: error: could not send reply message to service "": Marshalling failed: Invalid object path passed in arguments
QDBusConnection: Could not emit signal org.freedesktop.Secret.Service.CollectionChanged: Marshalling failed: Invalid object path passed in arguments
QDBusConnection: error: could not send reply message to service "": Marshalling failed: Invalid object path passed in arguments
Segmentation fault

I guess the OS may make a difference here? I run XFCE on Arch Linux updated to latest version.

@Aetf
Copy link
Contributor

Aetf commented Nov 3, 2020

Ok, so there were some errors when creating the collection dbus object, probably due to invalid paths used when registering the object according to the log. As of now, the errors are ignored in the release build. That's why the app crashes later because the internal state is messed up due to the error.

So two actionable here:

  • properly handle the dbus path registration error
  • figure out why the path is invalid in the first place. The encoding process must be missing a corner case.

What's your database file's file name and the database name? The registration path is built using an encoded version of these.

@invidian
Copy link

invidian commented Nov 3, 2020

What's your database file's file name and the database name? The registration path is built using an encoded version of these.

It's .NewDatabase.kdbx and .NewDatabase respectively, as it was auto-generated years ago by KeePass2 😄

@Aetf
Copy link
Contributor

Aetf commented Nov 3, 2020

Ha, a leading dot! Technically everything after the dot becomes part of the extension, not the base name. Thus the derived registration path is empty.

Working on a fix now.

@invidian
Copy link

invidian commented Nov 3, 2020 •
edited
Loading

Ha, a leading dot! Technically everything after the dot becomes part of the extension, not the base name. Thus the derived registration path is empty.

Working on a fix now.

It all make sense! Sorry I didn't provide more details in the first place.

If anyone else is affected by this, it seems renaming database file name makes it work, I can look up entries now!

As a side note, when I renamed the file and re-opened the database, UI still shows old database name, even though I also changed it in database settings:
Selection_269

I wonder if that can be changed somehow too.

@Aetf
Copy link
Contributor

Aetf commented Nov 3, 2020

UI still shows old database name

That's the root group's name. You can right-click and change it.

Too many names involved :D

@invidian
Copy link

invidian commented Nov 3, 2020

That's the root group's name. You can right-click and change it.

🤦 Right, I didn't expect that 😄 Changed now and works like charm! Thanks a lot @Aetf! 🙏 🎉

Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
9ed65d7 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
a23d68a 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
@Aetf Aetf mentioned this issue Nov 4, 2020
Merged
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
4c20b3b 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
92d728b 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
df84cdb 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 4, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
9efe04e 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 12, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
5574f90 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
@echozio
Copy link

echozio commented Nov 12, 2020

Ha, a leading dot! Technically everything after the dot becomes part of the extension, not the base name. Thus the derived registration path is empty.

Working on a fix now.

My db file was called .kdbx, this has been driving me crazy, thanks for a workaround until the fix is in the repos 😂

Aetf added a commit to Aetf/keepassxc that referenced this issue Nov 13, 2020
@Aetf
FdoSecrets: handle corner cases in collection dbus names, fix keepass…
a651d70 
…xreboot#5279

- Use completeBaseName rather than baseName to ensure nonempty name
- Handle two databases have the same name
- Cleanup Service::onDatabaseTabOpened logic
@Aetf Aetf mentioned this issue Jan 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Aetf Aetf

Labels
crash 💥 feature: Secret Service more info needed
Projects
None yet
Milestone
v2.7.0
Development

Successfully merging a pull request may close this issue.

Secret Service: cleanup and fix crash Aetf/keepassxc
4 participants
@Aetf @droidmonkey @echozio @invidian