Skip to content

ext/session: only return false when could not encode session at all #21181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Girgias wants to merge 1 commit into
base: master
Choose a base branch
from
+45 −46
Draft

ext/session: only return false when could not encode session at all #21181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 32 additions & 31 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,24 +507,24 @@ static void php_session_save_current_state(bool write)
IF_SESSION_VARS() {
zval *handler_function = &PS(mod_user_names).ps_write;
if (PS(mod_data) || PS(mod_user_implemented)) {
zend_string *val;

val = php_session_encode();
if (val) {
if (PS(lazy_write) && PS(session_vars)
&& PS(mod)->s_update_timestamp
&& PS(mod)->s_update_timestamp != php_session_update_timestamp
&& zend_string_equals(val, PS(session_vars))
) {
ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
handler_function = &PS(mod_user_names).ps_update_timestamp;
} else {
ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
}
zend_string_release_ex(val, false);
zend_string *val = php_session_encode();
/* Not being able to encode the session data means there is some kind of issue that prevents a write
* (e.g. a key containing the '|' character with the default serialization) */
if (UNEXPECTED(val == NULL)) {
return;
}

if (PS(lazy_write) && PS(session_vars)
&& PS(mod)->s_update_timestamp
&& PS(mod)->s_update_timestamp != php_session_update_timestamp
&& zend_string_equals(val, PS(session_vars))
) {
ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
handler_function = &PS(mod_user_names).ps_update_timestamp;
} else {
ret = PS(mod)->s_write(&PS(mod_data), PS(id), ZSTR_EMPTY_ALLOC(), PS(gc_maxlifetime));
ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
}
zend_string_release_ex(val, false);
}

if ((ret == FAILURE) && !EG(exception)) {
Expand Down Expand Up @@ -929,7 +929,7 @@ PS_SERIALIZER_ENCODE_FUNC(php_serialize)
php_var_serialize(&buf, Z_REFVAL(PS(http_session_vars)), &var_hash);
PHP_VAR_SERIALIZE_DESTROY(var_hash);
}
return buf.s;
return smart_str_extract(&buf);
}

PS_SERIALIZER_DECODE_FUNC(php_serialize)
Expand Down Expand Up @@ -980,11 +980,9 @@ PS_SERIALIZER_ENCODE_FUNC(php_binary)
smart_str_append(&buf, key);
php_var_serialize(&buf, struc, &var_hash);
);

smart_str_0(&buf);
PHP_VAR_SERIALIZE_DESTROY(var_hash);

return buf.s;
return smart_str_extract(&buf);
}

PS_SERIALIZER_DECODE_FUNC(php_binary)
Expand Down Expand Up @@ -1047,15 +1045,15 @@ PS_SERIALIZER_ENCODE_FUNC(php)
smart_str_appendc(&buf, PS_DELIMITER);
php_var_serialize(&buf, struc, &var_hash);
);
PHP_VAR_SERIALIZE_DESTROY(var_hash);
Copy link
Member

@ndossche ndossche Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a bug: now var_hash is destroyed twice because it is already destroyed at line 1039 too. I think you may drop line 1039.


if (fail) {
return NULL;
}

smart_str_0(&buf);
zend_string *encoded = smart_str_extract(&buf);

PHP_VAR_SERIALIZE_DESTROY(var_hash);
return buf.s;
return encoded;
}

