From 9143dfb0b490c2b6969466d6580408cbe1eb9cd8 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 4 Aug 2026 22:18:52 -0400 Subject: [PATCH] Close session save handler after encoding failures php_session_save_current_state() returned before s_close() when session encoding returned NULL. It also could not invoke a user close handler while an encoding exception was pending. Either path could leave a save handler and file lock open. Continue to s_close() after encoding failures. Protect pending encoding exceptions while invoking the close handler, then restore or chain them after cleanup. Avoid the redundant generic write-failure warning when encoding already reported the failure. --- NEWS | 4 + ext/session/session.c | 59 +++++++++++---- ...session_encode_exception_still_closes.phpt | 75 +++++++++++++++++++ .../session_encode_fail_still_closes.phpt | 62 +++++++++++++++ 4 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 ext/session/tests/session_encode_exception_still_closes.phpt create mode 100644 ext/session/tests/session_encode_fail_still_closes.phpt diff --git a/NEWS b/NEWS index 16a4ae4e99b8..2147ba0fefd5 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,10 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| ?? ??? ????, PHP 8.6.0beta1 +- Session: + . Fixed session encode failures not closing save handlers, which could leave + session file locks held. (iliaal) + - Core: . Changed run-tests.php to run test subprocesses without a shell where possible. (NickSdot) diff --git a/ext/session/session.c b/ext/session/session.c index dd968d453bda..67f30ad719e5 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -44,6 +44,7 @@ #include "ext/standard/head.h" #include "ext/random/php_random.h" #include "ext/random/php_random_csprng.h" +#include "zend_exceptions.h" #include "mod_files.h" #include "mod_user.h" @@ -511,6 +512,9 @@ static zend_result php_session_initialize(void) static void php_session_save_current_state(bool write) { zend_result ret = FAILURE; + bool encode_failed = false; + zend_object *saved_exception = NULL; + const zend_op *saved_opline_before_exception = NULL; if (write) { IF_SESSION_VARS() { @@ -519,24 +523,39 @@ static void php_session_save_current_state(bool write) zend_string *val = php_session_encode(); /* Not being able to encode the session data means there is some kind of issue that prevents a write * (e.g. a key containing the '|' character with the default serialization) */ - if (UNEXPECTED(val == NULL)) { - return; - } - - if (PS(lazy_write) && PS(session_vars) - && PS(mod)->s_update_timestamp - && PS(mod)->s_update_timestamp != php_session_update_timestamp - && zend_string_equals(val, PS(session_vars)) - ) { - ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime)); - handler_function = &PS(mod_user_names).ps_update_timestamp; + if (UNEXPECTED(EG(exception))) { + encode_failed = true; + if (EG(current_execute_data)) { + if (EG(current_execute_data)->func + && ZEND_USER_CODE(EG(current_execute_data)->func->common.type)) { + zend_rethrow_exception(EG(current_execute_data)); + } + EG(current_execute_data)->opline = EG(opline_before_exception); + saved_opline_before_exception = EG(opline_before_exception); + } + saved_exception = EG(exception); + EG(exception) = NULL; + if (val) { + zend_string_release_ex(val, false); + } + } else if (EXPECTED(val != NULL)) { + if (PS(lazy_write) && PS(session_vars) + && PS(mod)->s_update_timestamp + && PS(mod)->s_update_timestamp != php_session_update_timestamp + && zend_string_equals(val, PS(session_vars)) + ) { + ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime)); + handler_function = &PS(mod_user_names).ps_update_timestamp; + } else { + ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime)); + } + zend_string_release_ex(val, false); } else { - ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime)); + encode_failed = true; } - zend_string_release_ex(val, false); } - if ((ret == FAILURE) && !EG(exception)) { + if ((ret == FAILURE) && !EG(exception) && !encode_failed) { if (!PS(mod_user_implemented)) { php_error_docref(NULL, E_WARNING, "Failed to write session data (%s). Please " "verify that the current setting of session.save_path " @@ -557,6 +576,18 @@ static void php_session_save_current_state(bool write) if (PS(mod_data) || PS(mod_user_implemented)) { PS(mod)->s_close(&PS(mod_data)); } + + if (saved_exception) { + if (EG(current_execute_data)) { + EG(current_execute_data)->opline = EG(exception_op); + EG(opline_before_exception) = saved_opline_before_exception; + } + if (EG(exception)) { + zend_exception_set_previous(EG(exception), saved_exception); + } else { + EG(exception) = saved_exception; + } + } } static void php_session_normalize_vars(void) diff --git a/ext/session/tests/session_encode_exception_still_closes.phpt b/ext/session/tests/session_encode_exception_still_closes.phpt new file mode 100644 index 000000000000..2a1e349de164 --- /dev/null +++ b/ext/session/tests/session_encode_exception_still_closes.phpt @@ -0,0 +1,75 @@ +--TEST-- +Encode exception must still close the session save handler +--EXTENSIONS-- +session +--INI-- +session.use_cookies=0 +session.cache_limiter= +session.gc_probability=0 +--FILE-- +closed = true; + return true; + } + + public function read(string $id): string|false + { + return ''; + } + + public function write(string $id, string $data): bool + { + $this->written = true; + return true; + } + + public function destroy(string $id): bool + { + return true; + } + + public function gc(int $max_lifetime): int|false + { + return 0; + } +} + +class ThrowingSessionValue +{ + public function __serialize(): array + { + throw new Exception('encode exploded'); + } +} + +$handler = new ThrowingEncodeHandler; +session_set_save_handler($handler, true); +session_id('encode-exception'); +session_start(); +$_SESSION['value'] = new ThrowingSessionValue; + +try { + session_write_close(); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} + +var_dump($handler->closed); +var_dump($handler->written); +?> +--EXPECT-- +Exception: encode exploded +bool(true) +bool(false) diff --git a/ext/session/tests/session_encode_fail_still_closes.phpt b/ext/session/tests/session_encode_fail_still_closes.phpt new file mode 100644 index 000000000000..92e081ee28eb --- /dev/null +++ b/ext/session/tests/session_encode_fail_still_closes.phpt @@ -0,0 +1,62 @@ +--TEST-- +Encode failure must still close the session save handler +--EXTENSIONS-- +session +--INI-- +session.use_cookies=0 +session.cache_limiter= +session.gc_probability=0 +--FILE-- +closed = true; + return true; + } + + public function read(string $id): string|false + { + return ''; + } + + public function write(string $id, string $data): bool + { + $this->written = true; + return true; + } + + public function destroy(string $id): bool + { + return true; + } + + public function gc(int $max_lifetime): int|false + { + return 0; + } +} + +$handler = new FailingEncodeHandler; +session_set_save_handler($handler, true); +session_id('encode-failure'); +session_start(); +$_SESSION['bad|key'] = 'v'; +session_write_close(); + +var_dump($handler->closed); +var_dump($handler->written); +?> +--EXPECTF-- +Warning: session_write_close(): Failed to write session data. Data contains invalid key "bad|key" in %s on line %d +bool(true) +bool(false)