After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773587 - [PATCH] recent-manager: Add a limit to the list's size
[PATCH] recent-manager: Add a limit to the list's size
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkRecent
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
Emmanuele Bassi (:ebassi)
Depends on:
Blocks:
 
 
Reported: 2016-10-27 15:02 UTC by Lauri Kasanen
Modified: 2016-11-14 19:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
master patch (5.00 KB, text/plain)
2016-10-27 15:02 UTC, Lauri Kasanen
  Details
gtk2 patch (4.40 KB, patch)
2016-10-27 15:02 UTC, Lauri Kasanen
none Details | Review
gtk3 patch (5.01 KB, patch)
2016-10-27 15:03 UTC, Lauri Kasanen
none Details | Review
gtk2 patch (3.00 KB, patch)
2016-10-27 16:31 UTC, Lauri Kasanen
none Details | Review
gtk3 patch (3.40 KB, patch)
2016-10-27 16:32 UTC, Lauri Kasanen
none Details | Review
master patch (3.40 KB, patch)
2016-10-27 16:32 UTC, Lauri Kasanen
none Details | Review
gtk2: Fix the memory leak (815 bytes, patch)
2016-11-11 09:39 UTC, Lauri Kasanen
committed Details | Review
gtk3: Fix the memory leak (815 bytes, patch)
2016-11-11 09:40 UTC, Lauri Kasanen
committed Details | Review
master: Fix the memory leak (815 bytes, patch)
2016-11-11 09:40 UTC, Lauri Kasanen
committed Details | Review

Description Lauri Kasanen 2016-10-27 15:02:04 UTC
Created attachment 338615 [details]
master patch

This fixes a DOS where any app can cause all running gtk apps
to use arbitrary amounts of memory.

Originally reported against mate-panel, where running a big slideshow
in eye-of-mate caused increasing RAM usage in mate-panel.

This affects all active gtk versions. I'm attaching patches for master, gtk-3-22, and gtk-2-24 branches.
Comment 1 Lauri Kasanen 2016-10-27 15:02:32 UTC
Created attachment 338616 [details] [review]
gtk2 patch
Comment 2 Lauri Kasanen 2016-10-27 15:03:01 UTC
Created attachment 338617 [details] [review]
gtk3 patch
Comment 3 Emmanuele Bassi (:ebassi) 2016-10-27 16:03:07 UTC
We discussed this on IRC, so here's a summary:

 * a new GtkSettings is problematic on API/feature frozen branches (2.24, 3.22)
 * GtkSettings are also not a good way to control potential DoS scenarios
 * the DoS is still going to be a problem until applications are untrusted by default, and executed into a sandbox, so adding a limit is just a temporary mitigation
 * a hard-coded limit to an arbitrary large number of items when saving the list is perfectly acceptable; 10000 items seems to be a good ballpark figure in order to keep the parsing fast
 * any consumer of the recently used files list should use its own limit to match the UI — e.g. a menu should only load 10/15 items
 * any tool that adds files to the recently used files list should also exercise some caution when adding items to the list; automatically adding thousands or more items in a single session would make the list completely useless anyway
Comment 4 Lauri Kasanen 2016-10-27 16:31:34 UTC
Created attachment 338627 [details] [review]
gtk2 patch

Posting v2 with a hardcoded 1k value, as discussed.
Comment 5 Lauri Kasanen 2016-10-27 16:32:03 UTC
Created attachment 338628 [details] [review]
gtk3 patch
Comment 6 Lauri Kasanen 2016-10-27 16:32:23 UTC
Created attachment 338629 [details] [review]
master patch
Comment 7 Lauri Kasanen 2016-10-27 17:55:22 UTC
ebassi requested memory information; it's not strictly related to this bug, but writing it here so that it's permanently saved somewhere.

* mate-panel uses a gtkRecentChooserMenu, without setting a limit
* it's this menu that's responsible for most of the RAM waste
* with 2940 images in recent files, the menu used 7.15mb of RAM
* that makes 2552 bytes per entry
* 1024 bytes of that is in the mime type icon

For some reason, the mime type icons were not shared, each jpg image had its own copy in memory.

This is then multiplied for each active gtk app using the menu. At 10k items, it would be 24.3mb per application. IMHO that is too much, at 1k it'd be 2.4mb per app, a more palatable number.
Comment 8 Lauri Kasanen 2016-10-27 18:03:54 UTC
This is in addition to the 104-byte-sized _GtkRecentInfo, which at 10k would be 1mb and at 1k 100kb.
Comment 9 Lauri Kasanen 2016-10-28 07:56:55 UTC
Adding info on the dynamic allocations of GtkRecentInfo. From the same run as the above:

* 930602 bytes were in the GtkRecentInfo allocations (including themselves at 104 bytes/piece)
* that's ~316 bytes per entry

So to wrap up, every gtk application using recent files would take 3mb at 10k, or 300kb at 1k. If they also use the recent chooser menu, add the numbers from two comments above.
Comment 10 Lauri Kasanen 2016-11-05 13:59:00 UTC
Weekly ping.
Comment 11 Matthias Clasen 2016-11-07 15:16:10 UTC
plesae don't do that. it just adds to the noise
Comment 12 Lauri Kasanen 2016-11-07 15:27:12 UTC
Sorry. How do you prefer to get reminders so this doesn't get forgotten and bitrot?
Comment 13 Massimo 2016-11-11 07:18:17 UTC
The commit:

https://git.gnome.org/browse/gtk+/commit/?h=gtk-3-22&id=bf560369f2401e2cdd16f962f248e98c890835d0

introduced a new memory leak:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkrecentmanager.c?h=gtk-3-22#n1481

the early return code path skips the g_strfreev at the
end of the function.

Another memory leak dismissing filechoosers is here:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkfilesystemmodel.c?h=gtk-3-22#n1285

when error matches CANCELLED error is not freed.

Another one is here:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkpathbar.c?h=gtk-3-22#n1670

'file_info' is not '..finished' when g_error_matches returns TRUE
Comment 14 Lauri Kasanen 2016-11-11 09:39:16 UTC
Thanks for noticing! I'm attaching patches for that. Should I open a new bug for these?

The others weren't by me, these only fix the one I caused.
Comment 15 Lauri Kasanen 2016-11-11 09:39:59 UTC
Created attachment 339598 [details] [review]
gtk2: Fix the memory leak
Comment 16 Lauri Kasanen 2016-11-11 09:40:22 UTC
Created attachment 339599 [details] [review]
gtk3: Fix the memory leak
Comment 17 Lauri Kasanen 2016-11-11 09:40:43 UTC
Created attachment 339600 [details] [review]
master: Fix the memory leak
Comment 18 Matthias Clasen 2016-11-12 20:49:47 UTC
Review of attachment 339598 [details] [review]:

ok
Comment 19 Matthias Clasen 2016-11-12 20:50:04 UTC
Review of attachment 339599 [details] [review]:

ok
Comment 20 Matthias Clasen 2016-11-12 20:54:10 UTC
Review of attachment 339600 [details] [review]:

ok