Repository navigation
ext/session: Do not GC a session file another request still holds locked - #25002
Open
AsahiSoftWareEngineer wants to merge 4 commits into
Open
AsahiSoftWareEngineer wants to merge 4 commits into
AsahiSoftWareEngineer wants to merge 4 commits into
Conversation
ps_files_open() holds flock(LOCK_EX) on a session's data file for as long as the owning request is active (e.g. a slow payment callback or a large file upload with session.upload_progress). ps_files_cleanup_dir(), the files save handler's garbage collector, never checked for that lock: it unlinked any sess_* file whose mtime was older than gc_maxlifetime regardless of whether another process currently had it open. On POSIX, unlinking a file that is still open succeeds silently: the directory entry disappears, but the holder's already-open fd keeps writing into the now-orphaned inode. session_write_close() therefore returns success while the data vanishes, and the next read under the same session ID silently starts from an empty session. This is reachable without any special timing or privilege: session_gc() runs this sweep immediately and unconditionally (bypassing gc_probability), requires only the caller's own, unrelated active session, and session.gc_maxlifetime is PHP_INI_ALL so a caller can lower it to 0 for their own request regardless of what the administrator configured, making every session file in save_path other than the one just written a GC target. Probe each candidate file with a non-blocking flock() before deleting it; skip files that are still locked by another process instead of destroying them, and only count a deletion once unlink() actually succeeds (its return value was also previously ignored). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AsahiSoftWareEngineer
requested review from
Girgias and
jorgsowa
as code owners
October 28, 2026 14:55
AsahiSoftWareEngineer
marked this pull request as draft
October 28, 2026 15:19
The test calls session_start() more than once in a single script and produces output in between. With session.use_cookies and session.cache_limiter at their defaults, session_start() refuses to run once any output has already been sent, and session.gc_maxlifetime cannot be changed once a session is already active. Disable cookies and the cache limiter, set gc_maxlifetime before the first session_start(), and defer all var_dump() output to the end of the script so no session call runs after output has started. This only affects the test's own mechanics; the patched ps_files_cleanup_dir() behavior under test is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AsahiSoftWareEngineer
marked this pull request as ready for review
October 28, 2026 15:19
…ace.phpt
session.use_strict_mode defaults to on and rejects a caller-supplied
session ID that has no existing session file, silently replacing it
with a freshly generated one instead. This test relies on
session_id($victimId) / session_id('attackerxxx...') actually pinning
those IDs for brand-new sessions, so the victim and attacker files
were never the ones the test thought they were: $victimFile pointed
at a file that didn't exist, touch() created it empty, and the final
assertion compared against an unrelated empty session instead of the
one holding 'secret' => 'still-in-use'. This made the test fail on
every platform in CI regardless of whether the mod_files.c fix works.
Disabling strict mode for the duration of this test lets the chosen
IDs stick, so the test actually exercises the GC/flock race it is
meant to cover. Verified locally (CLI build) that the test now passes
reliably, and that the rest of ext/session/tests/ is unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ps_files_cleanup_dir()(thefilessession save handler's garbage collector) deletes anysess_*file whose mtime is older thangc_maxlifetimewithout checking whether another request currently has that file open and locked viaflock(LOCK_EX)-- a lockps_files_open()holds for the entire lifetime of a session.On POSIX,
unlink()on an open file succeeds silently: the directory entry disappears, but the holder's already-open file descriptor keeps working against the now-orphaned inode.session_write_close()therefore reports success while the session data is permanently lost, and neither side ever sees an error.See #25001 for the full writeup, reproduction steps, and reachability analysis (every ordinary
session_start()call runs this sweep internally withgc_probability/gc_divisorodds, so it's reachable without any special access).Changes
ext/session/mod_files.c: before deleting an expired candidate, probe it with a non-blockingflock(LOCK_EX | LOCK_NB); skip it if another process still holds it open instead of unlinking it. Only increment the deletion counter onceunlink()actually succeeds (its return value was previously discarded unconditionally).ext/session/tests/session_gc_active_session_race.phpt: creates a session file, backdates its mtime pastgc_maxlifetime, holds it open+locked via a secondfopen()/flock()handle, runssession_gc()from an unrelated session, and asserts the file -- and its data -- survive.Checklist
Closes #25001