I'm not sure if I raised this correctly, but could this fix also be applied to PHP 8.0 as this is the version available for oraclelinux 9

Requesting a review from George because this was changed recently. (499fbcd)

FWIW, there's another loop of the same kind and the fix is effectively reverting a commit made by @cmb69:
b510250

Note that until I added a validateId function to my custom session handler, I could not get session_regenerate_id to work at all

ie the hidden validateId function caused me a world of pain until I happened to discover the session.use_strict_mode + custom validateId function

Additionally, the fix by @cmb69 seems incorrect to be in that that loop will always call create_sid, ignore the validateId response and just use the last value from create_sid anyway

However, I can look at adding a test case for invoking create_session_id during an active session as well

I added a test case for session_create_id for the failing and non failing validation during an active session.

I wonder if I misinterpreted the documentation. From: https://www.php.net/manual/en/sessionupdatetimestamphandlerinterface.validateid.php

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

So the old code might've actually been correct. We're supposed to return false to indicate that no session already exists.
But this seems to be the opposite of what the name implies, hence my confusion.
The first comment on that doc page is then also wrong I think.
I'm not sure anymore what the right semantics here are to be honest...

Oh god, this part of ext/session again... let me try to recall what the semantics are because they are very weird and unintuitive.

Sorry about the confusion @bgardner-noggin, we should probably clarify the docs indeed... And thank you @Girgias for digging into this.

Sorry about the confusion @bgardner-noggin, we should probably clarify the docs indeed... And thank you @Girgias for digging into this.

No worries, I think the fact that this is confusing is something to be concerned about, especially as this is related to a security feature. Might dedicate some time to actually propose an RFC fixing the whole user session handler stuff...

This still seems broken. It seems you have confused my test case above which was ensuring that if validateId returns FALSE, that create_sid is reinvoked. My actual bug is that validateId is returning TRUE, ie the session token is valid, but PHP internals don't care and calls create_sid + validateSid multiple times

In my real world code, after a user successfully authenticates, I am invoking session_regenerate_id to prevent session fixation attacks. This leads to my original scenario

create_sid() <-- returns a new securely random generated token and inserts into the database
validateId($sid) <-this invocation returns TRUE as the session id is valid and is in the database
create_sid() + validateId($sid) <-- repeat 3 times
...

surely create_sid should only be reinvoked if validateId returns false?

This still seems broken. It seems you have confused my test case above which was ensuring that if validateId returns FALSE, that create_sid is reinvoked. My actual bug is that validateId is returning TRUE, ie the session token is valid, but PHP internals don't care and calls create_sid + validateSid multiple times

In my real world code, after a user successfully authenticates, I am invoking session_regenerate_id to prevent session fixation attacks. This leads to my original scenario

create_sid() <-- returns a new securely random generated token and inserts into the database validateId($sid) <-this invocation returns TRUE as the session id is valid and is in the database create_sid() + validateId($sid) <-- repeat 3 times ...

surely create_sid should only be reinvoked if validateId returns false?

So I've continued to stare at the code, trying to dig through this mess and understand it.

I still believe your changed are incorrect because the test for bug 79413 is broken (as seen by the total failure of CI).

The issue you are having is with the collision detections, because your implementation of create_sid() actually creates the database entry. Looking at the implementations of the built-in modules (namely the file one) they only generate the session ID, they do not initialize it. The initialization happens in the read() method.

That's why the validate method fails for them, as the ID is not yet in a created state.

So... according to the current implementation, create_sid() is not meant to actually create the session, just generate a session ID, and read() is meant to take care of initialising a non-existent session. Not at all confusing semantics here...

Now the question is should session_regenerate_id()/session_create_id() try to do collision detection. Because, looking at the other built-in implementations again, they do their collision detections during the create_sid().

I didn't see the 79413 failure during my make test run. I'll take a look at this tomorrow.

What you say above make sense, except it is different to what you said previously where you stated

