Bug 15553 - cgisess_ files polluting the /tmp directory
Summary: cgisess_ files polluting the /tmp directory
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Blou
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-11 21:40 UTC by Blou
Modified: 2017-12-07 22:15 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15553 - cgisess_ files polluting the /tmp directory (1.50 KB, patch)
2016-01-11 21:45 UTC, Blou
Details | Diff | Splinter Review
Bug 15553 - [SIGNED-OFF] cgisess_ files polluting the /tmp directory (1.91 KB, patch)
2016-01-12 18:51 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 15553 - [SIGNED-OFF] cgisess_ files polluting the /tmp directory (2.01 KB, patch)
2016-01-13 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15553: [QA Follow-up] Replace hardcoded tmp and add instance to dirname (1.54 KB, patch)
2016-01-13 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2016-01-11 21:40:59 UTC
When some users want to improve performance, they get the recommandation to switch the syspref "sessionstorage" to 'tmp', which store the session info in the /tmp directory instead of the database.

Depending on the frequency of the cleaning process (or lack thereof), this can be heavy pollution into the /tmp directory, making it unusable.  "rm cgi*" would not even work since cgi* extends to over 8000 items...

A simple fix is to modify the target directory in C4/Auth.pm to /tmp/cgisess.
Comment 1 Blou 2016-01-11 21:45:55 UTC Comment hidden (obsolete)
Comment 2 Blou 2016-01-11 21:51:00 UTC
My initial plan was to have some value specific to each library (when multiple libraries on a server), but I didn't want to add a syspref, nor could I find an easy, meaningful value to reuse.

Deleting the dir poses no problem, since CGI:Session creates the directory when needed, owned by www-data (or equivalent).

I only tested on Ubuntu & Debian.  No Windows.

I added that for our benefit, thought it might be useful to others.  No biggie if no one's interested :)
Comment 3 Magnus Enger 2016-01-12 18:35:56 UTC
Too many /tmp/cgisess* files for a simple rm can also come as an unpleasant surprise if you have Koha set up to use memcached for sessions, and memcached dies for some reason. Bam, next morning your disk is full...
Comment 4 Magnus Enger 2016-01-12 18:51:06 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-01-13 10:33:09 UTC
Created attachment 46572 [details] [review]
Bug 15553 - [SIGNED-OFF] cgisess_ files polluting the /tmp directory

When some users want to improve performance, one suggestion is to switch the syspref "sessionstorage" to 'tmp',
which store the session info in the /tmp directory instead of the database.

Depending on the frequency of the cleaning process (or lack thereof),
this can be heavy pollution into the /tmp directory, making it unusable.
"rm cgi*" would not even work since cgi* extends to over 8000 items...

A simple fix is to modify the target directory in C4/Auth.pm to /tmp/cgisess.

Testing scenario:
1) change the syspref SessionStorage to 'tmp'
2) Log into the OPAC, validate that a file named /tmp/cgisess_(something) got created
3) Apply the patch
4) Log into the OPAC, validate that a file /tmp/cgisess/cgisess_(something) got created

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Session files are created in /tmp/cgisess/ instead
of directly in /tmp. I usually store sessions in memcached, but a
couple of times memcached has died on me, and before I knew it, /tmp
was so full of sessions files, I could not easily delete them with
"rm /tmp/cgisess*". Being able to delete /tmp/cgisess/ should be a
bit easier.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2016-01-13 10:33:13 UTC
Created attachment 46573 [details] [review]
Bug 15553: [QA Follow-up] Replace hardcoded tmp and add instance to dirname

If you use multiple instances on one server, you could have the situation
that instance A creates the cgisess subfolder and instance B could have a
permission problem. This patch resolves that by allowing each instance to
have its own cgisess subfolder.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-01-13 10:35:36 UTC
QA Comment:
Looks good to me, adding the multiple instance case.
Another report could deal with cleaning up this folder. Why not remove all older session files in cleanup_database.pl on daily basis?

Passed QA
Comment 8 Jonathan Druart 2016-01-13 12:03:58 UTC
Note that users will be logged out.
Comment 9 Brendan Gallagher 2016-01-27 02:52:49 UTC
Pushed to Master - Should be in the may 2016 Release.  Thanks!
Comment 10 Julian Maurice 2016-01-29 14:48:31 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 11 Frédéric Demians 2016-02-12 06:42:13 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.