Review Request 128831: Check whether kwallet is enabled in Wallet::isOpen(name)

[Wolfgang Bauer]

> On Sept. 24, 2016, 2:40 p.m., Elvis Angelaccio wrote:
> > src/api/KWallet/kwallet.cpp, line 366
> > <https://git.reviewboard.kde.org/r/128831/diff/1/?file=475994#file475994line366>
> >
> >     You should probably use isEnabled() instead, in case someone is using ksecretservice (though m_walletEnabled is already used all over the place)...
> >     
> >     Anyway +1

This is inside the non-ksecretservice codepath, so it makes no difference whether one uses m_walletEnabled or isEnabled() (not to mention that I think the distinction is not even necessary in isEnabled(), AFAICS m_walletEnabled is set accordingly in the ksecretservice case too).

And as you write the check is like that everywhere else in the code.
I don't think it makes sense to deviate here in this one method, if that's desirable it should be changed everywhere in a separate commit IMHO.

In case you mean that the check should be done using isEnabled() right at the start of the method (i.e. before the if):
I'm not sure this check is needed at all in the ksecretservice case, there is no ->getInterface().xxx call that could crash.

KWalletDLauncher does check whether the wallet is enabled and just returns if not without doing anything. This means it doesn't start the KWallet service in the non-ksecretservice, that's what causes the mentioned crash in the first place because a nullptr is dereferenced.

I don't know anything about ksecretservice, but the code here probably will return false in case the wallet is disabled anyway (at least that's my feeling after looking at this code).


- Wolfgang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128831/#review99502
-----------------------------------------------------------


On Sept. 4, 2016, 11:20 p.m., Wolfgang Bauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128831/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2016, 11:20 p.m.)
> 
> 
> Review request for KDE Frameworks and Valentin Rusu.
> 
> 
> Bugs: 358260
>     https://bugs.kde.org/show_bug.cgi?id=358260
> 
> 
> Repository: kwallet
> 
> 
> Description
> -------
> 
> If kwallet is disabled, walletlauncher() fails and walletLauncher()->getInterface().isOpen(name) causes a crash.
> This affects e.g. drkonqi, but probably also other applications.
> 
> Return false in this case, if kwallet is disabled a wallet cannot be open either.
> 
> 
> Diffs
> -----
> 
>   src/api/KWallet/kwallet.cpp dffebda 
> 
> Diff: https://git.reviewboard.kde.org/r/128831/diff/
> 
> 
> Testing
> -------
> 
> - disable kwallet in the settings
> - start a KF5 application (say, dolphin)
> - make it crash, e.g. via "killall -ILL dolphin"
> - try to report the crash with drkonqi
> 
> When you get to the page where you have to enter the bugzilla username/password, drkonqi crashed, with this patch it doesn't crash.
> 
> drkonqi is also still able to get the username/password from kwallet if it is enabled.
> 
> 
> Thanks,
> 
> Wolfgang Bauer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160924/8f1a6653/attachment.html>