[php-src] Issue #9583: session_create_id() fails with user defined save handler that doesn't have a validateId() method

Issue: https://github.com/php/php-src/issues/9583 Author: warenzema ### Description The following code: ```php <?php declare(strict_types=1); class SessionHandlerTester implements \SessionHandlerInterface { public function close(){return true;} public function destroy($id){return true;} public function gc($max_lifetime){return true;} public function open($path, $name){return true;} public function read($id){return '';} public function write($id, $data){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(); echo "\nvalidateId() ".(method_exists($obj,'validateId')?('returns '.($obj->validateId(1)?'true':'false')):'is commented out'); echo "\n"; $sessionId = session_create_id(); echo "\nSession ID:".$sessionId; echo "\n"; ``` Resulted in this output: ``` [vagrant@local-test PHPUnit]$ php sessionCreateIdTest.php validateId() is commented out PHP Warning: session_create_id(): Failed to create new ID in /home/vagrant/sessionCreateIdTest.php on line 31 Session ID: ``` But I expected this output instead: ``` [vagrant@local-test PHPUnit]$ php sessionCreateIdTest.php validateId() is commented out Session ID:ismv386i33uasd5612o816ua54 ``` Here is a summary of the behavior of the above code across various versions, where I tested commenting out the validateId(), having it always return false, and have it always return true: |Version|no validateId()|validateId() returns false|validateId() returns true| |---|---|---|---| |7.2.26|||| |7.2.34||Error|| |7.4.30|Error||Error| |8.0.20|Error||Error| The varying behavior over time is believed to be due to two other bug fixes: [Fix #79091](https://github.com/php/php-src/commit/f79c7742746907d676989cb7f97fb4f7cd26789f) and [Fix #79413](https://github.com/php/php-src/commit/b510250b8ebe9d90b1db150d7a1edc75893f2e48) (The older versions above are listed because they represent the progression of behavior as those fixes were applied. I know most are out of support.) Those bug fixes created line 2325 below and changed FAILURE to SUCCESS on line 2323, respectively. (No problem with those bug fixes, them being fixed simply revealed this additional bug, and are merely provided for context.) https://github.com/php/php-src/blob/1084715494a1964f3caa69821cea636c75b2491d/ext/session/session.c#L2315-L2342 ## Hypothesis I don't know for sure what the issue is, as I'm unfamiliar with the language the source code is written in, but here's is the best I can work out from clues: Line 2319 contains the actual bug, as (I'm guessing here a little) that line is **meant** to check if the validateId() method exists on the save handler. If it does not exist, the code skips the collision check and just creates the new id without error (2335 is true). However, that line doesn't actually do that, as it appears that validateId() is **always** defined and always returns true if the user defined save handler doesn't override it. (In other words, it claims that every ID is already in use.) That may be due to the following code (again, guessing): https://github.com/php/php-src/blob/1084715494a1964f3caa69821cea636c75b2491d/ext/session/session.c#L1085-L1088 As a result, 2319 is always false, so the 2321 else block always executes, and 2323 is always true, which always ends with new_id = NULL which after 3 loops lands the code at 2340, emitting the warning. This explanation matches the chart of versions above: 7.2.26: With no new_id = NULL there was no way to fail 7.2.34 added new_id = NULL on 2325, which would cause 2323 to evaluate to true if validateId() returned false Later versions then fixed 2323 to be true if validateId() returned true, which inverted (relative to 7.2.34) the behavior we see. (I was unable to test other versions as I need to install PHP via amazon-linux-extras, which only on 7.2 allowed me to select which patch to install, didn't allow 7.3 at all, nor 8.1. Installing 7.4 and 8.0 got me 7.4.30 and 8.0.20.) ## Workaround A user can work around this issue by simply adding validateId($key){return false;} to their user defined save handler. To be safer, have it return true if the passed in $key is already in use, and false otherwise. See [documentation](https://www.php.net/manual/en/sessionupdatetimestamphandlerinterface.validateid.php). ## Comments I would imagine this bug is pretty widespread in general, as most user defined save handlers will not have a validateId() method defined (AWS PHP SDK doesn't, which is why I encountered this issue.) However, most code probably doesn't ever call create_session_id(), which means this problem is encountered extremely rarely. (I think I failed at being "brief", so here's a 4 line TLDR. Sorry, I didn't want my many hours of digging to go to waste.) ## TLDR ``` ini_set('session.use_strict_mode','1'); session_set_save_handler(new SessionHandlerTester()); session_start(); session_create_id(); ``` Causes the following error if SessionHandlerTester doesn't have a validateId() method: ``` PHP Warning: session_create_id(): Failed to create new ID in <the last line> ``` ### PHP Version 8.0.20 ### Operating System _No response_