The fix for bug 34893 alters the return value type of checkpw, but does not adequately update the code everywhere where this is used. In particular, with shibboleth enabled the cas_ticket is sent to a patron object which gets serialized into the user session which quickly consumes a large amount of disk space on a busy system.
Created attachment 161888 [details] [review] Bug 36034: fix capture of return values from checkpw Adapt code to the change of return value type of checkpw introduced in bug 34893
The disk space problem happens with binary logging enabled and using mysql as session storage. Also, it seems to happen both with and without shibboleth enabled. I have been running 23.05.08 for less than 2 days and are already seeing a large increase in disk space on several servers. I have emergency patched my servers and I suspect that many libraries are going to run into problems with disk space in the coming weeks.
(In reply to Andreas Jonsson from comment #2) > The disk space problem happens with binary logging enabled and using mysql > as session storage. > > Also, it seems to happen both with and without shibboleth enabled. > > I have been running 23.05.08 for less than 2 days and are already seeing a > large increase in disk space on several servers. > > I have emergency patched my servers and I suspect that many libraries are > going to run into problems with disk space in the coming weeks. Is this patch ready for sign-off? That status is still NEW.
It is complex code. I'll let someone else be the judge if it is ready for signoff. I have no good idea for test plan other than inspecting the binary logs of the database. But I would say that it is urgent.
Also I am already running this patch live on several servers.
Created attachment 161926 [details] [review] Bug 36034: fix capture of return values from checkpw Adapt code to the change of return value type of checkpw introduced in bug 34893 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Changes seem clearly correct, all unit tests are passing.
Created attachment 162130 [details] [review] Bug 36034: fix capture of return values from checkpw Adapt code to the change of return value type of checkpw introduced in bug 34893 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
As Kyle said, code review shows the fix clearly. You can also see the difference in the "sessions" table after a logging both before and after applying the patch. After applying the patch, the session data looks correct and isn't full of serialized data from a Koha::Patron object. QA tools are happy. This one is a ticking timebomb for anyone running one of the versions that contains bug 34893, so I think releasing this ASAP would be wise. I don't know how to reproduce the "Too many nested open parens in regex;" error thrown by YAML::XS::__qr_loader, but I imagine it's happening to people in the wild as it's been happening to us in dev. Good job on reporting and fixing this one, Andreas. I didn't notice it in my emails until I sent that listserv email where you commented. Not sure what happened there...
Created attachment 162131 [details] [review] Bug 36034: (bug 34893 follow-up) fix capture of return values from checkpw Adapt code to the change of return value type of checkpw introduced in bug 34893 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 162132 [details] [review] Bug 36034: Add test Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 24.05.00. I did an inline tidy of the block added to unit tests to make the QA script happy. Thanks Jonathan.
Pushed to 23.11.x for 23.11.03
Backported to 23.05.x for upcoming 23.05.09
Backported 22.11.15
Created attachment 162529 [details] [review] Bug 36034: [22.05] Add test Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>