← 返回首页
ext/session: Fix GH-16027 session close() not called when write fails by jorgsowa · Pull Request #22622 · php/php-src · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

ext/session: Fix GH-16027 session close() not called when write fails#22622

Draft
jorgsowa wants to merge 4 commits into
php:PHP-8.4from
jorgsowa:fix-gh16027-session-close-on-exception-8.4
Draft

ext/session: Fix GH-16027 session close() not called when write fails#22622
jorgsowa wants to merge 4 commits into
php:PHP-8.4from
jorgsowa:fix-gh16027-session-close-on-exception-8.4

Conversation

jorgsowa commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #16027

jorgsowa changed the base branch from master to PHP-8.4 July 6, 2026 21:14
Comment thread ext/session/mod_user.c Outdated
}

/* Run close() even with a pending exception, so the handler releases its resources; skip real exit()/die() (bug #60634). */
if (EG(exception) && !zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

This dance is never a good idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

When I use zend_exception_save()/zend_exception_restore() there is a test failing with the message Deprecated: Creation of dynamic property UnwindExit::$previous is deprecated. Should I fix those helpers and use them? Or is there another smart way for serving those exceptions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I believe zend_exception_save is even gone on master. But my point is that maybe close shouldn't be called anywya

Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I don't think running usercode while an exception hasn't been caught is sensible.
This whole saving and restoring exceptions functions have been removed in master because it is a bad idea.

This just seems like a user error where an exception shouldn't be thrown in the handler functions. As I don't see why this problem can't happen if you throw an exception on one of the other handlers (e.g. read).

… state in PS_CLOSE_FUNC(user) Replaces the zend_exception_save()/zend_exception_restore() with ps_close_default_mod(), which releases SessionHandler's wrapped native handler directly whenever its close() never ran, without touching EG(exception). This also covers a bailout during any userland session callback, not just close().

Girgias commented Jul 8, 2026

Copy link
Copy Markdown
Member

I don't think running usercode while an exception hasn't been caught is sensible.

I don't see how your new code still doesn't do exactly that. We can't fix users writing bogus code.

verify_bool_return_type_userland_calls()'s signature/indentation and PS(mod_user_implemented)'s assignment style are reverted to match master, since they were unrelated to the phpGH-16027 fix. gh16027_2.phpt (write() exception leaves close() unreachable) passes identically on master and on this branch, so it doesn't exercise the fix; it's replaced by the former gh16027_3.phpt, renamed to _2.

jorgsowa commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

We can't fix users writing bogus code.

But we can fix leaking locks of files in the built-in SessionHandler. I think it's a sound middle ground for this bug.

I don't see any other alternatives for this bug than:

  1. Reject bug report
  2. Make SessionHandler final, but it still doesn't fix the code from the bug report
session_set_save_handler(new \SessionHandler, true); session_start([ 'save_path' => __DIR__, ]); $_SESSION['test'] = function () {};

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using SessionHandler doesn't always close session file

3 participants

Footer

© 2026 GitHub, Inc.