Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Fixing insane session_start() behaviors »

Fixing insane session_start() behaviors

php.internals

Yasuo Ohgaki

9 years ago
Hi Davey and all, I was planning to fix session_start() behaviors by PHP 7.1, but I forgot to do this completely. Partial fix is merged currently. Following PR makes session_start() return FALSE when it cannot start session always. https://github.com/php/php-src/pull/2167 In short, this patch fixes number of session_start()'s insane behaviors. Original session_start() is designed to continue execution as much as it can. This design caused a lot of issues including number of crash bugs. We've removed most issues caused by this design, but session_start() behaves insane way. e.g. Return TRUE and initializes $_SESSION array for useless session, improper error messages, memory leak, etc. (Please verify phpt changes how this patch makes session_start() behave sane way) This fix may change app behavior. However, it changes behavior only when there is useless session which is fatal anyway. Therefore, it could be applied to PHP 7.1. IMO. What do you think? Regards, P.S. Sorry for late important bug fix.
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago
Hi Davey, On Sun, Oct 16, 2016 at 6:08 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> > I was planning to fix session_start() behaviors by PHP 7.1, but I > forgot to do this completely. Partial fix is merged currently. > > Following PR makes session_start() return FALSE when it cannot start > session always. > > https://github.com/php/php-src/pull/2167 > > In short, this patch fixes number of session_start()'s insane behaviors. > > Original session_start() is designed to continue execution as much as > it can. This design caused a lot of issues including number of crash > bugs. We've removed most issues caused by this design, but > session_start() behaves insane way. e.g. Return TRUE and initializes > $_SESSION array for useless session, improper error messages, memory > leak, etc. (Please verify phpt changes how this patch makes > session_start() behave sane way) > > This fix may change app behavior. However, it changes behavior only > when there is useless session which is fatal anyway. Therefore, it > could be applied to PHP 7.1. IMO. > > What do you think?
Since session_start() is made to set proper session status finally, many bugs and inconsistencies can be fixed altogether. I pushed patch fixes number of nonsense/inconsistent session function behaviors. The additional patch is pushed so that it's easy to cherry pick minimum fixes. The last push is the additional fixes. - Disallow nonsense function usage. . Do not allow to change cookie parameters when it has no effects. session_set_cookie_params() session_cache_limiter() . Dn not allow to change INI parameters when it cannot be changed. session_name() session_module_name() session_save_path() session_set_save_handler() session_cache_expire() . Do not allow parameter for void parameter functions. session_unset() session_write_close() session_commit() session_abort() session_reset() - Return function status as it should. . session_abort() session_flush() session_commit() session_write_close() session_reset() session_destroy() - Raise proper errors. . Many functions. Session module allows insane usage currently. This will fix most of them. I need to address nonsense INI usage. There will be one more push at least for this. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

9 years ago
Hi!
> I pushed patch fixes number of nonsense/inconsistent session function > behaviors. The additional patch is pushed so that it's easy to cherry > pick minimum fixes. The last push is the additional fixes.
These changes look like a reasonable cleanup. I'm not a big fan of zend_parse_parameters_none additions, since I don't see any useful function for them, but they don't hurt either. I left some notes on the patch though, please look there. I'm not sure about 7.1 - while I don't see anything that would break any reasonable code, except issues noted on the pull, the patch is pretty big and I might have missed something. I guess it's for RM to decide. No objection for 7.2. I notice that most test changes deal with scenarios that were covered before, are there tests that test the new things - the scenarios for which handling has changed? I couldn't find tests for some cases, like INI modification, would be nice to add them. Also, a reminder that you'd have to update all the documentation in the manual to reflect the change in the return values. And UPGRADING too.
-- Stas Malyshev smalyshev@gmail.com

Davey

9 years ago
On Tue, Oct 18, 2016 at 11:08 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > I pushed patch fixes number of nonsense/inconsistent session function > > behaviors. The additional patch is pushed so that it's easy to cherry > > pick minimum fixes. The last push is the additional fixes. > > These changes look like a reasonable cleanup. I'm not a big fan of > zend_parse_parameters_none additions, since I don't see any useful > function for them, but they don't hurt either. I left some notes on the > patch though, please look there. > > I'm not sure about 7.1 - while I don't see anything that would break any > reasonable code, except issues noted on the pull, the patch is pretty > big and I might have missed something. I guess it's for RM to decide. No > objection for 7.2. > > I notice that most test changes deal with scenarios that were covered > before, are there tests that test the new things - the scenarios for > which handling has changed? I couldn't find tests for some cases, like > INI modification, would be nice to add them. > > Also, a reminder that you'd have to update all the documentation in the > manual to reflect the change in the return values. And UPGRADING too.
> I was planning to fix session_start() behaviors by PHP 7.1, but I > forgot to do this completely. Partial fix is merged currently.
Yasuo, assuming "partial fix" doesn't mean "broken fix" but instead "it doesn't do everything I planned" then I do not want this in 7.1. As others have pointed out, it's not a small change and sessions are a fundamental part of PHP — we are simply too far into the release cycle to include this. Please target 7.2/master. Thanks, - Davey

Yasuo Ohgaki

