ext/session: hold the save handler recursion guard for the whole call (stacked on GH-22926)

ext/session/mod_user.c

@@ -27,7 +27,6 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval)
 {
 	int i;
 	if (PS(in_save_handler)) {
-		PS(in_save_handler) = 0;
 		ZVAL_UNDEF(retval);
 		php_error_docref(NULL, E_WARNING, "Cannot call session save handler in a recursive manner");
 	} else {

ext/session/session.c

@@ -145,6 +145,12 @@ static void php_rshutdown_session_globals(void) /* {{{ */
 			PS(mod)->s_close(&PS(mod_data));
 		} zend_end_try();
 	}
+	if (PS(mod_user_is_open) && PS(default_mod) && PS(mod_data)) {
+		PS(mod_user_is_open) = 0;
+		zend_try {
+			PS(default_mod)->s_close(&PS(mod_data));
+		} zend_end_try();
+	}
 	if (PS(id)) {
 		zend_string_release_ex(PS(id), 0);
 		PS(id) = NULL;
@@ -2408,8 +2414,14 @@ PHP_FUNCTION(session_regenerate_id)
 			RETURN_FALSE;
 		}
 	}
+
 	PS(mod)->s_close(&PS(mod_data));
 
+	if (PS(session_status) != php_session_active) {
+		php_error_docref(NULL, E_WARNING, "Session ID cannot be regenerated because the save handler closed the session");
+		RETURN_FALSE;
+	}
+
 	/* New session data */
 	if (PS(session_vars)) {
 		zend_string_release_ex(PS(session_vars), 0);

ext/session/tests/user_session_module/recursive_handler_argv_leak.phpt

@@ -28,4 +28,6 @@ echo "done\n";
 Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
 
 Warning: session_destroy(): Session object destruction failed in %s on line %d
+
+Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
 done

ext/session/tests/user_session_module/session_handler_close_without_parent.phpt

@@ -0,0 +1,31 @@
+--TEST--
+SessionHandler::close() that is never delegated still releases the default handler
+--INI--
+session.save_handler=files
+session.name=PHPSESSID
+session.gc_probability=0
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+ob_start();
+
+class MySessionHandler extends SessionHandler
+{
+    public function close(): bool
+    {
+        return true;
+    }
+}
+
+session_set_save_handler(new MySessionHandler(), true);
+session_start();
+$_SESSION['key'] = 'value';
+var_dump(session_write_close());
+var_dump(session_status() === PHP_SESSION_NONE);
+
+?>
+--EXPECT--
+bool(true)
+bool(true)

ext/session/tests/user_session_module/session_regenerate_id_handler_closes_session.phpt

@@ -0,0 +1,69 @@
+--TEST--
+session_regenerate_id() when the close handler destroys the session
+--INI--
+session.save_handler=files
+session.name=PHPSESSID
+session.gc_probability=0
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+ob_start();
+
+class MySessionHandler implements SessionHandlerInterface
+{
+    private bool $destroyed = false;
+
+    public function open(string $path, string $name): bool
+    {
+        return true;
+    }
+
+    public function close(): bool
+    {
+        if (!$this->destroyed) {
+            $this->destroyed = true;
+            session_destroy();
+        }
+        return true;
+    }
+
+    public function read(string $id): string|false
+    {
+        return '';
+    }
+
+    public function write(string $id, string $data): bool
+    {
+        return true;
+    }
+
+    public function destroy(string $id): bool
+    {
+        return true;
+    }
+
+    public function gc(int $max_lifetime): int|false
+    {
+        return 0;
+    }
+}
+
+session_set_save_handler(new MySessionHandler(), true);
+session_start();
+
+var_dump(session_regenerate_id(false));
+var_dump(session_status() === PHP_SESSION_NONE);
+
+?>
+--EXPECTF--
+Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
+
+Warning: session_destroy(): Session object destruction failed in %s on line %d
+
+Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
+
+Warning: session_regenerate_id(): Session ID cannot be regenerated because the save handler closed the session in %s on line %d
+bool(false)
+bool(true)

ext/session/tests/user_session_module/session_regenerate_id_handler_destroys_session.phpt

@@ -0,0 +1,39 @@
+--TEST--
+session_regenerate_id() when the save handler destroys the session
+--INI--
+session.save_handler=files
+session.name=PHPSESSID
+session.gc_probability=0
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+ob_start();
+
+class MySessionHandler extends SessionHandler
+{
+    public function destroy(string $id): bool
+    {
+        session_destroy();
+        return true;
+    }
+}
+
+session_set_save_handler(new MySessionHandler(), true);
+session_start();
+
+var_dump(session_regenerate_id(true));
+var_dump(session_status() === PHP_SESSION_NONE);
+
+?>
+--EXPECTF--
+Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
+
+Warning: session_destroy(): Session object destruction failed in %s on line %d
+
+Warning: session_destroy(): Cannot call session save handler in a recursive manner in %s on line %d
+
+Warning: session_regenerate_id(): Session ID cannot be regenerated because the save handler closed the session in %s on line %d
+bool(false)
+bool(true)

ext/session/tests/user_session_module/session_save_handler_recursion_guard.phpt

@@ -0,0 +1,51 @@
+--TEST--
+The save handler recursion guard survives a rejected nested call
+--INI--
+session.save_handler=files
+session.name=PHPSESSID
+session.gc_probability=0
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+ob_start();
+
+class MySessionHandler extends SessionHandler
+{
+    public bool $armed = true;
+    public int $gcEntered = 0;
+
+    public function gc(int $max_lifetime): int|false
+    {
+        $this->gcEntered++;
+        return 0;
+    }
+
+    public function write(string $id, string $data): bool
+    {
+        if ($this->armed) {
+            $this->armed = false;
+            var_dump(session_gc());
+            var_dump(session_gc());
+        }
+        return parent::write($id, $data);
+    }
+}
+
+$handler = new MySessionHandler();
+session_set_save_handler($handler, true);
+session_start();
+$_SESSION['key'] = 'value';
+session_write_close();
+
+echo 'gc() entered ', $handler->gcEntered, " time(s)\n";
+
+?>
+--EXPECTF--
+Warning: session_gc(): Cannot call session save handler in a recursive manner in %s on line %d
+bool(false)
+
+Warning: session_gc(): Cannot call session save handler in a recursive manner in %s on line %d
+bool(false)
+gc() entered 0 time(s)