[AsahiSoftWareEngineer]

Description

ps_files_cleanup_dir() (the files session save handler's garbage collector) deletes any sess_* file whose mtime is older than gc_maxlifetime without checking whether another request currently has that file open and locked via flock(LOCK_EX) -- a lock ps_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 with gc_probability/gc_divisor odds, so it's reachable without any special access).

Changes

• ext/session/mod_files.c: before deleting an expired candidate, probe it with a non-blocking flock(LOCK_EX | LOCK_NB); skip it if another process still holds it open instead of unlinking it. Only increment the deletion counter once unlink() actually succeeds (its return value was previously discarded unconditionally).
• Added ext/session/tests/session_gc_active_session_race.phpt: creates a session file, backdates its mtime past gc_maxlifetime, holds it open+locked via a second fopen()/flock() handle, runs session_gc() from an unrelated session, and asserts the file -- and its data -- survive.
• NEWS entry.

Checklist

• Regression test included
• NEWS entry added
• No existing test coverage needed updating (new behavior is additive: previously-untested concurrent-access case)

Closes #25001

[anun333]

Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.

This fixes the issue's case. Debug builds of today's master (125ffe30167) with and without this PR, session.gc_maxlifetime=60, a second request running GC (gc_probability=1, gc_divisor=1) at the moments below; what is in each session file afterwards:

Session file	master	this PR
held by a running request (3 s), mtime 1 h old, GC runs meanwhile	gone: the request's write went to the unlinked file	written by that request
expired, not held (control)	gone	gone
fresh, not held (control)	kept	kept
expired, not held, mode 0400	gone	kept
expired, resumed by the request that runs GC	gone	kept, written by that request

ext/session/tests passes on both (267, 268 with the new test), and session_gc_active_session_race.phpt fails on master without the fix.

Two rows change besides the one the PR is about, so worth a check that they're intended:

• A file GC can't open read-write (owned by another user in a shared, non-sticky directory, say, or mode 0400 as here) is now never collected, where unlink() used to remove it. If that's not wanted, falling back to the old unlink() when open() fails would keep the earlier cleanup for those files.
• A request that resumes its own expired session now keeps it: its own flock() from ps_files_open() makes the probe fail. Before, its own GC pass deleted the file under it and the data it wrote at the end was lost. Keeping it reads like the better outcome, but it changes what gc_maxlifetime does for a client that comes back after it, so maybe a line in NEWS.

And from reading the code, not tested: the lock is released and the file closed before the unlink(), so a request that opens and locks the same file in that gap ends up with the unlinked file, the same loss in a much smaller window. On POSIX, unlinking while still holding the lock, then closing, would close that gap.

How this was tested

Linux x86_64, ext4, gcc 13.3, debug builds of origin/master and of it with this PR's commits cherry-picked:

./buildconf --force
./configure --enable-debug --disable-cgi --disable-phpdbg --disable-fileinfo --enable-pcntl --enable-zend-test --enable-fpm --with-curl --enable-mbstring --disable-mbregex --with-mysqli --with-pdo-mysql
make -j4

Each case runs sapi/cli/php -n -d session.save_path=$DIR -d session.use_cookies=0 -d session.use_strict_mode=0 -d session.gc_maxlifetime=60. The held case: sess_busy is written and aged with touch -d '-1 hour'; one process does session_id("busy"); session_start(); sleep(3); $_SESSION["v"] = "written by the busy request"; with gc_probability=0, and a second later another, with gc_probability=1 gc_divisor=1, does session_id("gcrunner"); session_start(); session_write_close();. The other rows: files written with printf, aged with touch -d '-1 hour' (and chmod 0400 for that row), then the same GC run; the last row runs GC from the request that resumes sess_own. Suites: TEST_PHP_EXECUTABLE=$PWD/sapi/cli/php sapi/cli/php run-tests.php -n -q -j4 ext/session/tests on both.

[AsahiSoftWareEngineer]

Thanks for the detailed verification and the three follow-up points.

Adopted the lock-held-unlink reorder: ps_files_cleanup_dir() now does VCWD_UNLINK(buf) while still holding the probe's flock(LOCK_EX), then unlocks and closes. A concurrent open() on the same path now blocks in its own flock(LOCK_EX) for as long as the probe holds it, so it can only acquire the lock once the unlink has already happened, not while the file is still live.

To be precise about what this does and doesn't fix: if that concurrent request's open() already completed before our unlink (it only needs open(), not the lock, to have happened first), it's still holding a valid fd on the now-unlinked inode once its flock() unblocks, and whatever it writes afterward is lost the same way. Closing that fully needs the open side (ps_files_open()) to detect after acquiring its lock that the file was unlinked out from under it (e.g. via fstat() st_nlink) and retry -- a separate, larger change to the open path rather than something this function can do on its own. Left that as a documented residual limitation in the comment rather than something this PR claims to fully solve.

On the two other points:

• Files GC can no longer open read-write (foreign ownership, 0400, etc.) are left uncollected going forward rather than falling back to the old unconditional unlink(). Keeping that as-is: falling back would reopen the same kind of silent-destruction risk for exactly the files where we have the least information about who might be using them, which cuts against the point of this PR.
• Added a NEWS line: 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. That's a real behavior change for what gc_maxlifetime does on a client's own return visit, now documented.

Pushed as a new commit; ext/session/tests (267/267, including session_gc_active_session_race.phpt) still passes.