session: fix some expectations for C session handlers

ext/session/mod_files.c

@@ -101,13 +101,13 @@ const ps_module ps_mod_files = {
 	PS_MOD_UPDATE_TIMESTAMP(files)
 };
 
-static char *ps_files_path_create(char *buf, size_t buflen, ps_files *data, const zend_string *key)
+ZEND_ATTRIBUTE_NONNULL static char *ps_files_path_create(char *buf, size_t buflen, ps_files *data, const zend_string *key)
 {
 	const char *p;
 	int i;
 	size_t n;
 
-	if (!data || ZSTR_LEN(key) <= data->dirdepth ||
+	if (ZSTR_LEN(key) <= data->dirdepth ||
 		buflen < (ZSTR_LEN(data->basedir) + 2 * data->dirdepth + ZSTR_LEN(key) + 5 + sizeof(FILE_PREFIX))) {
 		return NULL;
 	}
@@ -351,12 +351,12 @@ static int ps_files_cleanup_dir(const zend_string *dirname, zend_long maxlifetim
 	return nrdels;
 }
 
-static zend_result ps_files_key_exists(ps_files *data, const zend_string *key)
+ZEND_ATTRIBUTE_NONNULL static zend_result ps_files_key_exists(ps_files *data, const zend_string *key)
 {
 	char buf[MAXPATHLEN];
 	zend_stat_t sbuf = {0};
 
-	if (!key || !ps_files_path_create(buf, sizeof(buf), data, key)) {
+	if (!ps_files_path_create(buf, sizeof(buf), data, key)) {
 		return FAILURE;
 	}
 	if (VCWD_STAT(buf, &sbuf)) {
@@ -678,8 +678,7 @@ PS_CREATE_SID_FUNC(files)
 			}
 		}
 		/* Check collision */
-		/* FIXME: mod_data(data) should not be NULL (User handler could be NULL) */
-		if (data && ps_files_key_exists(data, sid) == SUCCESS) {
+		if (ps_files_key_exists(data, sid) == SUCCESS) {
 			zend_string_release_ex(sid, false);
 			sid = NULL;
 			if (--maxfail < 0) {

ext/session/mod_user.c

@@ -16,6 +16,8 @@
 #include "php_session.h"
 #include "mod_user.h"
 
+#include "zend_exceptions.h"
+
 const ps_module ps_mod_user = {
 	PS_MOD_UPDATE_TIMESTAMP(user)
 };
@@ -222,27 +224,23 @@ PS_CREATE_SID_FUNC(user)
 {
 	/* maintain backwards compatibility */
 	if (!Z_ISUNDEF(PSF(create_sid))) {
-		zend_string *id = NULL;
 		zval retval;
 
 		ps_call_handler(&PSF(create_sid), 0, NULL, &retval);
-
-		if (!Z_ISUNDEF(retval)) {
-			if (Z_TYPE(retval) == IS_STRING) {
-				id = zend_string_copy(Z_STR(retval));
-			}
-			zval_ptr_dtor(&retval);
-		} else {
-			zend_throw_error(NULL, "No session id returned by function");
+		/* Exception was thrown */
+		if (Z_ISUNDEF(retval)) {
 			return NULL;
 		}
 
-		if (!id) {
-			zend_throw_error(NULL, "Session id must be a string");
+		if (UNEXPECTED(Z_TYPE(retval) != IS_STRING)) {
+			/* Will no longer be needed in PHP 9 as the interface return type will be in effect */
+			zend_throw_error(zend_ce_type_error, "Session id must be of type string, %s given", zend_zval_type_name(&retval));
+			zval_ptr_dtor(&retval);
 			return NULL;
 		}
+		ZEND_ASSERT(Z_TYPE(retval) == IS_STRING);
 
-		return id;
+		return Z_STR(retval);
 	}
 
 	/* function as defined by PS_MOD */

ext/session/mod_user_class.c

@@ -51,6 +51,7 @@ PHP_METHOD(SessionHandler, open)
 	} zend_end_try();
 
 	if (SUCCESS == ret) {
+		ZEND_ASSERT(PS(mod_data) && "opened default session must have mod_data");
 		PS(mod_user_is_open) = true;
 	}
 
@@ -142,13 +143,18 @@ PHP_METHOD(SessionHandler, gc)
 
 PHP_METHOD(SessionHandler, create_sid)
 {
-	zend_string *id;
-
 	ZEND_PARSE_PARAMETERS_NONE();
 
 	PS_SANITY_CHECK;
+	if (!PS(mod_user_is_open)) {
+		php_error_docref(NULL, E_WARNING, "Parent session handler is not open, defaulting to session_create_id()");
+		RETURN_STR(php_session_create_id(NULL));
+	}
 
-	id = PS(default_mod)->s_create_sid(&PS(mod_data));
+	zend_string *id = PS(default_mod)->s_create_sid(&PS(mod_data));
+	if (UNEXPECTED(id == NULL)) {
+		zend_throw_error(NULL, "Failed to create session ID: %s (path: %s)", PS(mod)->s_name, ZSTR_VAL(PS(save_path)));
+	}
 
 	RETURN_STR(id);
 }

ext/session/session.c

@@ -429,9 +429,9 @@ static zend_result php_session_initialize(void)
 	}
 
 	/* Open session handler first */
-	if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE
-		/* || PS(mod_data) == NULL */ /* FIXME: open must set valid PS(mod_data) with success */
-	) {
+	const zend_result open_status = PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name));
+	/* NOTE: PS(mod_data) might be null if the session is a custom userland session handler */
+	if (open_status == FAILURE) {
 		php_session_abort();
 		if (!EG(exception)) {
 			php_error_docref(NULL, E_WARNING, "Failed to initialize storage module: %s (path: %s)", PS(mod)->s_name, ZSTR_VAL(PS(save_path)));
@@ -2399,7 +2399,10 @@ PHP_FUNCTION(session_regenerate_id)
 	zend_string_release_ex(PS(id), false);
 	PS(id) = NULL;
 
-	if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE) {
+	/* Open session handler first */
+	const zend_result open_status = PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name));
+	/* NOTE: PS(mod_data) might be null if the session is a custom userland session handler */
+	if (open_status == FAILURE) {
 		PS(session_status) = php_session_none;
 		if (!EG(exception)) {
 			zend_throw_error(NULL, "Failed to open session: %s (path: %s)", PS(mod)->s_name, ZSTR_VAL(PS(save_path)));
@@ -2480,6 +2483,7 @@ PHP_FUNCTION(session_create_id)
 		}
 	}
 
+	/* NOTE: PS(mod_data) might be null if the session is a custom userland session handler */
 	if (!PS(in_save_handler) && PS(session_status) == php_session_active) {
 		int limit = 3;
 		while (limit--) {

ext/session/tests/user_session_module/session_create_id_create_sid_throws.phpt

@@ -44,6 +44,6 @@ var_dump(session_status() === PHP_SESSION_ACTIVE);
 
 ?>
 --EXPECT--
-Error: Session id must be a string
+TypeError: Session id must be of type string, null given
 Exception: create_sid failed
 bool(true)

ext/session/tests/user_session_module/session_set_save_handler_class_005.phpt

@@ -37,11 +37,13 @@ session_unset();
 --EXPECTF--
 *** Testing session_set_save_handler() : incomplete implementation ***
 
-Warning: SessionHandler::read(): Parent session handler is not open in %ssession_set_save_handler_class_005.php on line %d
+Warning: SessionHandler::create_sid(): Parent session handler is not open, defaulting to session_create_id() in %s on line %d
 
-Warning: SessionHandler::close(): Parent session handler is not open in %ssession_set_save_handler_class_005.php on line %d
+Warning: SessionHandler::read(): Parent session handler is not open in %s on line %d
 
-Warning: session_start(): Failed to read session data: user (%s) in %ssession_set_save_handler_class_005.php on line %d
+Warning: SessionHandler::close(): Parent session handler is not open in %s on line %d
+
+Warning: session_start(): Failed to read session data: user (%s) in %s on line %d
 bool(false)
 string(0) ""
 string(4) "user"

ext/session/tests/user_session_module/session_set_save_handler_class_016.phpt

@@ -72,6 +72,8 @@ session_unset();
 ?>
 --EXPECTF--
 *** Testing session_set_save_handler() function: class with create_sid ***
+
+Warning: SessionHandler::create_sid(): Parent session handler is not open, defaulting to session_create_id() in %s on line %d
 string(%d) "%s"
 string(4) "user"
 array(1) {

ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt

@@ -1,16 +1,12 @@
 --TEST--
-Test session_set_save_handler() function: create_sid
+session_set_save_handler() with create_sid handler not returning string
 --INI--
 session.save_path="{TMP}"
 --EXTENSIONS--
 session
 --FILE--
 <?php
 
-ob_start();
-
-echo "*** Testing session_set_save_handler() function: create_sid ***\n";
-
 class MySession2 implements SessionHandlerInterface, SessionIdInterface {
     public $path;
 
@@ -27,7 +23,7 @@ class MySession2 implements SessionHandlerInterface, SessionIdInterface {
     }
 
     public function read($id): string|false {
-        return @file_get_contents($this->path . $id);
+        return file_get_contents($this->path . $id);
     }
 
     public function write($id, $data): bool {
@@ -55,26 +51,13 @@ class MySession2 implements SessionHandlerInterface, SessionIdInterface {
 }
 
 session_set_save_handler(new MySession2());
-session_start();
-
-$_SESSION['foo'] = "hello";
-
-var_dump(session_id(), ini_get('session.save_handler'), $_SESSION);
 
-session_write_close();
-session_unset();
-
-session_start();
-var_dump($_SESSION);
+try {
+    session_start();
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+}
 
-session_write_close();
-session_unset();
 ?>
---EXPECTF--
-*** Testing session_set_save_handler() function: create_sid ***
-
-Fatal error: Uncaught Error: Session id must be a string in %s:%d
-Stack trace:
-#0 %s(%d): session_start()
-#1 {main}
-  thrown in %s on line %d
+--EXPECT--
+TypeError: Session id must be of type string, bool given