From 30673392a6a5da92c9574766e6a7f0d9a2e468bc Mon Sep 17 00:00:00 2001 From: Asahi Date: Wed, 28 Oct 2026 19:53:39 +0900 Subject: [PATCH 1/5] ext/session: Do not GC a session file another request still holds locked 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 --- NEWS | 4 ++ ext/session/mod_files.c | 32 ++++++++++- .../tests/session_gc_active_session_race.phpt | 56 +++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 ext/session/tests/session_gc_active_session_race.phpt diff --git a/NEWS b/NEWS index cd9ddd3994cc..966a003a1046 100644 --- a/NEWS +++ b/NEWS @@ -113,6 +113,10 @@ PHP NEWS set to an empty string. (Ilia Alshanetsky) - Session: + . Fixed the files save handler's garbage collector deleting a session file + that another request still holds open and locked, silently destroying + its data without either side seeing an error. (reported and fixed by + [reporter name]) . Fixed session_start() continuing after a failed create_sid() when session.use_strict_mode rejects the supplied ID. (Ilia Alshanetsky) . Fixed exceptions from user-defined create_sid() handlers being replaced diff --git a/ext/session/mod_files.c b/ext/session/mod_files.c index 2e2d798431d4..7bf3f71333b3 100644 --- a/ext/session/mod_files.c +++ b/ext/session/mod_files.c @@ -324,8 +324,36 @@ static int ps_files_cleanup_dir(const zend_string *dirname, zend_long maxlifetim } time(&now); if ((now - sbuf.st_mtime) > maxlifetime) { - VCWD_UNLINK(buf); - nrdels++; + /* ps_files_open() holds flock(LOCK_EX) on a session file for + * as long as the owning request is active. Unlinking a file + * while another process still has it open succeeds silently + * on POSIX (the directory entry disappears, but the holder's + * already-open fd keeps working against the now-orphaned + * inode), so a session actively in use can be destroyed + * out from under its owner without either side ever seeing + * an error. Probe for that with a non-blocking lock attempt + * before deleting, and skip files currently in use instead + * of unlinking them. */ + int fd = VCWD_OPEN(buf, O_RDWR); + if (fd < 0) { + /* Can no longer be opened (already removed, permissions + * changed, etc.); nothing to delete or protect. */ + continue; + } + if (flock(fd, LOCK_EX | LOCK_NB) == 0) { + /* We obtained the lock: no other request currently has + * this session open. Safe to remove. */ + flock(fd, LOCK_UN); + close(fd); + if (VCWD_UNLINK(buf) == 0) { + nrdels++; + } + } else { + /* Another request holds the lock; this session is + * actively in use despite its old mtime. Leave it for a + * future GC pass instead of destroying live data. */ + close(fd); + } } } else { /* intermediate depth: recurse into subdirectories */ diff --git a/ext/session/tests/session_gc_active_session_race.phpt b/ext/session/tests/session_gc_active_session_race.phpt new file mode 100644 index 000000000000..ce6f511bc604 --- /dev/null +++ b/ext/session/tests/session_gc_active_session_race.phpt @@ -0,0 +1,56 @@ +--TEST-- +session_gc() must not delete a session file another request still has locked +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) +array(1) { + ["secret"]=> + string(12) "still-in-use" +} From dc80fb80cc54399bccecd2c863cd1314dd8a475f Mon Sep 17 00:00:00 2001 From: Asahi Date: Wed, 28 Oct 2026 23:55:11 +0900 Subject: [PATCH 2/5] NEWS: fill in reporter attribution placeholder Co-Authored-By: Claude Sonnet 5 --- NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 966a003a1046..b4a09a1fc105 100644 --- a/NEWS +++ b/NEWS @@ -116,7 +116,7 @@ PHP NEWS . Fixed the files save handler's garbage collector deleting a session file that another request still holds open and locked, silently destroying its data without either side seeing an error. (reported and fixed by - [reporter name]) + (Asahi) . Fixed session_start() continuing after a failed create_sid() when session.use_strict_mode rejects the supplied ID. (Ilia Alshanetsky) . Fixed exceptions from user-defined create_sid() handlers being replaced From f4ef44ef44b4f2890ad1233d30136afd3241ec9e Mon Sep 17 00:00:00 2001 From: Asahi Date: Thu, 29 Oct 2026 00:19:23 +0900 Subject: [PATCH 3/5] tests: fix session_gc_active_session_race.phpt for CLI header-sent rules 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 --- .../tests/session_gc_active_session_race.phpt | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/ext/session/tests/session_gc_active_session_race.phpt b/ext/session/tests/session_gc_active_session_race.phpt index ce6f511bc604..5d424cd1ab23 100644 --- a/ext/session/tests/session_gc_active_session_race.phpt +++ b/ext/session/tests/session_gc_active_session_race.phpt @@ -12,6 +12,16 @@ $save_path = sys_get_temp_dir() . '/' . uniqid('ps_gc_lock_test_', true); mkdir($save_path); ini_set('session.save_path', $save_path); ini_set('session.save_handler', 'files'); +// This single script calls session_start() several times. With cookies and +// the cache limiter enabled, session_start() refuses to run once any +// output has already been sent, so disable both and defer all var_dump() +// output to the very end of the script. +ini_set('session.use_cookies', 0); +ini_set('session.cache_limiter', ''); +// gc_maxlifetime cannot be changed once a session is active, so set it +// before the first session_start() call; it is irrelevant to the "victim" +// session below, only to the "attacker" one further down. +ini_set('session.gc_maxlifetime', 1); // The "victim": a session file a still-active request has open+locked, the // way ps_files_open() holds flock(LOCK_EX) for as long as a slow request @@ -33,11 +43,10 @@ flock($lockHandle, LOCK_EX); // Forcing GC only requires an unrelated session of one's own. session_id('attackerxxxxxxxxxxxxxxxxxxxxxxxx'); session_start(); -ini_set('session.gc_maxlifetime', 1); session_gc(); session_write_close(); -var_dump(file_exists($victimFile)); +$survived_while_locked = file_exists($victimFile); flock($lockHandle, LOCK_UN); fclose($lockHandle); @@ -45,8 +54,11 @@ fclose($lockHandle); // The data must still be there once nobody holds the lock any more. session_id($victimId); session_start(); -var_dump($_SESSION); +$restored_session = $_SESSION; session_write_close(); + +var_dump($survived_while_locked); +var_dump($restored_session); ?> --EXPECT-- bool(true) From 38150074696696b1648061cdcf0cd47419eaad74 Mon Sep 17 00:00:00 2001 From: Asahi Date: Thu, 29 Oct 2026 07:49:06 +0900 Subject: [PATCH 4/5] tests: disable session.use_strict_mode in session_gc_active_session_race.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 --- ext/session/tests/session_gc_active_session_race.phpt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ext/session/tests/session_gc_active_session_race.phpt b/ext/session/tests/session_gc_active_session_race.phpt index 5d424cd1ab23..f2ea19ebf708 100644 --- a/ext/session/tests/session_gc_active_session_race.phpt +++ b/ext/session/tests/session_gc_active_session_race.phpt @@ -22,6 +22,11 @@ ini_set('session.cache_limiter', ''); // before the first session_start() call; it is irrelevant to the "victim" // session below, only to the "attacker" one further down. ini_set('session.gc_maxlifetime', 1); +// session.use_strict_mode (default on) rejects a caller-supplied ID that +// does not already have a session file and silently replaces it with a +// freshly generated one. This test depends on session_id() actually +// pinning $victimId/'attackerxxx...', so strict mode must be off. +ini_set('session.use_strict_mode', 0); // The "victim": a session file a still-active request has open+locked, the // way ps_files_open() holds flock(LOCK_EX) for as long as a slow request From f75f76d2430b0544c189d46ca550daffcb617053 Mon Sep 17 00:00:00 2001 From: Asahi Date: Thu, 29 Oct 2026 22:25:18 +0900 Subject: [PATCH 5/5] ext/session: unlink expired session file while still holding its lock The previous ordering released the probe lock and closed the fd before unlinking: flock(LOCK_UN); close(fd); VCWD_UNLINK(buf). Between the unlock and the unlink, a request that calls open() on the same path can acquire flock(LOCK_EX) immediately (nothing holds it at that point) and proceed as a normal, fully successful session open, only to have its file unlinked out from under it moments later by this function -- the same data loss this function exists to prevent, just in a much smaller window, as pointed out in review on GH-25002. Unlinking while the probe's own lock is still held closes most of that window: a concurrent open() on the same path now blocks in its own flock(LOCK_EX) call for as long as we hold the lock, so it can only acquire it once the unlink has already happened, never while the file is still live. It does not fully close it -- if that request's open() already completed before our unlink(), it is still holding a valid fd on the now-unlinked inode once its flock() unblocks, and whatever it writes afterward is still lost. Closing that requires the open side (ps_files_open()) to detect it was unlinked out from under it after acquiring the lock and retry, which is a larger, separate change; documented in the comment as a known residual limitation instead of silently leaving it unaddressed. Also note in NEWS that a request resuming its own already-expired session during the GC pass that would have collected it now keeps that session instead of losing it, since its own flock() makes the probe fail -- a visible behavior change from the first commit on this branch, not from this one, but previously undocumented. Co-Authored-By: Claude Sonnet 5 --- NEWS | 8 ++++++-- ext/session/mod_files.c | 23 ++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index b4a09a1fc105..7cac87ee347a 100644 --- a/NEWS +++ b/NEWS @@ -115,8 +115,12 @@ PHP NEWS - Session: . Fixed the files save handler's garbage collector deleting a session file that another request still holds open and locked, silently destroying - its data without either side seeing an error. (reported and fixed by - (Asahi) + its data without either side seeing an error. As a side effect, a + request that resumes its own already-expired session during the same + GC pass that would have collected it now keeps that session instead of + losing it; previously, gc_maxlifetime could cause a client's own + request to destroy the very session data it was about to write. + (reported and fixed by Asahi) . Fixed session_start() continuing after a failed create_sid() when session.use_strict_mode rejects the supplied ID. (Ilia Alshanetsky) . Fixed exceptions from user-defined create_sid() handlers being replaced diff --git a/ext/session/mod_files.c b/ext/session/mod_files.c index 7bf3f71333b3..bb758e97b063 100644 --- a/ext/session/mod_files.c +++ b/ext/session/mod_files.c @@ -342,12 +342,29 @@ static int ps_files_cleanup_dir(const zend_string *dirname, zend_long maxlifetim } if (flock(fd, LOCK_EX | LOCK_NB) == 0) { /* We obtained the lock: no other request currently has - * this session open. Safe to remove. */ - flock(fd, LOCK_UN); - close(fd); + * this session open. Unlink while still holding it, + * instead of unlocking first: a request that already + * called open() on this same path is blocked in its own + * flock(LOCK_EX) call until we release, so it can only + * acquire the lock after the unlink below has already + * happened, never on a still-linked file we are about + * to remove out from under it. + * + * This does not fully close the race: that request's + * open() happened before our unlink(), so once its + * flock() unblocks it is still holding a valid fd on + * the now-unlinked inode, and whatever it writes is + * lost the same way as the bug this function guards + * against, just in a far narrower window. Closing that + * last gap needs the open side (ps_files_open()) to + * notice its file was unlinked out from under it after + * acquiring the lock (e.g. via fstat() st_nlink) and + * retry; this function cannot do that from here. */ if (VCWD_UNLINK(buf) == 0) { nrdels++; } + flock(fd, LOCK_UN); + close(fd); } else { /* Another request holds the lock; this session is * actively in use despite its old mtime. Leave it for a