PS_SERIALIZER_DECODE_FUNC(php)
Expand Down Expand Up @@ -2280,7 +2278,6 @@ PHP_FUNCTION(session_id)
PHP_FUNCTION(session_regenerate_id)
{
bool del_ses = false;
zend_string *data;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|b", &del_ses) == FAILURE) {
RETURN_THROWS();
Expand All @@ -2307,14 +2304,17 @@ PHP_FUNCTION(session_regenerate_id)
RETURN_FALSE;
}
} else {
zend_result ret;
data = php_session_encode();
if (data) {
ret = PS(mod)->s_write(&PS(mod_data), PS(id), data, PS(gc_maxlifetime));
zend_string_release_ex(data, false);
} else {
ret = PS(mod)->s_write(&PS(mod_data), PS(id), ZSTR_EMPTY_ALLOC(), PS(gc_maxlifetime));
zend_string *old_session_data = php_session_encode();
/* If we have no data we must destroy the related session ID */
if (UNEXPECTED(old_session_data == NULL)) {
PS(mod)->s_close(&PS(mod_data));
PS(session_status) = php_session_none;
RETURN_FALSE;
}

zend_result ret = PS(mod)->s_write(&PS(mod_data), PS(id), old_session_data, PS(gc_maxlifetime));
zend_string_release_ex(old_session_data, false);

if (ret == FAILURE) {
PS(mod)->s_close(&PS(mod_data));
PS(session_status) = php_session_none;
Expand Down Expand Up @@ -2368,6 +2368,7 @@ PHP_FUNCTION(session_regenerate_id)
// TODO warn that ID cannot be verified? else { }
}
/* Read is required to make new session data at this point. */
zend_string *data;
Copy link
Member

@devnexen devnexen Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be initialised to NULL wdyt ?

Copy link
Member

@ndossche ndossche Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it wasn't previously though, so is that a bug then do you think?

Copy link
Member

@devnexen devnexen Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure all callbacks guarantee initialisation, this is all I m saying but I do not session that well :)

if (PS(mod)->s_read(&PS(mod_data), PS(id), &data, PS(gc_maxlifetime)) == FAILURE) {
PS(mod)->s_close(&PS(mod_data));
PS(session_status) = php_session_none;
Expand Down
8 changes: 4 additions & 4 deletions ext/session/tests/session_encode_error2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,28 +51,28 @@ ob_end_flush();
bool(true)

Warning: session_encode(): Skipping numeric key 0 in %s on line %d
bool(false)
string(0) ""
bool(true)

-- Iteration 2 --
bool(true)

Warning: session_encode(): Skipping numeric key 1 in %s on line %d
bool(false)
string(0) ""
bool(true)

-- Iteration 3 --
bool(true)

Warning: session_encode(): Skipping numeric key 12345 in %s on line %d
bool(false)
string(0) ""
bool(true)

-- Iteration 4 --
bool(true)

Warning: session_encode(): Skipping numeric key -2345 in %s on line %d
bool(false)
string(0) ""
bool(true)

-- Iteration 5 --
Expand Down
6 changes: 3 additions & 3 deletions ext/session/tests/session_encode_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ ob_end_flush();
Warning: session_encode(): Cannot encode non-existent session in %s on line %d
bool(false)
bool(true)
bool(false)
string(0) ""
bool(true)
bool(false)
string(0) ""
bool(true)
bool(false)
string(0) ""
bool(true)

Warning: session_encode(): Cannot encode non-existent session in %s on line %d
Expand Down
2 changes: 1 addition & 1 deletion ext/session/tests/session_encode_variation2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ob_end_flush();
?>
--EXPECTF--
*** Testing session_encode() : variation ***
bool(false)
string(0) ""
bool(true)

Warning: session_encode(): Cannot encode non-existent session in %s on line %d
Expand Down
6 changes: 3 additions & 3 deletions ext/session/tests/session_encode_variation6.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,16 @@ ob_end_flush();
bool(true)

Warning: session_encode(): Skipping numeric key 0 in %s on line %d
bool(false)
string(0) ""
bool(true)
bool(true)

Warning: session_encode(): Skipping numeric key 1234567890 in %s on line %d
bool(false)
string(0) ""
bool(true)
bool(true)

Warning: session_encode(): Skipping numeric key -1234567890 in %s on line %d
bool(false)
string(0) ""
bool(true)
Done
4 changes: 1 addition & 3 deletions ext/session/tests/user_session_module/bug71162.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ updateTimestamp never called when session data is empty
session
--INI--
session.use_strict_mode=0
--XFAIL--
Current session module is designed to write empty session always.
--FILE--
<?php
class MySessionHandler extends SessionHandler implements SessionUpdateTimestampHandlerInterface
Expand Down Expand Up @@ -71,7 +69,7 @@ session_commit();
--EXPECT--
string(40) "da39a3ee5e6b4b0d3255bfef95601890afd80709"
bool(true)
write
updateTimestamp
string(40) "da39a3ee5e6b4b0d3255bfef95601890afd80709"
bool(true)
updateTimestamp
Expand Down
2 changes: 1 addition & 1 deletion ext/session/tests/user_session_module/session_set_save_handler_variation5.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ Read [%s,PHPT-%d]
GC [0]
1 deleted
bool(true)
Write [%s,PHPT-%d,]
Update [%s,PHPT-%d]
Close [%s,PHPSESSID]
bool(true)
string(%d) "PHPT-%d"
Expand Down
Loading