keepassium / KeePassium

KeePass-compatible password manager for iOS
https://keepassium.com
Other
1.18k stars 104 forks source link

Autofill Memory Limits exceeded on relatively small database #353

Open ixs opened 6 months ago

ixs commented 6 months ago

Description I have recently started running into the memory issues related to autofill described already in https://keepassium.com/articles/autofill-memory-limits/.

Database has always been at around 850kB but suddenly grew to around 4MB. The DB is AES encrypted and thus should be easy on memory use. I think a 4MB file should not be a huge problem, even when expanded.

I did a bit of digging into the database using the pykeepass library. Dumping the prettyfied XML structure seems to clock in at around 18MB. This does not seem that much for the database itself objectively but it seems this is already too much, as autofill just dies silently.

I was astonished however to see that my CustomIcons list is pretty big. The kdbx4 file on disk is 3.760.867 Bytes big. Iterating over the Icon stucture, decoding the base64 stored icons and adding the binary-size together all the icons add up to 3.389.518 Bytes.

I am currently wrangling with the pykeepass library to see if I cannot clean the icons in an automated fashion as there are some 300+kB favicons in there which is ridiculous.

But in the meantime, I am wondering if the autofill extension couldn't be changed slightly to disregard icons completely when running in autofill mode.

That might save some memory and maybe prevent the extension from crashing?

Environment:

This is happening with the KeePassium Pro version (the black icon one), not sure if it's relevant.

keepassium commented 6 months ago

Dumping the prettyfied XML structure seems to clock in at around 18MB. This does not seem that much for the database itself objectively but it seems this is already too much, as autofill just dies silently.

I agree, such an XML this should not be too big of a deal. Dying silently means running out of memory at XML parsing or processing stage.

As much as I would love to skip custom icons and attachments while loading in AutoFill, this is quite problematic. Due to an early design mistake, KeePassium uses a DOM parser (which loads the whole XML tree to memory), rather than an event-based SAX parser (which parses XML in small chunks and can make decisions during the process). To my defense, at the time of this decision AutoFill was not even announced, so memory consumption was not that important.

Well, now memory became important, but optimization requires rewriting the whole database processing code. Which is a large all-or-nothing task that cannot be done incrementally. It is indeed on the list, but there is always something more urgent…

\</rant>

there are some 300+kB favicons in there which is ridiculous.

This might be the reason.

When adding larger favicons, KeePassium specifically downsizes them to 128x128 PNG. But when showing the existing ones, resizing is done only in UI. Which means, according to this post, that the original image, in full resolution and colors, is decompressed to memory for displaying. I guess a 300 kB compressed PNG turns into quite a heavy bitmap…

(By the way, were the large favicons added by KeePassium? If so, there is probably a separate issue in the downsizing code.)

ixs commented 6 months ago

I agree, such an XML this should not be too big of a deal. Dying silently means running out of memory at XML parsing or processing stage.

Thank you for confirming. This was my understanding as well.

As much as I would love to skip custom icons and attachments while loading in AutoFill, this is quite problematic. Due to an early design mistake, KeePassium uses a DOM parser (which loads the whole XML tree to memory), rather than an event-based SAX parser (which parses XML in small chunks and can make decisions during the process). To my defense, at the time of this decision AutoFill was not even announced, so memory consumption was not that important.

Right. DOM Parser seems like a sensible choice. If there were no memory pressure. Which there now unfortunately is. Bummer.

Well, now memory became important, but optimization requires rewriting the whole database processing code. Which is a large all-or-nothing task that cannot be done incrementally. It is indeed on the list, but there is always something more urgent…

Of course. But hey, easter is coming up, what better time to relax, ponder past sins and rewrite such code?! 😆

there are some 300+kB favicons in there which is ridiculous.

This might be the reason.

When adding larger favicons, KeePassium specifically downsizes them to 128x128 PNG. But when showing the existing ones, resizing is done only in UI. Which means, according to this post, that the original image, in full resolution and colors, is decompressed to memory for displaying. I guess a 300 kB compressed PNG turns into quite a heavy bitmap…

Interesting. Just read that post and if that's the case that would indeed explain the problem I'm seeing.

(By the way, were the large favicons added by KeePassium? If so, there is probably a separate issue in the downsizing code.)

I am not certain. I've been using KeepassXC and KeePassium in parallel and do not remember exactly, which icons were added where.

When I was looking for testcases for large icons in my database, I noticed a weird thing though: The large icons in my db are all icons with 384 x 384px resolution. But the source websites only offer smaller icons:

So it looks like something is actually saving the icons in a larger format than needed? Could that someone be KeePassium?

keepassium commented 6 months ago

Could that someone be KeePassium?

Guess what, you are absolutely right! KeePassium successfully "downscales" 180x180px icons into 128x128pt, which happens to be 384x384px on modern devices :) I've created a separate issue for that: https://github.com/keepassium/KeePassium/issues/354

keepassium commented 6 months ago

Of course. But hey, easter is coming up, what better time to relax, ponder past sins and rewrite such code?! 😆

See, there is always something smaller and more urgent :)

ixs commented 6 months ago

See, there is always something smaller and more urgent :)

🤯

I wiped the 384x384px icons from the database. Database is now back down to about 1MB and Autofill is not dying anymore with that size. So I guess this is a win? 😆

Of course, the overall problem is still unsolved, autofill should not be dying over icons...