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.

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

Performance hit while using the KeeTrayTOTP column on large databases #27

Open
Swoy opened this issue Nov 5, 2018 · 4 comments
Open
Assignees
Labels

Comments

@Swoy
Copy link

Swoy commented Nov 5, 2018

So, a long story short:

I've had problems with what I thought was @navossoc's YAFD plugins fault. However, it seems to be KeeTrayTOTP that is the culprit;

I received the following error code in Windows 10 (latest build), running KeePass 2.40 and KeeTrayTOTP 0.9.5.0 combined with HIBPOffline plugin and YAFD:

Application: KeePass.exe
Framework Version: v4.0.30319
Description: The process was terminated due to an unhandled exception.
Exception Info: System.NullReferenceException
   at KeePass.UI.UIUtil.SetAlternatingBgColors(System.Windows.Forms.ListView, System.Drawing.Color, Boolean)
   at KeePass.Forms.MainForm.UpdateEntryList(KeePassLib.PwGroup, Boolean)
   at KeePass.Forms.MainForm.UpdateUI(Boolean, KeePass.UI.PwDocument, Boolean, KeePassLib.PwGroup, Boolean, KeePassLib.PwGroup, Boolean, System.Windows.Forms.Control)
   at KeePass.Forms.MainForm.UpdateUI(Boolean, KeePass.UI.PwDocument, Boolean, KeePassLib.PwGroup, Boolean, KeePassLib.PwGroup, Boolean)
   at HIBPOfflineCheck.HIBPOfflineColumnProv.UpdateStatus()
   at HIBPOfflineCheck.HIBPOfflineColumnProv.PwdTouchedHandler(System.Object, KeePassLib.ObjectTouchedEventArgs)
   at KeePassLib.PwEntry.Touch(Boolean, Boolean)
   at YetAnotherFaviconDownloader.FaviconDialog+<>c__DisplayClass6.<BgWorker_DoWork>b__0(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   at System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object, Boolean)
   at System.Threading.QueueUserWorkItemCallback.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

As you can see, it breaks during list repaint. This happens when using the column TOTP _(Using either open view or asteriks) with any number of entries present. What is more, is that if used with a large number of entries such as this test Database.zip (password to open db is 123 (there is no password on the zip file itself.)) You can clearly see the effect of the seconds counter working the list every second.

  • I think a urgent fix would be to allow this to update only once every time the TOTP changes - as opposed to every second, or simply disable the column feature all together.
  • Another, maybe more sustainable solution would be to somehow check if there are other plugins updating the list, and pause this action until their job is complete on the list.
  • Yet one more; if no TOTP is present in the current list, I see no reason to perform this update at all, and as it stands now, it will continue to check or try to update empty fields.

I've noticed some sluggishness on my main db (around 300-350 entries spanned ~10 groups as well, so it is definitively a performance hit.

Note: Using the column feature of KeeTrayTOTP with other plugins that require list repaint can potentially corrupt data during save, and any change is definitively lost when KeePass crash. So do not try this on your mainDB.

Let me know if you need anything else.

@robinvanpoppel
Copy link
Collaborator

robinvanpoppel commented Nov 6, 2018

Hi, thanks for reporting in such detail. I tried the steps this morning using your test database, but I could not yet get it reproduced.

I had YAFD already, and installed the HIBP Offline plugin just yet (without a password file though).

  • What actions did you take after opening the database to cause this to happen?
  • You mention _This happens when using the column TOTP (Using either open view or asteriks) with any number of entries present.
  • The stack trace indicates a null reference exception in SetAlternatingBgColors. Do you suspect that is also caused by this plugin, or is it just that it is slowing down / hanging?

@robinvanpoppel robinvanpoppel self-assigned this Nov 6, 2018
@Swoy
Copy link
Author

Swoy commented Nov 6, 2018

Update: navossoc updated the issue in his repo where he says that the HIBP-module mainly caused the crashes when it tries to update each entry with new data in it's own cell. So this issue is mostly because of the slugishness. I apologize for that.

Hello again, if you try and add a few more TOTP's in the Database file? And then try to scroll? I mean, it could be that my computer is the culprit? I'm on a Dell 9570 ( i7 / with 1050 Ti Q-max ). Maybe there is something I can do on my side. But I swear, once I enable the TOTP column, the fans start to spin immediately:

20181106103728-task manager

And once I disable it again, it quickly goes down to normal:

20181106103822-task manager

Steps I take ( updated ):

  • Open the DB
  • Enable TOTP column
  • Scroll quickly in the root folder where all entries are.
  • I observe that the list is lagging each second and the computer spins up the fan due to high CPU load.
  • I disable the TOTP column.
  • And I observe that the list is butter smooth and the computer spins down the fan due to low CPU load.

For this I am only using the TOTP plugin enabled.

@chiwou
Copy link

chiwou commented Dec 25, 2018

Hi, had the same issues, it has something to do with the fields you display in KeePass, try reducing it to a minimum.
Title | Username | Url
And try again

@robinvanpoppel
Copy link
Collaborator

I've been investigating this one. When the column is enabled a timer will try and update the otp code every second (KeeTrayTOTPExt.OnTimerTick) . There are some checks whether to do a repaint , and although this part could be improved a little bit, the real drag is requesting a repaint. The plugin is calling PluginHost.MainWindow.RefreshEntriesList(); which is really heavy and also causes all other plugins to provide new entry data. Preferably we could ask Keepass to repaint / refresh only the password entries for which we know there are totp settings.

For now I don't have a definite solution. I'll have to take a look at the source code of Keepass if such a method exists. We could touch the password entry, which might trigger a repaint, but that would also prompt you to save the database when exiting so I'd rather not do that. Otherwise we might have to reach out to the Keepass author.

robinvanpoppel added a commit that referenced this issue Apr 13, 2020
This is by no means stable and a questionable solution.

* Did a lot of refactors that might be moved to a separate branch.
* Added a build / run configuration for the database provided in the issue.
  It contains 2000 entries which is already a lot to choke on for KeePass itself, let alone the custom column
* Reproduce by enabling the TOTP column via View > Configure Columns.

Gotcha's:
* Tried to locate the KeePass's listview based on it's name and type.
  Probably not the way a plugin should extend the functionality
* In the old implementation the timer would trigger a redraw for all the items in the listview.
  In the new this code tries to locate entries that have TOTP settings and keepps them in a few dictionaries to
  prevent repainting / looping through all the items. That means the cache has to be invalidated on certain conditions.
  I did not really look for events that we can subscribe to, but the old implementation kept track of the selected group.
  The new one invalidates when the number of items change, of the modification time changes on one of the tracked entries.
* Tried to keep the old otp value around to prevent recalculating a perfectly valid value.

Thoughts:
* What are the security concerns for keeping TOTP code's around. I guess this also applies to the current implementation
  as the otp code was recalculated every few seconds and is visible in the UI.
* Double clicking the column invokes an action on the column provider, so we could also chose to show a text "Double click to copy TOTP code".
  On the other side we already have a Ctrl-T for the current selection, so what is the purpose of the column anyway?
* If the idea is to show all TOTP codes for your entries, we could remove the column functionality alltogether and create a separate window
  where you can view all your TOTP Entries (like Google Authenticator).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants