diff --git a/NEWS b/NEWS index cd9ddd3994cc..7cac87ee347a 100644 --- a/NEWS +++ b/NEWS @@ -113,6 +113,14 @@ 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. 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 2e2d798431d4..bb758e97b063 100644 --- a/ext/session/mod_files.c +++ b/ext/session/mod_files.c @@ -324,8 +324,53 @@ 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. 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 + * 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..f2ea19ebf708 --- /dev/null +++ b/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-- + +--FILE-- + +--EXPECT-- +bool(true) +array(1) { + ["secret"]=> + string(12) "still-in-use" +}