ext/session: Close session without retrying write() after handler bailout

NEWS

@@ -37,6 +37,10 @@ PHP                                                                        NEWS
   . Fixed infinite loop in sapi_header_op() for headers of 4 GiB or more.
     (Martin Mirchev)
 
+- Session:
+  . Fixed session save handler wrongly reported as recursive after a fatal
+    error inside a handler callback. (Ilia Alshanetsky)
+
 - SPL:
   . Fixed bug GH-24264 (Unwind/GracefulExit discarded in spl_iterators.c).
     (Lazizbek Ergashev)

ext/session/mod_user.c

@@ -25,23 +25,32 @@ const ps_module ps_mod_user = {
 static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval)
 {
 	int i;
+	bool bailout = false;
 	if (PS(in_save_handler)) {
 		PS(in_save_handler) = false;
 		ZVAL_UNDEF(retval);
 		php_error_docref(NULL, E_WARNING, "Cannot call session save handler in a recursive manner");
 	} else {
 		PS(in_save_handler) = true;
-		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 {
+			bailout = true;
+			PS(handler_bailout) = true;
+		} zend_end_try();
 		PS(in_save_handler) = false;
 	}
 	for (i = 0; i < argc; i++) {
 		zval_ptr_dtor(&argv[i]);
 	}
+	if (bailout) {
+		zend_bailout();
+	}
 }
 
 #define PSF(a) PS(mod_user_names).ps_##a
