Skip to content

Fix double warning display for database open#744

Closed
kortschak wants to merge 3 commits intokeepassxreboot:developfrom
kortschak:hotfix/741-fix-not-found-warning
Closed

Fix double warning display for database open#744
kortschak wants to merge 3 commits intokeepassxreboot:developfrom
kortschak:hotfix/741-fix-not-found-warning

Conversation

@kortschak
Copy link
Copy Markdown

@kortschak kortschak commented Jul 4, 2017

This change fixes the rapid display and covering of the file not found warning dialogue described in #741.

Description

gui/DatabaseOpenWidget.cpp looks like DatabaseOpenWidget::openDatabase accepts the invalid CompositeKey returned by DatabaseWidget::databaseKey on failure.

This change short circuits the call to file.open in the case that an empty masterKey has been returned to allows the user to see the reason for that failure.

Motivation and context

Fixes #741.

How has this been tested?

This has been manually tested using the steps to reproduce at the issue.

  1. Create a database with a password and key file.
  2. Attempt to open the database with a bad path to the key file.

Screenshots (if appropriate):

Before change:
unhelpful-warning

After change:
correct-warning

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Note that -DWITH_ASAN=ON tests fail with

The following tests FAILED:
	  1 - testgroup (Failed)
	  5 - testkeepass2writer (Failed)
	  9 - testsymmetriccipher (Failed)
	 19 - testcsvparser (Failed)
Errors while running CTest

However this is identical to the behaviour seen with the current develop HEAD 7580c38.

phoerious
phoerious requested changes Jul 4, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, it's possible to have a database with an empty key. This patch would prevent those databases from being opened. I would therefore prefer a different implementation (maybe exceptions?)

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 4, 2017

Maybe a valid key should not be returned by DatabaseWidget::databaseKey on failure.

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 4, 2017

Abandoning.

@kortschak kortschak closed this Jul 4, 2017
@phoerious
Copy link
Copy Markdown
Member

phoerious commented Jul 5, 2017

Maybe the key could also have a property that says if it is a valid key or not.

BTW you don't need to abandon this PR. You can just force push anything else over it.

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017

If keeping a state field for validity is acceptable, I'll reopen; using exceptions for this seem obscene to me.

@kortschak kortschak reopened this Jul 5, 2017
@phoerious
Copy link
Copy Markdown
Member

phoerious commented Jul 5, 2017

I don't see why that would be obscene. You can't return a nullptr, because the instance is returned by value. Using exceptions would be the cleanest option. But I'd also be fine with an extra attribute.

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017

Just different aesthetics.

@phoerious
Copy link
Copy Markdown
Member

phoerious commented Jul 5, 2017

C++ programmers seem to never use exceptions, although they are one of the best language features compared to C.

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017

Please take another look.

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017

I have checked that this does not block opening an empty-keyed database by creating a new database without completing the creation dialogue - this leaves the database file created. This database can be opened.

@kortschak kortschak force-pushed the hotfix/741-fix-not-found-warning branch from 86745f1 to 09113ad Compare July 5, 2017 09:27
@kortschak
Address review comments
ac2933d 
SQUASH BEFORE MERGE
@kortschak kortschak force-pushed the hotfix/741-fix-not-found-warning branch 2 times, most recently from ac2933d to b8a62ac Compare July 5, 2017 09:41
@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017

It is not clear to me how the change here is causing the failure in the gui test (looking at the test and trying to replay it manually, I'm not actually sure what it's trying to do). I can reproduce the failure locally with CONFIG=Debug with gcc, though it does not help me all that much. Any suggestions welcome.

