ext/session: hold the save handler recursion guard for the whole call (stacked on GH-22926) - #200
Open
iliaal wants to merge 2 commits into
Open
ext/session: hold the save handler recursion guard for the whole call (stacked on GH-22926)#200iliaal wants to merge 2 commits into
iliaal wants to merge 2 commits into
Conversation
session_regenerate_id() runs the userland write or destroy handler and then the close handler, and keeps operating on PS(id) afterwards. Any of them may call session_destroy(): php_session_destroy() runs php_rshutdown_session_globals() even when the recursive call was rejected, releasing PS(id) and setting it to NULL, so the following zend_string_release_ex() dereferences NULL. Check that the session is still active once every handler has run. Closes phpGH-22926
ps_call_handler() cleared PS(in_save_handler) on the branch that rejects a recursive call, so the guard was released by the frame that was refused rather than by the frame that set it. One rejected nested call disarmed it and the next nested call from the same handler re-entered userland. Holding it exposes a second defect: php_rshutdown_session_globals() can no longer run SessionHandler::close(), and PS(default_mod)'s data is orphaned once PS(mod_data) is cleared. Release it there, which also covers a handler that never delegates close() to its parent.
iliaal
force-pushed
the
fix/session-recursion-guard-ownership
branch
from
July 29, 2026 18:14
634d86e to
f967d5e
Compare
iliaal
changed the title
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
1 participant
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.
Stacked on php#22926, which is the first of the two commits here. Its test expectations change once the guard actually holds, so the two cannot land separately.
ps_call_handler()clearedPS(in_save_handler)on the branch that rejects a recursive call, so the guard was released by the frame that was refused rather than by the frame that set it. One rejected nested call disarms it and the next one re-enters userland. Twosession_gc()calls from inside awrite()handler show it, the first correctly refused, the second runninggc(). phpGH-22382 moved the assignment but kept it, so the defect predates that commit.Holding the guard stops
php_rshutdown_session_globals()from runningSessionHandler::close(), which orphansPS(default_mod)'s data oncePS(mod_data)is cleared, so the release moves there. That also covers a leak needing no recursion at all: a SessionHandler subclass overridingclose()without delegating to the parent leaks 3 allocations per request on stock PHP-8.4, including theps_filesstruct.Three existing expectations gain a "Cannot call session save handler in a recursive manner" line where the nested
s_closeused to run silently. Their assertions are unchanged.