The purpose of the validateId() method is only to validate that the given ID exists in the data storage.

As well as what the docs of validateId state

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

Now if validateId is actually meant to be

Return True if the id is generated by us, but has no associated storage yet

then it makes sense, but the naming of the function and docs make me think otherwise.

Return True if the id is generated by us, but has no associated storage yet

This statement cannot be true though, because validateId is invoked during the course of a normal start_session, ie not creating a new session id or regenerating an id, but through the normal course of using an existing session id which is in storage already.

I didn't see the 79413 failure during my make test run. I'll take a look at this tomorrow.

What you say above make sense, except it is different to what you said previously where you stated

The purpose of the validateId() method is only to validate that the given ID exists in the data storage.

I don't see how what I said contradicts my other comments. That's still it's only purpose.

As well as what the docs of validateId state

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

Now if validateId is actually meant to be

Return True if the id is generated by us, but has no associated storage yet

then it makes sense, but the naming of the function and docs make me think otherwise.

Naming and docs are completely irrelevant in PHP.
The actual implementation is the source of truth, for better or for worse (mostly worse). And the docs need to reflect the actual implementation. Moreover, there has been little to no change in the implementation of that aspect since its introduction in 2016, so I'm just here trying to figure out what on earth is happening.

However, I never implied that validateId() is meant to mean:

Return True if the id is generated by us, but has no associated storage yet

I said the expectation is that create_sid() is to only generate the session ID without creating an entry in the storage mechanism. As you correctly pointed out that validateId() is called during session initialization.

However, I don't think the expectation that create_sid() should not create an entry in the storage mechanism is sensible and even less intuitive than the validateId() method. As if we follow the current design, it means that read() needs to take into account creation of entries. So I'm not sure that the current implementation is sensible trying to perform that ID collision check as this is the source of a bunch of these issues.

Just a side question, but it seems you've basically rolled out a custom session mechanism already. Why rely on ext/session and on $_SESSION?

So if I understand this correctly if I have a custom save handler

1. session_regenerate_id will invoke create_sid
2. create_sid should generate an id, but not insert it
3. validateId will be invoked, which will return false as the id is not in storage
4. read(id) should then insert the record

As to why I was using the session extension, this is because it provides other useful things, like managing the session cookie

However, the docs are not clear and I think i will end up using a completely custom model for this and other reasons, which is disappointing.

Eg another problem I can see from looking at the source code is that if I have a custom save handler that implements the SessionIDInterface, then I would expect session_create_id() would always call my custom handler, yet I can see that this is not the case when the session is not active and it will fallback to the internal PHP function

https://github.com/php/php-src/blob/master/ext/session/session.c#L2357

if (!PS(in_save_handler) && PS(session_status) == php_session_active) {
...
} else {
new_id = php_session_create_id(NULL);
}

So if I understand this correctly if I have a custom save handler

1. session_regenerate_id will invoke create_sid

2. create_sid should generate an id, but not insert it

3. validateId will be invoked, which will return false as the id is not in storage

4. read(id) should then insert the record

This is my understanding, yes. But I do think the semantics of 2 are bad and this might need some discussion on the Internals Mailing list as this behaviour is unintuitive.

As to why I was using the session extension, this is because it provides other useful things, like managing the session cookie

However, the docs are not clear and I think i will end up using a completely custom model for this and other reasons, which is disappointing.

Right, thanks for the explanation. Yeah, the docs are in a sad state of affairs for that section.

Eg another problem I can see from looking at the source code is that if I have a custom save handler that implements the SessionIDInterface, then I would expect session_create_id() would always call my custom handler, yet I can see that this is not the case when the session is not active and it will fallback to the internal PHP function

https://github.com/php/php-src/blob/master/ext/session/session.c#L2357

if (!PS(in_save_handler) && PS(session_status) == php_session_active) {
...
} else {
new_id = php_session_create_id(NULL);
}

Yeah, this does look problematic indeed...

Thanks for your time anyway.