[php-src] master: session: fix some expectations for C session handlers (#23297)
Gina Peter Banyard via GitHub <[email protected]>
| Newsgroups | gmane.comp.php.cvs.general |
|---|---|
| Message-ID | <[email protected]> |
Author: Gina Peter Banyard (Girgias)
Committer: GitHub (web-flow)
Pusher: Girgias
Date: 2026-08-15T16:54:05+01:00
Commit: https://github.com/php/php-src/commit/61c2b72eec91ce57124bb66c686582d2ae12cf63
Raw diff: https://github.com/php/php-src/commit/61c2b72eec91ce57124bb66c686582d2ae12cf63.diff
session: fix some expectations for C session handlers (#23297)
We *explicitly* say that mod_data will be not NULL for create_sid hooks and we violate this invariant if SessionHandler::open() is overwridden without calling the parent method.
At the same time add comments explaining the situation and fix some other potential footguns.
Changed paths:
M ext/session/mod_files.c
M ext/session/mod_user.c
M ext/session/mod_user_class.c
M ext/session/session.c
M ext/session/tests/user_session_module/session_create_id_create_sid_throws.phpt
M ext/session/tests/user_session_module/session_set_save_handler_class_005.phpt
M ext/session/tests/user_session_module/session_set_save_handler_class_016.phpt
M ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt
Diff:
diff --git a/ext/session/mod_files.c b/ext/session/mod_files.c
index 3a7f8aec4522..c3c616c6479d 100644
--- a/ext/session/mod_files.c
+++ b/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) {
diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c
index 71b8abdea8b0..5c6fe557771f 100644
--- a/ext/session/mod_user.c
+++ b/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 */
diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c
index a6bd69c91a07..9626dd27dfb1 100644
--- a/ext/session/mod_user_class.c
+++ b/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);
}
diff --git a/ext/session/session.c b/ext/session/session.c
index 452a3446fc14..b1f1d2a36304 100644
--- a/ext/session/session.c
+++ b/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--) {
diff --git a/ext/session/tests/user_session_module/session_create_id_create_sid_throws.phpt b/ext/session/tests/user_session_module/session_create_id_create_sid_throws.phpt
index b65c0671d940..c87df2ab458e 100644
--- a/ext/session/tests/user_session_module/session_create_id_create_sid_throws.phpt
+++ b/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)
diff --git a/ext/session/tests/user_session_module/session_set_save_handler_class_005.phpt b/ext/session/tests/user_session_module/session_set_save_handler_class_005.phpt
index c989cf39d737..cf9460c53df8 100644
--- a/ext/session/tests/user_session_module/session_set_save_handler_class_005.phpt
+++ b/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"
diff --git a/ext/session/tests/user_session_module/session_set_save_handler_class_016.phpt b/ext/session/tests/user_session_module/session_set_save_handler_class_016.phpt
index 242035380ea0..61eecc7141d6 100644
--- a/ext/session/tests/user_session_module/session_set_save_handler_class_016.phpt
+++ b/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) {
diff --git a/ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt b/ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt
index 3b9a3f411efa..c9a10de4e442 100644
--- a/ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt
+++ b/ext/session/tests/user_session_module/session_set_save_handler_sid_002.phpt
@@ -1,5 +1,5 @@
--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--
@@ -7,10 +7,6 @@ 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