Bug #76358 [Com]: Cannot change session name when session is active

Edit report at https://bugs.php.net/bug.php?id=76358&edit=1 ID: 76358 Comment by: tony at marston-home dot demon dot co dot uk Reported by: tony at marston-home dot demon dot co dot uk Summary: Cannot change session name when session is active Status: Assigned Type: Bug Package: Session related Operating System: Windows 10 PHP Version: 7.2.5 Assigned To: yohgaki Block user comment: N Private report: N New Comment: These are the five code samples that Yasuo sent me as "proof" of the broken behaviour with the session functions which his fix supposedly cures. Below each example is my response which shows that the problem actually lies with the code that calls those functions and not the functions themselves. Example #1 - Wrong write and/or crash> <?php session_start(); session_name('new_name'); session_commit(); // Save handler can write session data to wrong storage. There is no such thing as "wrong" storage in this case as session_commit() does NOT reference $session_name when calling the write() method in the session handler, it uses $session_id. This code does not change $session_id so the session data is put back in the same place from where it was read. Example #2 - Wrong name returned <?php session_start(); session_name('new_name'); // somewhere in the code $my_current_session_name = session_name(); // Wrong session name. session_start() uses old one. // This behavior can cause bug when user is distributing session storage accesses by session name. There are a number of glaring mistakes here: - Changing the value of session.name does not take effect until the next call to session_start(). When a session is started the value of session.name is provided in the $session_name argument in the open() method in the session handler. - No method or function has ever been provided which will return the value of $session_name which was used in the call to open(). - If the value of session.name is changed while a session is active that change is not communicated with the session handler and does not affect the session handler in any way as all read() and write() operations are performed using the $session_id. - If the session name is used to change the value in $save_path then it should be done in the open() method in the session handler where both $save_path and $session_name are provided as arguments. If these values need to be used in other methods then they should be stored as class properties and accessed using $this->varname. Example #3 - Wrong write and/or crash. Pattern 2. <?php session_start(); session_name('new_name'); // somewhere in the code session_regenerate_id(); // Save handler can write session data to wrong storage. // In worst cases, PHP crashes ?> Bad usage again. session_regenerate_id() will regenerate a new id for the CURRENT session, but this will be linked with the value of session.name which was available when the current session was started. The value 'new_name' will not take effect until the next call to session_start(), just as the manual says. Example #4 - Wrong write and/or crash. Pattern 3. <?php session_start(); session_name('new_name'); session_write(); // Save handler can write session data to wrong storage. // In worst cases, PHP crashes ?> This is the same as Example #1. Example #5 - Wrong write and/or crash. Pattern 4. + Bogus session_name() call. <?php session_start(); session_name('new_name'); // Save handler can write session data to wrong storage at the end of script execution. // In worst cases, PHP crashes // In addition, user wouldn't notice bogus session_name() call if there is no error. ?> This is a duplicate of #1 and #4. The session is started with a particular $session_id, and as that $session_id is never changed any updates to that session data will be written back using the same $session_id. A change in $session_name does not change the $session_id. The $session_name and $session_id are different entities which are accessed using different functions. They are only brought together when the cookie is accessed, which is either by session_start() or session_regenerate_id(). This code does nothing to cause 'new_name' to be written out as a cookie, therefore it simply disappears. Previous Comments: ------------------------------------------------------------------------ [2018-06-06 09:50:10] requinix@php.net Related To: Bug #76413 ------------------------------------------------------------------------ [2018-05-31 11:09:37] tony at marston-home dot demon dot co dot uk > Not every (minor) BC breaking change requires an RFC or discussion on the internals mailing list. There is no such thing as a "minor" BC break. Every BC break causes code that previously worked to suddenly fail for no good reason. Every BC break should be discussed on the internals list so that other developers can review it to see if it is actually justified or can be improved. It must then be voted upon, which then means that the change MUST be subject to an RFC. The idea that a developer can sneak in a BC break WITHOUT it being discussed on the internals list and WITHOUT any prior warning or even a mention in the change log will create a bad smell for the millions of application developers who expect the language to be reliable and stable. It is the fear of BC breaks which causes developers to delay upgrading to the latest version of the language. > This very change was submitted as pull request on Github (which is an official and publicly > available channel) GitHub may be the official channel for pull requests, but it is *NOT* the official channel for discussions. There were *NO* discussions on the proposed changes to session handling which identified this change to session_name(). > I still do not think that your interpretation of the (former) documentation is absolutely correct; at the very least there is some room for interpretation. The documentation at http://php.net/manual/en/function.session-name.php is perfectly clear - the session_name() function can be used either to get the current name or set a new one, or even both at the same time. Example #1 in the manual clearly demonstrates this usage. This means that it has always been perfectly legitimate to call session_name() even if a session is already active. The documentation also states: "The session name is reset to the default value stored in session.name at request startup time. Thus, you need to call session_name() for every request (and before session_start() is called)." This tells me that if you change the session name that it will not be effective until the next call to session_start(). > Yasuo claims … "the former behavior is not valid and allows crash and > misbehaviors” I have been conversing with Yasuo via private email on this claim, and he sent me five examples of code where there was a call to session_name('newname') after a call to session_start() and it did not work as expected. This is simply because there was *NO* call to session_start() after a change in name EVEN THOUGH THE DOCUMENTATION SAYS THAT THERE SHOULD BE, or that the 2nd call to session_start() failed because a session was already active. The documentation at http://php.net/manual/en/function.session-start.php clearly states: "As of PHP 4.3.3, calling session_start() after the session was previously started will result in an error". It is obvious to me that if session_name('newname') is called when a session is already active then the new name will not take effect until the next call to session_start() and that the 2nd call to session_start() must be preceded by a call to session_wtite_close(). It was a mistake on Yasuo's part to think that it was the call to session_name() which should be preceded by session_write_close(). This mistake was brought about by his misunderstanding of how the various session functions could legitimately be used. He actually admitted to me in one of his emails: "I didn't expect users to call session_name() after session_start() because it does not make sense with session module structure". There you have it. It didn't make sense to him, so he changed the behaviour of the function to forbid it. Even worse, he changed the legitimate behaviour of this function without any discussion or warning whatsoever, and without even a mention in the change log. This is a very bad precedent which, if allowed to continue in the future, will cause millions of application developers to lose confidence in the language. We (because I am one of those application developers) expect the language to be reliable and stable, and by "stable" I do *NOT* mean "full of manure". ------------------------------------------------------------------------ [2018-05-30 21:30:15] cmb@php.net > I think that it is a sad day when somebody can decide that the > documented behaviour of a 20 year old function does not fit it > with their beliefs, so they go ahead and change it without > following the correct procedure - raise RFC, discuss it on the > internals list, then put it to a vote. Not every (minor) BC breaking change requires an RFC or discussion on the internals mailing list. This very change was submitted as pull request on Github (which is an official and publicly available channel), and since there have been no general objections, the PR was committed. Also note, that I still do not think that your interpretation of the (former) documentation is absolutely correct; at the very least there is some room for interpretation. Anyhow, assigning to Yasuo, who recently committed a documentation change[1] which claims that the former behavior “is not valid and allows crash and misbehaviors”. Maybe some short explanation what could go wrong would be approriate here. [1] <http://svn.php.net/viewvc?view=revision&revision=345080> ------------------------------------------------------------------------ [2018-05-30 13:59:20] tony at marston-home dot demon dot co dot uk When the documentation at http://uk1.php.net/manual/en/function.session-name.php says "session_name() must be called before session_start() in order session to work properly" it actually means that the new session name will not take effect until the next call to session_start(). It will not change the name of any currently open session. ------------------------------------------------------------------------ [2018-05-29 10:34:32] tony at marston-home dot demon dot co dot uk When the documentation says that session_name() should be called before session_start() it means that the name change will not take effect until the next call to session_start(). The reason that I call session_start() before the call to session_name() is that I want to access the $_SESSION array for the current session so that I can copy it across to the new session. I actually want to have two browser windows running different parts of my application at the same time, and this will only work if each of the browser windows has its own session_id, and this requires each session to have its own name. I have been using these functions as documented since 2003 without any problems, so imagine my horror when this well-established behaviour was suddenly changed WITHOUT ANY WARNING WHATSOEVER. Breaks in BC should always be handled by following the correct procedures, and I'm afraid that those procedures were completely ignored on this occasion. It is perfectly valid to call session_name() while a session is active as it would otherwise be unable to follow the documentation and return the name of the current session. The documentation also states that this function can be used to both get *AND* set the session name at the same time, which means that it has always been possible to change the session name while a session is active. The new name will not take effect until the next call to session_start(), and it is *ONLY* session_start() that will fail if a session is already active. Thus it is *ONLY* necessary to close the current session before the 2nd call to session_start(), *NOT* the call to session_name(). The only bug with session_name() is that if you tried to both get *AND* set at the same time it would actually do neither. In changing the documented behaviour of this function they failed to fix a genuine bug and instead introduced an artificial one. Note that session_name does NOT change any session cookies. That is done either by session_regenerate_id() or session_start(). ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=76358 -- Edit this bug report at https://bugs.php.net/bug.php?id=76358&edit=1