Differential D26872

Fix crash when KWallet is not available.
ClosedPublic
Actions

Authored by puneethchanda on Jan 23 2020, 3:08 PM.

Details

Reviewers
SGOrava
drosca
Commits
R875:1cecd14fd069: Fix crash when KWallet is not available.
Summary

Bug 398767
Currently, when the user clicks remember password when KWallet is disabled, falkon gets crashed.
This patch fixes the crash by checking if KWallet object is created and only then it adds to the folder.
The following functions are fixed:

  • addEntry
  • Update Entry
  • updateLastUsed
  • removeEntry
  • removeAll

Diff Detail

Repository
R875 Falkon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
puneethchanda created this revision.Jan 23 2020, 3:08 PM
Restricted Application added a subscriber: falkon. · View Herald TranscriptJan 23 2020, 3:08 PM
puneethchanda requested review of this revision.Jan 23 2020, 3:08 PM
SGOrava added inline comments.Jan 23 2020, 3:12 PM
src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp
84

The should be a space after the if and before the block start

if (condition) {
//something
}
91

There is no need to check again here because this should always be true when the code gets here.

puneethchanda updated this revision to Diff 74247.Jan 23 2020, 3:20 PM

updated with the suggested changes.

SGOrava added inline comments.Jan 23 2020, 3:23 PM
src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp
88

Kepp the newline here.
It makes it easier to read.

puneethchanda updated this revision to Diff 74248.Jan 23 2020, 3:33 PM
puneethchanda marked 3 inline comments as done.

add new line.

puneethchanda added a comment.Jan 23 2020, 3:33 PM

@SGOrava can you please review this.

SGOrava added a comment.Jan 23 2020, 3:35 PM

Than you can add the condition into all methods.

src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp
91

empty line would be best, no need for spaces.

SGOrava added a comment.Jan 23 2020, 3:37 PM

To be more precise I think the condition should be in these methids

  • addEntry
  • updateEntry
  • updateLastUsed
  • removeEntry
  • removeAll

Rest should work as is.

drosca requested changes to this revision.Jan 23 2020, 3:38 PM
drosca added a subscriber: drosca.

This check should be in every place where m_wallet is being used.

This revision now requires changes to proceed.Jan 23 2020, 3:38 PM
SGOrava added a comment.Jan 23 2020, 3:41 PM

@drosca Even in destructor ?

puneethchanda updated this revision to Diff 74255.Jan 23 2020, 4:18 PM
puneethchanda retitled this revision from fix crash when remember password is clicked to Fix crash when KWallet is not available..
puneethchanda edited the summary of this revision. (Show Details)

updated the given functions to check for the m_wallet instance.

puneethchanda added a comment.Jan 23 2020, 4:19 PM

SGOrava, changes are made in all the functions described.

SGOrava added a comment.Jan 23 2020, 4:23 PM

Looks fine to me.

@drosca What do you think ?

drosca requested changes to this revision.Jan 23 2020, 4:57 PM
drosca added inline comments.
src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp
101

No reason for this warning.

Also I think it would be just better to change the initialize to bool initialize() and then check everywhere

if (!initialize()) {
   return;
}
This revision now requires changes to proceed.Jan 23 2020, 4:57 PM
SGOrava added a comment.Jan 23 2020, 5:04 PM

In such case it should be totally rethought since the false can be triggered also by not having write permissions.
So i think we should in such case introduce new internal variable to check if we can write into Falkon folder.
Also at first run, we should probably always return true for migration thing, but need to make sure we are in Falkon folder.

much more complicated for first contribution.

drosca added a comment.Jan 23 2020, 5:10 PM
In D26872#599903, @SGOrava wrote:

In such case it should be totally rethought since the false can be triggered also by not having write permissions.

That's not true. If initialize fails then it doesn't make any sense to keep using the wallet as it is in "inconsistent" state.

SGOrava added a comment.Jan 23 2020, 5:12 PM
In D26872#599904, @drosca wrote:

That's not true. If initialize fails then it doesn't make any sense to keep using the wallet as it is in "inconsistent" state.

And how can we do that ?
Can you propose a good idea ?

puneethchanda updated this revision to Diff 74306.Jan 24 2020, 12:42 PM

remove unnecessary warning and clear white space issues.

drosca accepted this revision.Jan 24 2020, 12:53 PM
This revision is now accepted and ready to land.Jan 24 2020, 12:53 PM
SGOrava accepted this revision.Jan 24 2020, 1:09 PM
Closed by commit R875:1cecd14fd069: Fix crash when KWallet is not available. (authored by puneethchanda, committed by SGOrava). · Explain WhyJan 24 2020, 1:15 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 74309

View Options

src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp

Loading...