From 3aabacc6cf933d105378c222854ff7f43f570c8d Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Mon, 6 Jul 2026 22:23:56 +0200 Subject: [PATCH 1/4] Fix GH-16027: session close() not called when write fails --- ext/session/mod_user.c | 19 +++++++ ext/session/tests/gh16027.phpt | 45 ++++++++++++++++ ext/session/tests/gh16027_2.phpt | 53 +++++++++++++++++++ ext/session/tests/gh16027_3.phpt | 51 ++++++++++++++++++ .../user_session_module/bug60634_error_1.phpt | 6 +-- .../user_session_module/bug60634_error_2.phpt | 1 + .../session_set_save_handler_class_012.phpt | 2 + 7 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 ext/session/tests/gh16027.phpt create mode 100644 ext/session/tests/gh16027_2.phpt create mode 100644 ext/session/tests/gh16027_3.phpt diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 71b18612683d..3ec227659411 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -17,6 +17,7 @@ #include "php.h" #include "php_session.h" #include "mod_user.h" +#include "zend_exceptions.h" const ps_module ps_mod_user = { PS_MOD_UPDATE_TIMESTAMP(user) @@ -112,6 +113,7 @@ PS_CLOSE_FUNC(user) bool bailout = 0; zval retval; zend_result ret = FAILURE; + zend_object *old_exception = NULL; ZEND_ASSERT(!Z_ISUNDEF(PSF(close))); @@ -120,6 +122,12 @@ PS_CLOSE_FUNC(user) return SUCCESS; } + /* 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))) { + old_exception = EG(exception); + EG(exception) = NULL; + } + zend_try { ps_call_handler(&PSF(close), 0, NULL, &retval); } zend_catch { @@ -128,6 +136,17 @@ PS_CLOSE_FUNC(user) PS(mod_user_implemented) = 0; + if (old_exception) { + if (!EG(exception)) { + EG(exception) = old_exception; + } else if (!zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception))) { + zend_exception_set_previous(EG(exception), old_exception); + } else { + /* close() itself exited/died: that takes precedence, the stashed exception is moot. */ + OBJ_RELEASE(old_exception); + } + } + if (bailout) { if (!Z_ISUNDEF(retval)) { zval_ptr_dtor(&retval); diff --git a/ext/session/tests/gh16027.phpt b/ext/session/tests/gh16027.phpt new file mode 100644 index 000000000000..40a77f573af1 --- /dev/null +++ b/ext/session/tests/gh16027.phpt @@ -0,0 +1,45 @@ +--TEST-- +GH-16027 (Using SessionHandler doesn't always close session file) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- +getMessage(), "\n"; +} + +$file = "$save_path/sess_$id"; +echo "session file exists: "; +var_dump(file_exists($file)); + +/* A leaked lock from close() never being called would make this fail immediately. */ +$fp = fopen($file, 'r+'); +echo "lock acquired after failed write: "; +var_dump(flock($fp, LOCK_EX | LOCK_NB)); +fclose($fp); +?> +--CLEAN-- + +--EXPECT-- +Exception: Serialization of 'Closure' is not allowed +session file exists: bool(true) +lock acquired after failed write: bool(true) diff --git a/ext/session/tests/gh16027_2.phpt b/ext/session/tests/gh16027_2.phpt new file mode 100644 index 000000000000..ca38191c19f8 --- /dev/null +++ b/ext/session/tests/gh16027_2.phpt @@ -0,0 +1,53 @@ +--TEST-- +GH-16027 (close() throwing while a write() exception is pending chains it as previous) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- +getMessage(), "\n"; + $previous = $e->getPrevious(); + echo 'previous: '; + var_dump($previous ? $previous->getMessage() : null); +} +?> +--EXPECT-- +write: goodbye cruel world +close: goodbye cruel world +Exception: close failed +previous: string(12) "write failed" diff --git a/ext/session/tests/gh16027_3.phpt b/ext/session/tests/gh16027_3.phpt new file mode 100644 index 000000000000..c5b7129876df --- /dev/null +++ b/ext/session/tests/gh16027_3.phpt @@ -0,0 +1,51 @@ +--TEST-- +GH-16027 (exit() in close() while a write() exception is pending terminates cleanly) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- +getMessage(), "\n"; +} +echo "unreachable after\n"; +?> +--EXPECT-- +write: goodbye cruel world +close: goodbye cruel world +bye diff --git a/ext/session/tests/user_session_module/bug60634_error_1.phpt b/ext/session/tests/user_session_module/bug60634_error_1.phpt index 98e4d371dd8c..cb6143a1e551 100644 --- a/ext/session/tests/user_session_module/bug60634_error_1.phpt +++ b/ext/session/tests/user_session_module/bug60634_error_1.phpt @@ -43,14 +43,10 @@ session_start(); session_write_close(); echo "um, hi\n"; -/* -FIXME: Something wrong. It should try to close after error, otherwise session -may keep "open" state. -*/ - ?> --EXPECTF-- write: goodbye cruel world +close: goodbye cruel world Fatal error: Uncaught Error: Call to undefined function undefined_function() in %s:%d Stack trace: diff --git a/ext/session/tests/user_session_module/bug60634_error_2.phpt b/ext/session/tests/user_session_module/bug60634_error_2.phpt index 0de1f0b70b67..71a8e50bfb75 100644 --- a/ext/session/tests/user_session_module/bug60634_error_2.phpt +++ b/ext/session/tests/user_session_module/bug60634_error_2.phpt @@ -47,6 +47,7 @@ echo "um, hi\n"; ?> --EXPECTF-- write: goodbye cruel world +close: goodbye cruel world Fatal error: Uncaught Exception in %s Stack trace: diff --git a/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt b/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt index f96206efbb64..a532dff7f821 100644 --- a/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt +++ b/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt @@ -43,6 +43,8 @@ var_dump(session_id(), $oldHandler, ini_get('session.save_handler'), $handler->i --EXPECTF-- *** Testing session_set_save_handler() : incorrect arguments for existing handler open *** Open: + +Warning: SessionHandler::close(): Parent session handler is not open in %s on line %d SessionHandler::open() expects exactly 2 arguments, 0 given Warning: Undefined global variable $_SESSION in %s on line %d From 9c1009bec236ae8b6c38fe0a414460f0669ce8d9 Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Tue, 7 Jul 2026 22:51:31 +0200 Subject: [PATCH 2/4] Use zend_exception_save()/zend_exception_restore() in PS_CLOSE_FUNC(user) --- ext/session/mod_user.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 3ec227659411..ecf76a6cc4f9 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -113,7 +113,6 @@ PS_CLOSE_FUNC(user) bool bailout = 0; zval retval; zend_result ret = FAILURE; - zend_object *old_exception = NULL; ZEND_ASSERT(!Z_ISUNDEF(PSF(close))); @@ -123,9 +122,9 @@ PS_CLOSE_FUNC(user) } /* 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))) { - old_exception = EG(exception); - EG(exception) = NULL; + bool clear_exception = EG(exception) && !zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception)); + if (clear_exception) { + zend_exception_save(); } zend_try { @@ -136,15 +135,8 @@ PS_CLOSE_FUNC(user) PS(mod_user_implemented) = 0; - if (old_exception) { - if (!EG(exception)) { - EG(exception) = old_exception; - } else if (!zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception))) { - zend_exception_set_previous(EG(exception), old_exception); - } else { - /* close() itself exited/died: that takes precedence, the stashed exception is moot. */ - OBJ_RELEASE(old_exception); - } + if (clear_exception) { + zend_exception_restore(); } if (bailout) { From 7cdb3aad1d73f0a3a4cf198ecea3915216886041 Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Wed, 8 Jul 2026 22:16:13 +0200 Subject: [PATCH 3/4] Release the native handler directly instead of manipulating exception 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(). --- ext/session/mod_user.c | 48 +++++++------- ext/session/tests/gh16027_2.phpt | 13 ++-- ext/session/tests/gh16027_3.phpt | 62 +++++++++---------- .../user_session_module/bug60634_error_1.phpt | 1 - .../user_session_module/bug60634_error_2.phpt | 1 - .../session_set_save_handler_class_012.phpt | 2 - 6 files changed, 63 insertions(+), 64 deletions(-) diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index ecf76a6cc4f9..759dba0e6f56 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -17,13 +17,23 @@ #include "php.h" #include "php_session.h" #include "mod_user.h" -#include "zend_exceptions.h" const ps_module ps_mod_user = { PS_MOD_UPDATE_TIMESTAMP(user) }; +/* Releases the native handler SessionHandler wraps when its close() never ran. */ +static void ps_close_default_mod(void) +{ + if (PS(mod_user_is_open) && PS(default_mod)) { + PS(mod_user_is_open) = false; + zend_try { + PS(default_mod)->s_close(&PS(mod_data)); + } zend_end_try(); + } +} + static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) { int i; @@ -33,12 +43,17 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) php_error_docref(NULL, E_WARNING, "Cannot call session save handler in a recursive manner"); } else { PS(in_save_handler) = 1; - if (call_user_function(NULL, NULL, func, retval, argc, argv) == FAILURE) { - zval_ptr_dtor(retval); - ZVAL_UNDEF(retval); - } else if (Z_ISUNDEF_P(retval)) { - ZVAL_NULL(retval); - } + zend_try { + if (call_user_function(NULL, NULL, func, retval, argc, argv) == FAILURE) { + zval_ptr_dtor(retval); + ZVAL_UNDEF(retval); + } else if (Z_ISUNDEF_P(retval)) { + ZVAL_NULL(retval); + } + } zend_catch { + ps_close_default_mod(); + zend_bailout(); + } zend_end_try(); PS(in_save_handler) = 0; } for (i = 0; i < argc; i++) { @@ -48,7 +63,7 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) #define PSF(a) PS(mod_user_names).ps_##a -static zend_result verify_bool_return_type_userland_calls(const zval* value) +static zend_result verify_bool_return_type_userland_calls(const zval *value) { /* Exit or exception in userland call */ if (Z_TYPE_P(value) == IS_UNDEF) { @@ -76,8 +91,8 @@ static zend_result verify_bool_return_type_userland_calls(const zval* value) } if (!EG(exception)) { zend_type_error("Session callback must have a return value of type bool, %s returned", zend_zval_value_name(value)); \ - } - return FAILURE; + } + return FAILURE; } PS_OPEN_FUNC(user) @@ -121,23 +136,14 @@ PS_CLOSE_FUNC(user) return SUCCESS; } - /* Run close() even with a pending exception, so the handler releases its resources; skip real exit()/die() (bug #60634). */ - bool clear_exception = EG(exception) && !zend_is_unwind_exit(EG(exception)) && !zend_is_graceful_exit(EG(exception)); - if (clear_exception) { - zend_exception_save(); - } - zend_try { ps_call_handler(&PSF(close), 0, NULL, &retval); } zend_catch { bailout = 1; } zend_end_try(); - PS(mod_user_implemented) = 0; - - if (clear_exception) { - zend_exception_restore(); - } + PS(mod_user_implemented) = false; + ps_close_default_mod(); if (bailout) { if (!Z_ISUNDEF(retval)) { diff --git a/ext/session/tests/gh16027_2.phpt b/ext/session/tests/gh16027_2.phpt index ca38191c19f8..dc237af689a5 100644 --- a/ext/session/tests/gh16027_2.phpt +++ b/ext/session/tests/gh16027_2.phpt @@ -1,5 +1,5 @@ --TEST-- -GH-16027 (close() throwing while a write() exception is pending chains it as previous) +GH-16027 (plain SessionHandlerInterface: close() stays unreachable under a pending write() exception) --EXTENSIONS-- session --SKIPIF-- @@ -13,7 +13,7 @@ class MySessionHandler implements SessionHandlerInterface { function close(): bool { echo "close: goodbye cruel world\n"; - throw new Exception('close failed'); + return true; } function read($id): string|false { @@ -41,13 +41,10 @@ try { session_write_close(); } catch (\Throwable $e) { echo $e::class, ': ', $e->getMessage(), "\n"; - $previous = $e->getPrevious(); - echo 'previous: '; - var_dump($previous ? $previous->getMessage() : null); + var_dump($e->getPrevious()); } ?> --EXPECT-- write: goodbye cruel world -close: goodbye cruel world -Exception: close failed -previous: string(12) "write failed" +Exception: write failed +NULL diff --git a/ext/session/tests/gh16027_3.phpt b/ext/session/tests/gh16027_3.phpt index c5b7129876df..bb5daa9bf66f 100644 --- a/ext/session/tests/gh16027_3.phpt +++ b/ext/session/tests/gh16027_3.phpt @@ -1,51 +1,51 @@ --TEST-- -GH-16027 (exit() in close() while a write() exception is pending terminates cleanly) +GH-16027 (SessionHandler subclass throwing inside close() still releases the session file lock) --EXTENSIONS-- session --SKIPIF-- --FILE-- getMessage(), "\n"; + echo $e::class, ': ', $e->getMessage(), "\n"; } -echo "unreachable after\n"; + +$file = "$save_path/sess_$id"; +echo "session file exists: "; +var_dump(file_exists($file)); + +/* close() threw before ever calling parent::close(); the lock must still be released. */ +$fp = fopen($file, 'r+'); +echo "lock acquired after close() threw: "; +var_dump(flock($fp, LOCK_EX | LOCK_NB)); +fclose($fp); +?> +--CLEAN-- + --EXPECT-- -write: goodbye cruel world -close: goodbye cruel world -bye +Exception: close blew up before delegating +session file exists: bool(true) +lock acquired after close() threw: bool(true) diff --git a/ext/session/tests/user_session_module/bug60634_error_1.phpt b/ext/session/tests/user_session_module/bug60634_error_1.phpt index cb6143a1e551..d27d2d34b37d 100644 --- a/ext/session/tests/user_session_module/bug60634_error_1.phpt +++ b/ext/session/tests/user_session_module/bug60634_error_1.phpt @@ -46,7 +46,6 @@ echo "um, hi\n"; ?> --EXPECTF-- write: goodbye cruel world -close: goodbye cruel world Fatal error: Uncaught Error: Call to undefined function undefined_function() in %s:%d Stack trace: diff --git a/ext/session/tests/user_session_module/bug60634_error_2.phpt b/ext/session/tests/user_session_module/bug60634_error_2.phpt index 71a8e50bfb75..0de1f0b70b67 100644 --- a/ext/session/tests/user_session_module/bug60634_error_2.phpt +++ b/ext/session/tests/user_session_module/bug60634_error_2.phpt @@ -47,7 +47,6 @@ echo "um, hi\n"; ?> --EXPECTF-- write: goodbye cruel world -close: goodbye cruel world Fatal error: Uncaught Exception in %s Stack trace: diff --git a/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt b/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt index a532dff7f821..f96206efbb64 100644 --- a/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt +++ b/ext/session/tests/user_session_module/session_set_save_handler_class_012.phpt @@ -43,8 +43,6 @@ var_dump(session_id(), $oldHandler, ini_get('session.save_handler'), $handler->i --EXPECTF-- *** Testing session_set_save_handler() : incorrect arguments for existing handler open *** Open: - -Warning: SessionHandler::close(): Parent session handler is not open in %s on line %d SessionHandler::open() expects exactly 2 arguments, 0 given Warning: Undefined global variable $_SESSION in %s on line %d From 8cf831cdd5c8c903715f32f7438fa931613a112f Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Wed, 8 Jul 2026 22:51:54 +0200 Subject: [PATCH 4/4] Revert incidental formatting changes and drop redundant gh16027 test 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 GH-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. --- ext/session/mod_user.c | 4 +-- ext/session/tests/gh16027_2.phpt | 59 ++++++++++++++++---------------- ext/session/tests/gh16027_3.phpt | 51 --------------------------- 3 files changed, 32 insertions(+), 82 deletions(-) delete mode 100644 ext/session/tests/gh16027_3.phpt diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c index 759dba0e6f56..5f98f4169b3c 100644 --- a/ext/session/mod_user.c +++ b/ext/session/mod_user.c @@ -63,7 +63,7 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval) #define PSF(a) PS(mod_user_names).ps_##a -static zend_result verify_bool_return_type_userland_calls(const zval *value) +static zend_result verify_bool_return_type_userland_calls(const zval* value) { /* Exit or exception in userland call */ if (Z_TYPE_P(value) == IS_UNDEF) { @@ -142,7 +142,7 @@ PS_CLOSE_FUNC(user) bailout = 1; } zend_end_try(); - PS(mod_user_implemented) = false; + PS(mod_user_implemented) = 0; ps_close_default_mod(); if (bailout) { diff --git a/ext/session/tests/gh16027_2.phpt b/ext/session/tests/gh16027_2.phpt index dc237af689a5..223485005bdd 100644 --- a/ext/session/tests/gh16027_2.phpt +++ b/ext/session/tests/gh16027_2.phpt @@ -1,50 +1,51 @@ --TEST-- -GH-16027 (plain SessionHandlerInterface: close() stays unreachable under a pending write() exception) +GH-16027 (SessionHandler subclass throwing inside close() still releases the session file lock) --EXTENSIONS-- session --SKIPIF-- --FILE-- getMessage(), "\n"; - var_dump($e->getPrevious()); } + +$file = "$save_path/sess_$id"; +echo "session file exists: "; +var_dump(file_exists($file)); + +/* close() threw before ever calling parent::close(); the lock must still be released. */ +$fp = fopen($file, 'r+'); +echo "lock acquired after close() threw: "; +var_dump(flock($fp, LOCK_EX | LOCK_NB)); +fclose($fp); +?> +--CLEAN-- + --EXPECT-- -write: goodbye cruel world -Exception: write failed -NULL +Exception: close blew up before delegating +session file exists: bool(true) +lock acquired after close() threw: bool(true) diff --git a/ext/session/tests/gh16027_3.phpt b/ext/session/tests/gh16027_3.phpt deleted file mode 100644 index bb5daa9bf66f..000000000000 --- a/ext/session/tests/gh16027_3.phpt +++ /dev/null @@ -1,51 +0,0 @@ ---TEST-- -GH-16027 (SessionHandler subclass throwing inside close() still releases the session file lock) ---EXTENSIONS-- -session ---SKIPIF-- - ---FILE-- -getMessage(), "\n"; -} - -$file = "$save_path/sess_$id"; -echo "session file exists: "; -var_dump(file_exists($file)); - -/* close() threw before ever calling parent::close(); the lock must still be released. */ -$fp = fopen($file, 'r+'); -echo "lock acquired after close() threw: "; -var_dump(flock($fp, LOCK_EX | LOCK_NB)); -fclose($fp); -?> ---CLEAN-- - ---EXPECT-- -Exception: close blew up before delegating -session file exists: bool(true) -lock acquired after close() threw: bool(true)