ext/session: Do not GC a session file another request still holds locked

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
+    (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

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 */

ext/session/tests/session_gc_active_session_race.phpt

@@ -0,0 +1,73 @@
+--TEST--
+session_gc() must not delete a session file another request still has locked
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+if (substr(PHP_OS, 0, 3) === 'WIN') die('skip flock()/unlink() interaction differs on Windows');
+?>
+--FILE--
+<?php
+$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);
+// 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
+// (large upload, slow payment callback, ...) is running.
+$victimId = 'victimxxxxxxxxxxxxxxxxxxxxxxxxx';
+session_id($victimId);
+session_start();
+$_SESSION['secret'] = 'still-in-use';
+session_write_close();
+
+$victimFile = $save_path . '/sess_' . $victimId;
+// Make it look idle long enough to be GC-eligible...
+touch($victimFile, time() - 100000);
+// ...while keeping it open and locked, simulating the owning request still
+// being alive.
+$lockHandle = fopen($victimFile, 'r+');
+flock($lockHandle, LOCK_EX);
+
+// Forcing GC only requires an unrelated session of one's own.
+session_id('attackerxxxxxxxxxxxxxxxxxxxxxxxx');
+session_start();
+session_gc();
+session_write_close();
+
+$survived_while_locked = file_exists($victimFile);
+
+flock($lockHandle, LOCK_UN);
+fclose($lockHandle);
+
+// The data must still be there once nobody holds the lock any more.
+session_id($victimId);
+session_start();
+$restored_session = $_SESSION;
+session_write_close();
+
+var_dump($survived_while_locked);
+var_dump($restored_session);
+?>
+--EXPECT--
+bool(true)
+array(1) {
+  ["secret"]=>
+  string(12) "still-in-use"
+}