9 years ago
Hi Davey, On Wed, Oct 19, 2016 at 6:06 PM, Davey Shafik <davey@php.net> wrote:
> Yasuo, assuming "partial fix" doesn't mean "broken fix" but instead "it > doesn't do everything I planned" then I do not want this in 7.1. As others > have pointed out, it's not a small change and sessions are a fundamental > part of PHP — we are simply too far into the release cycle to include this. > > Please target 7.2/master.
OK. I'll merge this to master. Partial fix is done so that it does not break internal API signatures. It's fixed as plain bug fix. PHP 7.1's session_start() sets a little better session status and 'FALSE' than older PHP, but it's far from correct/reliable. I'll update UPGRADING comment. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago
Hi Stas, On Wed, Oct 19, 2016 at 3:08 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> I pushed patch fixes number of nonsense/inconsistent session function >> behaviors. The additional patch is pushed so that it's easy to cherry >> pick minimum fixes. The last push is the additional fixes. > > These changes look like a reasonable cleanup. I'm not a big fan of > zend_parse_parameters_none additions, since I don't see any useful > function for them, but they don't hurt either. I left some notes on the > patch though, please look there. > > I'm not sure about 7.1 - while I don't see anything that would break any > reasonable code, except issues noted on the pull, the patch is pretty > big and I might have missed something. I guess it's for RM to decide. No > objection for 7.2. > > I notice that most test changes deal with scenarios that were covered > before, are there tests that test the new things - the scenarios for > which handling has changed? I couldn't find tests for some cases, like > INI modification, would be nice to add them. > > Also, a reminder that you'd have to update all the documentation in the > manual to reflect the change in the return values. And UPGRADING too.
Thank you. I replied to your comments. There is one debatable comment regarding session_cache_limiter(). We may delay error check at 'headers sent'. I suppose almost all usage is misuse. Only valid use case is <?php ob_start(); session_start(); session_set_cache_limiter('public'); // <== Call this between session_start() and session_regenerate_id() session_regenerate_id(); ?> Other than this, all calls after session_start() is invalid. Valid use case is very limited and unlikely. If users have to change cache limiter, they can session_commit(), then session_start(). So I would like to keep as it is now. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago
Hi Stas, On Wed, Oct 19, 2016 at 8:57 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Only valid use case is > > <?php > ob_start(); > session_start(); > session_set_cache_limiter('public'); // <== Call this between > session_start() and session_regenerate_id() > session_regenerate_id(); > ?> > > Other than this, all calls after session_start() is invalid. Valid use > case is very limited and unlikely. If users have to change cache > limiter, they can session_commit(), then session_start(). So I would > like to keep as it is now.
I thought php_session_reset_id() sends all headers, but it sends only session ID cookie header. So current code is good. Sorry for confusion. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

9 years ago
Hi Davey, On Sun, Oct 16, 2016 at 6:08 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> > I was planning to fix session_start() behaviors by PHP 7.1, but I > forgot to do this completely. Partial fix is merged currently. > > Following PR makes session_start() return FALSE when it cannot start > session always. > > https://github.com/php/php-src/pull/2167 > > In short, this patch fixes number of session_start()'s insane behaviors.
I think I've finished the patch for now. There are many other places that I would like to improve/fix error handlings. e.g. Session save handlers. This patch is too large already. I'll fix them in master. The patch could be divided into "session_start() error handling fix only" and "other session core error handling issues". It may be good to include "session_start() error handling fix only" for PHP 7.1, but other fixes won't bother correct PHP scripts. Please note that session_start() fix in PHP-7.1 and master branch is partial fix. I suggest to merge "session_start() error handling fix only" at least, but it's your choice. Please let me know when you decide what to do or merge appropriate patch from the PR. BTW, I expect UnitTest failures, framework/etc, that test session module functionalities. They have tests for failure cases. Even if there are minor BC issues (UnitTests report error, bad code emit errors), this fix must be done at some point anyway. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pierre Joye

9 years ago
Hi Yasuo, On Mon, Oct 17, 2016 at 4:47 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Davey, > > On Sun, Oct 16, 2016 at 6:08 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: >> >> I was planning to fix session_start() behaviors by PHP 7.1, but I >> forgot to do this completely. Partial fix is merged currently. >> >> Following PR makes session_start() return FALSE when it cannot start >> session always. >> >> https://github.com/php/php-src/pull/2167 >> >> In short, this patch fixes number of session_start()'s insane behaviors. > > I think I've finished the patch for now. > > There are many other places that I would like to improve/fix error > handlings. e.g. Session save handlers. This patch is too large > already. I'll fix them in master. > > The patch could be divided into "session_start() error handling fix > only" and "other session core error handling issues". It may be good > to include "session_start() error handling fix only" for PHP 7.1, but > other fixes won't bother correct PHP scripts. Please note that > session_start() fix in PHP-7.1 and master branch is partial fix. I > suggest to merge "session_start() error handling fix only" at least, > but it's your choice. > > Please let me know when you decide what to do or merge appropriate > patch from the PR. > > BTW, I expect UnitTest failures, framework/etc, that test session > module functionalities. They have tests for failure cases. Even if > there are minor BC issues (UnitTests report error, bad code emit > errors), this fix must be done at some point anyway.
I like this change as it adds some cleanup and common sense to this API. However 7.1 is in RC, this patch, while covering logical changes, affect behaviors and is quite big (lot of similar changes but still). I would feel much more comfortable to push it after 7.1 (as in, for 7.2 and later). Cheers,
-- Pierre @pierrejoye | http://www.libgd.org