From 1f81a3b9a79970b867e473451cbe68c7f841a782 Mon Sep 17 00:00:00 2001
From: George Peter Banyard <girgias@php.net>
Date: Thu, 29 Sep 2022 13:31:53 +0100
Subject: [PATCH 1/2] Actually fix GH-9583

The issue is that PS(mod)->s_validate_sid is always defined for user modules, thus we need to check that the actual callable is set
Add another regression test to ensure current working behaviour is not broken (which was by the previous incorrect fix)
---
 ext/session/session.c               |  7 +++--
 ext/session/tests/gh9583-extra.phpt | 49 +++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 ext/session/tests/gh9583-extra.phpt

diff --git a/ext/session/session.c b/ext/session/session.c
index fe39381950b44..f0712f3fcaedf 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -1083,9 +1083,10 @@ PHPAPI int php_session_register_module(const ps_module *ptr) /* {{{ */
 /* }}} */
 
 /* Dummy PS module function */
-/* We consider any ID valid, so we return FAILURE to indicate that a session doesn't exist */
+/* We consider any ID valid (thus also implying that a session with such an ID exists),
+	thus we always return SUCCESS */
 PHPAPI int php_session_validate_sid(PS_VALIDATE_SID_ARGS) {
-	return FAILURE;
+	return SUCCESS;
 }
 
 /* Dummy PS module function */
@@ -2317,7 +2318,7 @@ PHP_FUNCTION(session_create_id)
 		int limit = 3;
 		while (limit--) {
 			new_id = PS(mod)->s_create_sid(&PS(mod_data));
-			if (!PS(mod)->s_validate_sid) {
+			if (!PS(mod)->s_validate_sid || (PS(mod_user_implemented) && Z_ISUNDEF(PS(mod_user_names).name.ps_validate_sid))) {
 				break;
 			} else {
 				/* Detect collision and retry */
diff --git a/ext/session/tests/gh9583-extra.phpt b/ext/session/tests/gh9583-extra.phpt
new file mode 100644
index 0000000000000..4332dd9b3eb12
--- /dev/null
+++ b/ext/session/tests/gh9583-extra.phpt
@@ -0,0 +1,49 @@
+--TEST--
+GH-9583: session_create_id() fails with user defined save handler that doesn't have a validateId() method
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+
+class SessionHandlerTester implements \SessionHandlerInterface
+{
+
+    public function close(): bool { return true; }
+
+    public function destroy($id): bool { return true; }
+
+    public function gc($max_lifetime): int|false { return 1; }
+
+    public function open($path, $name): bool { return true; }
+
+    public function read($id): string { return ''; }
+
+    public function write($id, $data): bool { return true; }
+
+    //public function create_sid() { return uniqid(); }
+
+    //public function validateId($key) { return true; }
+}
+
+$obj = new SessionHandlerTester();
+ini_set('session.use_strict_mode','1');
+session_set_save_handler($obj);
+session_start();
+
+$originalSessionId = session_id();
+
+session_write_close();
+
+session_start();
+
+$newSessionId = session_id();
+
+echo 'validateId() ', (method_exists($obj, 'validateId') ? ('returns ' . ($obj->validateId(1) ? 'true' : 'false')) : 'is commented out'), "\n";
+var_dump($originalSessionId == $newSessionId);
+
+?>
+--EXPECT--
+validateId() is commented out
+bool(true)

From 0c07c5573b8caade85c5e745c256385824456435 Mon Sep 17 00:00:00 2001
From: George Peter Banyard <girgias@php.net>
Date: Fri, 30 Sep 2022 12:22:51 +0100
Subject: [PATCH 2/2] Also amend session_regenerate_id()

---
 ext/session/session.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/ext/session/session.c b/ext/session/session.c
index f0712f3fcaedf..3c875296c3a17 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -2256,18 +2256,24 @@ PHP_FUNCTION(session_regenerate_id)
 		}
 		RETURN_THROWS();
 	}
-	if (PS(use_strict_mode) && PS(mod)->s_validate_sid &&
-		PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {
-		zend_string_release_ex(PS(id), 0);
-		PS(id) = PS(mod)->s_create_sid(&PS(mod_data));
-		if (!PS(id)) {
-			PS(mod)->s_close(&PS(mod_data));
-			PS(session_status) = php_session_none;
-			if (!EG(exception)) {
-				zend_throw_error(NULL, "Failed to create session ID by collision: %s (path: %s)", PS(mod)->s_name, PS(save_path));
+	if (PS(use_strict_mode)) {
+		if ((!PS(mod_user_implemented) && PS(mod)->s_validate_sid) || !Z_ISUNDEF(PS(mod_user_names).name.ps_validate_sid)) {
+			int limit = 3;
+			/* Try to generate non-existing ID */
+			while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {
+				zend_string_release_ex(PS(id), 0);
+				PS(id) = PS(mod)->s_create_sid(&PS(mod_data));
+				if (!PS(id)) {
+					PS(mod)->s_close(&PS(mod_data));
+					PS(session_status) = php_session_none;
+					if (!EG(exception)) {
+						zend_throw_error(NULL, "Failed to create session ID by collision: %s (path: %s)", PS(mod)->s_name, PS(save_path));
+					}
+					RETURN_THROWS();
+				}
 			}
-			RETURN_THROWS();
 		}
+		// TODO warn that ID cannot be verified? else { }
 	}
 	/* Read is required to make new session data at this point. */
 	if (PS(mod)->s_read(&PS(mod_data), PS(id), &data, PS(gc_maxlifetime)) == FAILURE) {
@@ -2294,7 +2300,6 @@ PHP_FUNCTION(session_regenerate_id)
 /* }}} */
 
 /* {{{ Generate new session ID. Intended for user save handlers. */
-/* This is not used yet */
 PHP_FUNCTION(session_create_id)
 {
 	zend_string *prefix = NULL, *new_id;