@@ -91,7 +100,9 @@ PS_OPEN_FUNC(user)
 	zend_try {
 		ps_call_handler(&PSF(open), 2, args, &retval);
 	} zend_catch {
-		PS(session_status) = php_session_none;
+		if (!PS(mod_user_implemented)) {
+			PS(session_status) = php_session_none;
+		}
 		if (!Z_ISUNDEF(retval)) {
 			zval_ptr_dtor(&retval);
 		}

ext/session/php_session.h

@@ -184,6 +184,7 @@ typedef struct _php_ps_globals {
 	bool mod_user_uses_object_methods_as_handlers;
 	bool use_trans_sid; /* contains the INI value of whether to use trans-sid */
 	bool random_seeded;
+	bool handler_bailout;
 
 	bool in_rfc1867_update;    /* php_session_rfc1867_update() is running */
 } php_ps_globals;

ext/session/session.c

@@ -114,6 +114,7 @@ static inline void php_rinit_session_globals(void)
 	PS(session_status) = php_session_none;
 	PS(in_save_handler) = false;
 	PS(set_handler) = false;
+	PS(handler_bailout) = false;
 	PS(mod_data) = NULL;
 	PS(mod_user_is_open) = false;
 	PS(mod_user_uses_object_methods_as_handlers) = false;
@@ -421,6 +422,7 @@ static zend_result php_session_initialize(void)
 	zend_string *val = NULL;
 
 	PS(session_status) = php_session_active;
+	PS(handler_bailout) = false;
 
 	if (!PS(mod)) {
 		PS(session_status) = php_session_disabled;
@@ -1804,8 +1806,18 @@ PHPAPI zend_result php_session_start(void)
 PHPAPI bool php_session_flush(bool write)
 {
 	if (PS(session_status) == php_session_active) {
-		php_session_save_current_state(write);
+		bool bailout = false;
+		zend_try {
+			php_session_save_current_state(write && !PS(handler_bailout));
+		} zend_catch {
+			bailout = true;
+		} zend_end_try();
+		if (bailout) {
+			PS(handler_bailout) = true;
+			zend_bailout();
+		}
 		PS(session_status) = php_session_none;
+		PS(handler_bailout) = false;
 		return true;
 	}
 	return false;
@@ -2395,6 +2407,11 @@ PHP_FUNCTION(session_regenerate_id)
 		RETURN_FALSE;
 	}
 
+	if (PS(handler_bailout)) {
+		php_error_docref(NULL, E_WARNING, "Session ID cannot be regenerated after a fatal error in session handling");
+		RETURN_FALSE;
+	}
+
 	/* Process old session data */
 	if (del_ses) {
 		if (PS(mod)->s_destroy(&PS(mod_data), PS(id)) == FAILURE) {
@@ -2950,6 +2967,7 @@ static PHP_GINIT_FUNCTION(ps)
 	ps_globals->mod_user_uses_object_methods_as_handlers = false;
 	ps_globals->session_vars = NULL;
 	ps_globals->set_handler = false;
+	ps_globals->handler_bailout = false;
 	ps_globals->session_started_filename = NULL;
 	ps_globals->session_started_lineno = 0;
 	/* Unset user defined handlers */

ext/session/tests/bailout_in_encode_closes_handler.phpt

@@ -0,0 +1,48 @@
+--TEST--
+GH-15529: Bailout while encoding session data still closes a SessionHandler-based handler
+--EXTENSIONS--
+session
+zend_test
+--INI--
+session.save_handler=files
+session.serialize_handler=php
+--FILE--
+<?php
+ob_start();
+class Handler extends SessionHandler
+{
+    public function write(string $id, string $data): bool {
+        echo "write\n";
+        return parent::write($id, $data);
+    }
+    public function close(): bool {
+        echo "close\n";
+        return parent::close();
+    }
+}
+class Bomb
+{
+    public function __serialize(): array {
+        zend_trigger_bailout();
+    }
+}
+$dir = __DIR__ . '/bailout_in_encode_closes_handler';
+@mkdir($dir);
+session_save_path($dir);
+session_set_save_handler(new Handler(), true);
+session_start();
+$_SESSION['bomb'] = new Bomb();
+session_write_close();
+echo "not reached\n";
+?>
+--CLEAN--
+<?php
+$dir = __DIR__ . '/bailout_in_encode_closes_handler';
+foreach (glob($dir . '/*') as $file) {
+    unlink($file);
+}
+@rmdir($dir);
+?>
+--EXPECTF--
+Fatal error: Bailout in %s on line %d
+close

ext/session/tests/bailout_in_read_keeps_data.phpt

@@ -0,0 +1,57 @@
+--TEST--
+GH-15529: Bailout in read() does not overwrite the stored session data
+--EXTENSIONS--
+session
+zend_test
+--INI--
+session.save_handler=files
+session.serialize_handler=php
+session.use_cookies=0
+session.use_strict_mode=0
+session.cache_limiter=
+--FILE--
+<?php
+ob_start();
+class Handler extends SessionHandler
+{
+    public function read(string $id): string|false {
+        zend_trigger_bailout();
+    }
+    public function write(string $id, string $data): bool {
+        echo "write\n";
+        return parent::write($id, $data);
+    }
+    public function close(): bool {
+        echo "close\n";
+        return parent::close();
+    }
+}
+$dir = __DIR__ . '/bailout_in_read_keeps_data';
+@mkdir($dir);
+$file = $dir . '/sess_gh15529read';
+file_put_contents($file, 'x|i:1;');
+session_save_path($dir);
+session_id('gh15529read');
+session_set_save_handler(new Handler(), true);
+register_shutdown_function(function () use ($file) {
+    var_dump(session_regenerate_id(true));
+    var_dump(file_get_contents($file));
+});
+session_start();
+echo "not reached\n";
+?>
+--CLEAN--
+<?php
+$dir = __DIR__ . '/bailout_in_read_keeps_data';
+foreach (glob($dir . '/*') as $file) {
+    unlink($file);
+}
+@rmdir($dir);
+?>
+--EXPECTF--
+Fatal error: Bailout in %s on line %d
+
+Warning: session_regenerate_id(): Session ID cannot be regenerated after a fatal error in session handling in %s on line %d
+bool(false)
+string(6) "x|i:1;"
+close

ext/session/tests/bailout_in_write_no_sticky_flag.phpt

@@ -0,0 +1,32 @@
+--TEST--
+GH-15529: Bailout in userland save handler write() is not retried, close() still runs
+--EXTENSIONS--
+session
+zend_test
+--FILE--
+<?php
+class Handler implements SessionHandlerInterface, SessionIdInterface
+{
+    public function create_sid(): string { return 'gh15529'; }
+    public function validateId(string $id): bool { return true; }
+    public function open($path, $name): bool { return true; }
+    public function close(): bool { echo "close\n"; return true; }
+    public function read($id): string|false { return ''; }
+    public function write($id, $data): bool {
+        echo "write\n";
+        zend_trigger_bailout();
+    }
+    public function destroy($id): bool { return true; }
+    public function gc(int $max_lifetime): int|false { return 0; }
+}
+session_set_save_handler(new Handler(), true);
+session_start();
+$_SESSION['x'] = 1;
+session_write_close();
+echo "not reached\n";
+?>
+--EXPECTF--
+write
+
+Fatal error: Bailout in %s on line %d
+close

ext/session/tests/user_session_module/bug60634_error_3.phpt

@@ -50,5 +50,4 @@ Stack trace:
 #0 [internal function]: write(%s, '')
 #1 {main}
   thrown in %s on line %d
-
-Warning: PHP Request Shutdown: Cannot call session save handler in a recursive manner in Unknown on line 0
+close: goodbye cruel world

ext/session/tests/user_session_module/bug60634_error_4.phpt

@@ -45,5 +45,4 @@ Stack trace:
 #0 [internal function]: write('%s', '')
 #1 {main}
   thrown in %s on line %d
-
-Warning: PHP Request Shutdown: Cannot call session save handler in a recursive manner in Unknown on line 0
+close: goodbye cruel world