[Bug 43570] New: Memory leark in SafeURL abd HtmlScrubber Template::Toolkit filters
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Bug ID: 43570 Summary: Memory leark in SafeURL abd HtmlScrubber Template::Toolkit filters Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Templates Assignee: oleonard@myacpl.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Right now every request that renders a page using SafeURL or HtmlScrubber leaks the entire template context. It seems to be around 7 to 13 MB per request depending on the page ( almost 8 MB on opac-detail, 11 on circulation.pl, and 13 on moremember.pl ). This memory doesn't get released at the end of the request because the context points to the filter, and the filter points to the context. The reason it hurts so much is what the context is. It's the root of the whole render. Every compiled template, the stash, and everything the stash was holding. On opac-detail.pl that's the Koha::Biblio, the MARC::Record and all the compiled templates. So one stuck context drags a whole page's worth of stuff along with it, and then we do it again on the next request, and the next. This is actually a known issue in TT 2.20+ and was done on purpose: # This causes problems: https://rt.cpan.org/Ticket/Display.html?id=46691 # If the plugin is loaded twice in different templates (one INCLUDEd into # another) then the filter gets garbage collected when the inner template # ends (at least, I think that's what's happening). So I'm going to take # the "suck it and see" approach, comment it out, and wait for someone to # complain that this module is leaking memory. # weaken($this); The call to weaken() was restored in TT 3.1, and they weaken _CONTEXT rather than the plugin itself, which is the same thing my patch will do. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |critical Assignee|oleonard@myacpl.org |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #1 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Tp reproduce: 1) Set plack_workers to 1 and plack_max_requests to 5000 in koha-conf.xml 2) Restart all the things! 3) Note the RSS of the starman worker 4) Request /cgi-bin/koha/opac-detail.pl?biblionumber=N fifty times, using a different biblionumber each time 5) Note the worker has put on about 400 MB! 6) Do the same against /cgi-bin/koha/members/moremember.pl and /cgi-bin/koha/circ/circulation.pl for the HtmlScrubber half 7) Note that opac-main.pl doesn't grow at all 8) Apply this patch, restart all the things, and run through it again 9) Note the worker stays put! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Hmmm interesting! I'll definitely put this on my todo list to review! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 206475 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206475&action=edit Bug 43570: Add unit tests Patch from commit e679703 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 206476 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206476&action=edit Bug 43570: SafeURL and HtmlScrubber template plugins leak the template context on every request Every request that renders a template using the SafeURL or HtmlScrubber filter leaks the whole Template::Context: the compiled templates, the stash and everything in it. That's 7 to 13 MB per request that doesn't come back until the worker is recycled. Both plugins call install_filter, which stores a closure over the plugin in the context's filter provider, while the plugin holds the context in _CONTEXT. Template::Plugin::Filter has the weaken() that would break the cycle commented out. Weakening the plugin's reference to the context fixes it; the context is always alive while a template is being processed, which is the only time the plugin uses it. Test Plan: 1) Apply the first patch 2) prove t/db_dependent/Template/Plugin/SafeURL.t t/db_dependent/Template/Plugin/HtmlScrubber.t 3) Note the "template context is released" subtests fail 4) Set plack_workers to 1 and plack_max_requests to 5000 in koha-conf.xml 5) Restart all the things! 6) Note the RSS of the starman worker 7) Request /cgi-bin/koha/opac-detail.pl?biblionumber=N fifty times, using a different biblionumber each time 8) Note the worker has put on about 400 MB! 9) Apply the second patch 10) prove the tests again, note they pass! 11) Restart all the things and repeat steps 6 through 8 12) Note the worker stays put! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Laura O'Neil <laura@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Memory leark in SafeURL abd |Memory leak in SafeURL abd |HtmlScrubber |HtmlScrubber |Template::Toolkit filters |Template::Toolkit filters -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Memory leak in SafeURL abd |Memory leak in SafeURL and |HtmlScrubber |HtmlScrubber |Template::Toolkit filters |Template::Toolkit filters CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206475|0 |1 is obsolete| | Attachment #206476|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #5 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 206706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206706&action=edit Bug 43570: Add unit tests Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #6 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 206707 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206707&action=edit Bug 43570: SafeURL and HtmlScrubber template plugins leak the template context on every request Every request that renders a template using the SafeURL or HtmlScrubber filter leaks the whole Template::Context: the compiled templates, the stash and everything in it. That's 7 to 13 MB per request that doesn't come back until the worker is recycled. Both plugins call install_filter, which stores a closure over the plugin in the context's filter provider, while the plugin holds the context in _CONTEXT. Template::Plugin::Filter has the weaken() that would break the cycle commented out. Weakening the plugin's reference to the context fixes it; the context is always alive while a template is being processed, which is the only time the plugin uses it. Test Plan: 1) Apply the first patch 2) prove t/db_dependent/Template/Plugin/SafeURL.t t/db_dependent/Template/Plugin/HtmlScrubber.t 3) Note the "template context is released" subtests fail 4) Set plack_workers to 1 and plack_max_requests to 5000 in koha-conf.xml 5) Restart all the things! 6) Note the RSS of the starman worker 7) Request /cgi-bin/koha/opac-detail.pl?biblionumber=N fifty times, using a different biblionumber each time 8) Note the worker has put on about 400 MB! 9) Apply the second patch 10) prove the tests again, note they pass! 11) Restart all the things and repeat steps 6 through 8 12) Note the worker stays put! Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from David Cook <dcook@prosentient.com.au> --- Reading the description in more detail... what a weird situation. So the problem is with the install_filter method which we don't use in most of our Koha::Template::Plugin filter plugins... OK. Looking at Template::Plugin::JSON::Escape it uses $context->define_filter() and that $context gets saved within the $self... But the problem with install_filter is that it wraps the filter method with a closure. I'm not sure of the significance of 3.1 as it seems the weaken fix was re-added in 2.28 eight years ago. Of course, we're still on 2.27... -- I'm going to fail this one as it looks like we're copying the 2.28 fix but newer versions (like 2.29 and up) use the following: weaken( $this->{_CONTEXT} ) if ref $this->{_CONTEXT} && !isweak $this->{_CONTEXT}; (See https://github.com/cpan-authors/Template2/issues/206) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- Also, I think this should only affect dynamic plugins... SafeURL doesn't need to be dynamic. I probably just did that out of habit. HtmlScrubber on the other hand does need to be dynamic because it takes args. Could be interesting to play around a bit more with this one... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #9 from David Cook <dcook@prosentient.com.au> --- Actually tell you what I'll move it back to Signed Off, and I'll try to write a follow-up tomorrow... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #206706|0 |1 is obsolete| | Attachment #206707|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207083 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207083&action=edit Bug 43570: Add unit tests Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207084 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207084&action=edit Bug 43570: SafeURL and HtmlScrubber template plugins leak the template context on every request Every request that renders a template using the SafeURL or HtmlScrubber filter leaks the whole Template::Context: the compiled templates, the stash and everything in it. That's 7 to 13 MB per request that doesn't come back until the worker is recycled. Both plugins call install_filter, which stores a closure over the plugin in the context's filter provider, while the plugin holds the context in _CONTEXT. Template::Plugin::Filter has the weaken() that would break the cycle commented out. Weakening the plugin's reference to the context fixes it; the context is always alive while a template is being processed, which is the only time the plugin uses it. Test Plan: 1) Apply the first patch 2) prove t/db_dependent/Template/Plugin/SafeURL.t t/db_dependent/Template/Plugin/HtmlScrubber.t 3) Note the "template context is released" subtests fail 4) Set plack_workers to 1 and plack_max_requests to 5000 in koha-conf.xml 5) Restart all the things! 6) Note the RSS of the starman worker 7) Request /cgi-bin/koha/opac-detail.pl?biblionumber=N fifty times, using a different biblionumber each time 8) Note the worker has put on about 400 MB! 9) Apply the second patch 10) prove the tests again, note they pass! 11) Restart all the things and repeat steps 6 through 8 12) Note the worker stays put! Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207086 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207086&action=edit Bug 43570: (QA follow-up) Guard weaken() against non-ref/already-weak _CONTEXT -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 207087 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207087&action=edit Bug 43570: (QA follow-up) Make SafeURL a static filter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- QA follow-up: attached two small patches building on the signed-off/passed fix, addressing David's two comments (#7 and #8) which were never followed up on: 1) Bug 43570: (QA follow-up) Guard weaken() against non-ref/already-weak _CONTEXT Mirrors the guard Template-Toolkit itself uses from 2.29 onwards (weaken(...) if ref ... && !isweak ...) instead of the bare weaken() copied from the 2.28 fix. Confirmed calling weaken() twice on the same slot is a safe no-op with the Scalar::Util shipped here, so this isn't fixing a live bug - just bringing us in line with upstream's own defensive style. 2) Bug 43570: (QA follow-up) Make SafeURL a static filter SafeURL's filter() never reads $args/$config, so _DYNAMIC = 1 isn't needed there (HtmlScrubber genuinely needs it for its 'type' config arg). Verified the filter still works correctly as static (same test suite + a manual render check). Both are small, low-risk, isolated commits on top of the existing signed-off fix - not blocking, just tidying up before this ships. Re-ran the full test suite and koha-qa.pl across all 4 commits, all green. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I took the liberty of adding the two follow-ups David mentioned in his absence. They were clear easy wins .. ran through testing after and everything still works as expected then the memory leak remains resolved. I do question why we're pinned to an old TT.. but that should be it's own bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize (ashimema) from comment #15)
I took the liberty of adding the two follow-ups David mentioned in his absence. They were clear easy wins .. ran through testing after and everything still works as expected then the memory leak remains resolved.
Thanks, Martin :).
I do question why we're pinned to an old TT.. but that should be it's own bug.
It's a good question. I think Debian/Ubuntu maintainers must've opted to keep things as stable as possible while backporting security fixes: See my comments on https://github.com/cpan-authors/Template2/issues/327 That said, Template packages in Debian Trixie and older are missing some important security fixes (or was in June at least). Since there is some XS code in Template2, packaging our own version is a bit trickier, but not impossible. I should really get onto the Debian maintainer to try to get him to at least backport more fixes... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #17 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|26.11.00 |26.11.00,26.05.05 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43570 --- Comment #18 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.05 release. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org