ASAN:SIGSEGV
=================================================================
==13869==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000078 (pc 0x55afc5515779 bp 0x7ffc984bc020 sp 0x7ffc984bc010 T0)
    #0 0x55afc5515778 in DatabaseWidget::switchToOpenMergeDatabase(QString const&) /home/dan/proj/keepassxc/src/gui/DatabaseWidget.cpp:921
    #1 0x55afc55060bf in DatabaseTabWidget::mergeDatabase(QString const&) /home/dan/proj/keepassxc/src/gui/DatabaseTabWidget.cpp:244
    #2 0x55afc5505ff8 in DatabaseTabWidget::mergeDatabase() /home/dan/proj/keepassxc/src/gui/DatabaseTabWidget.cpp:238
    #3 0x55afc546b80e in DatabaseTabWidget::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/dan/proj/keepassxc/build/src/moc_DatabaseTabWidget.cpp:228
    #4 0x7fd883ad4d29 in QMetaObject::activate(QObject*, int, int, void**) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b4d29)
    #5 0x7fd884908411 in QAction::triggered(bool) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x151411)
    #6 0x7fd88490a897 in QAction::activate(QAction::ActionEvent) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x153897)
    #7 0x7fd884a8ce51  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x2d5e51)
    #8 0x7fd884a930eb  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x2dc0eb)
    #9 0x7fd884a9705f in QMenu::mouseReleaseEvent(QMouseEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x2e005f)
    #10 0x7fd884954fc7 in QWidget::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x19dfc7)
    #11 0x7fd884a97ab2 in QMenu::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x2e0ab2)
    #12 0x7fd88491205b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15b05b)
    #13 0x7fd884917c18 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x160c18)
    #14 0x7fd883aa638a in QCoreApplication::notifyInternal(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28638a)
    #15 0x7fd884916b31 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15fb31)
    #16 0x7fd88496f91c  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1b891c)
    #17 0x7fd884971b7a  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1bab7a)
    #18 0x7fd88491205b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15b05b)
    #19 0x7fd884917515 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x160515)
    #20 0x7fd883aa638a in QCoreApplication::notifyInternal(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28638a)
    #21 0x7fd8843614e0 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Gui.so.5+0xf24e0)
    #22 0x7fd8843631a4 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Gui.so.5+0xf41a4)
    #23 0x7fd884346f07 in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Gui.so.5+0xd7f07)
    #24 0x7fd87a3821ff  (/usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5+0x6b1ff)
    #25 0x7fd881a33196 in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a196)
    #26 0x7fd881a333ef  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a3ef)
    #27 0x7fd881a3349b in g_main_context_iteration (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a49b)
    #28 0x7fd883afc7ce in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2dc7ce)
    #29 0x7fd883aa3b49 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x283b49)
    #30 0x7fd883aabbeb in QCoreApplication::exec() (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28bbeb)
    #31 0x55afc540846f in main /home/dan/proj/keepassxc/src/main.cpp:155
    #32 0x7fd882deb82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #33 0x55afc54073d8 in _start (/home/dan/proj/keepassxc/build/src/keepassxc+0xa23d8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/dan/proj/keepassxc/src/gui/DatabaseWidget.cpp:921 DatabaseWidget::switchToOpenMergeDatabase(QString const&)
==13869==ABORTING

@phoerious
Copy link
Copy Markdown
Member

phoerious commented Jul 5, 2017 •
edited
Loading

You should properly initialize the member variable using an initialization list in both constructors. You should also call it m_isValid or m_valid, not invalid (although that won't be the reason for the error).

@kortschak
Copy link
Copy Markdown
Author

kortschak commented Jul 5, 2017 •
edited
Loading

You should properly initialize the member variable using an initialization list in both constructors. You should also call it m_isValid or m_valid, not invalid (although that won't be the reason for the error).

Happy to change the name and orientation. I tried an explicit initialisation in ad875402c5577c024f87c4d353c21aa558ed9f7c, but it made no difference - similarly with the logic inversion here.

@phoerious
Copy link
Copy Markdown
Member

phoerious commented Jul 5, 2017

That's just a style thing. The compiler will default-initialize the boolean otherwise.
I'm also not quite sure what causes the error, though.

@TheZ3ro TheZ3ro added this to the v2.3.0 milestone Sep 19, 2017
@louib
Copy link
Copy Markdown
Member

louib commented Sep 19, 2017

@phoerious exceptions are disabled for the project, using the -fno-exceptions flag. Not sure what's the reason for this.

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Sep 19, 2017

Exceptions in c++ are terrible (also not implemented past the basic exception was thrown). Using try/catch programming leads to sloppy choices in architecture in my opinion.

@louib louib mentioned this pull request Sep 26, 2017
Closed
@louib louib mentioned this pull request Oct 5, 2017
Merged
@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Oct 7, 2017

Closing in favor of #1037

@TheZ3ro TheZ3ro closed this Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phoerious phoerious phoerious requested changes

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.3.0

Development

Successfully merging this pull request may close these issues.

File not found warning immediately replaced when key file not found

5 participants

@kortschak @phoerious @louib @droidmonkey @TheZ3ro