session_regenerate_id with custom handler and use_strict_mode generates three new session ids

[bgardner-noggin]

Description

If I have set session.use_strict_mode = 1 and I have a custom session handler which implements the validateId function, then invoking session_regenerate_id calls

customhandler->create_sid
customhandler->validateId
customhandler->create_sid
customhandler->validateId
customhandler->create_sid
customhandler->validateId
customhandler->create_sid

Looking at the source code for session_regenerate_id, I can see the bug

https://github.com/php/php-src/blob/PHP-8.0.27/ext/session/session.c#L2263

while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {

Shouldn't this be

while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE) {

Note that this is the same code in master

PHP Version

PHP 8.0.27

Operating System

No response

[nielsdos]

This is indeed a bug, and I believe you found the correct fix :)
Would you like to create a PR for this with the fix you proposed?
In case you want to create a PR, please target branch PHP-8.1 and include a testcase as a .phpt file in ext/session/tests/. An extensive guide about testcases can be found at: https://qa.php.net/write-test.php

[ranvis]

@nielsdos Just to be clear, would you investigate this further?

php-src/ext/session/session.c

Lines 2297 to 2305 in 4177257

	/* 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));

validate_sid
A session ID is valid, if a session with that ID already exists.

I think the validate_sid user handler is expected to return false (FAILURE) if the session of the ID is just created.

[bgardner-noggin]

Why would validate_sid return false for an id just created? ie whats the point of creating three sids?

If you look further down, you can see that the "failed to ever validate a sid" case has a comment

// TODO warn that ID cannot be verified? else { }

[nielsdos]

The purpose of that code is to validate and ID, and when the validation fails, generate a new ID and validate that again.
This happens for up to limit times, so 3 times in this case.
The reason you're seeing 3 generations of sids is because the check condition is wrong as you correctly noticed. We're supposed to check against FAILURE, and only try again in that case. In case the validation was succesful, we don't have to try again to generate & validate.

[bgardner-noggin]

yep. I have a test case and I'm about to submit a pull request

[bgardner-noggin]

Raised pull request - #10813