Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug #71038 - session_start() returns true even when it failed #2167

Closed
wants to merge 24 commits into from

Conversation

5 participants
@yohgaki
Copy link
Contributor

commented Oct 16, 2016

Sorry for late bug fix for important feature.
I felt complete fix requires RFC and I forgot to create RFC for this.
https://bugs.php.net/bug.php?id=71038

In short, this patch fixes session_start()'s insane behaviors.

NOTE: There is no functional changes at all, but only error handling changes. Therefore, proper codes will not be affected by this change. Only bad codes are detected.

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.

I pushed patch ( 637f72c and later commits) fixes number of nonsense/inconsistent session function behaviors. Many of them are impossible to fix due to improper session status management. This bug fix allows us to fix many error handling issues in session module. The additional patch is pushed so that it's easy to cherry pick minimum fixes. 637f72c and later commits are additional fixes other than session_start() return value issue.

Change Summary:

  • Disallow nonsense function usages/behaviors.
    • 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()
    • Do not allow nonsense INI changes by ini_set()
      • All of cookie related INIs, session.use_strict_mode and session.referer_check.
    • Return function status properly.
      • session_start() - It sets correct session status always by this patch.
    • Return function status as it should. Previously void.
      • session_abort() session_flush() session_commit() session_write_close() session_reset() session_destroy()
  • Raise proper error and return status.
    • Many functions.

Yasuo Ohgaki added some commits Oct 16, 2016

Yasuo Ohgaki
Yasuo Ohgaki
Yasuo Ohgaki
Since session_start() issue is fixed. Number of inconsistent behavior…
…s can be fixed also.

 - Disallow nonsense function usage that has no effect
 - Return proper value on failure
 - Show proper errors on failure

Additionally

 - Made session_flash()/session_commit()/session_write_close() report errors and return bool, parameter is checked as other functions.
@KalleZ

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

Maybe a side thing, but Yasuo, from how I read php_session_start(), then it is possible for it to return SUCCESS and have PS(id) be NULL, is this intended, or?

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

@KalleZ Do you mean php_session_start()? Then yes, it is intended. If you read code closely, PS(id) is set eventually for successful call. Otherwise, many PHP sites will have severe problems.

When session cannot be started, it results in empty session ID because it does not make sense to have unusable session ID. Is there any valid use case for this?

Yasuo Ohgaki
@@ -1531,36 +1542,50 @@ PHPAPI void php_session_start(void) /* {{{ */
PS(id) = NULL;

This comment has been minimized.

Copy link
@KalleZ

KalleZ Oct 17, 2016

Member

@yohgaki I mean here for example, don't we miss a return as well?

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 17, 2016

Author Contributor

@KalleZ Thank you for checking!
php_session_start() only initializes session ID from client. Invalid session ID or initial access can result in PS(id) == NULL. Therefore there should not be "return" here.

There is php_session_initialize() bellow. It creates new session ID when PS(id) == NULL. If it failed for reasons, it should return FAILURE with PS(id) == NULL or PS(id) != NULL.

As you can see from this PR. Session module has a lot of problems and legacy codes. There may be redundant/strange/obsolete codes. Some are intentional, but please let me know if you find anything.

This comment has been minimized.

Copy link
@KalleZ

KalleZ Oct 17, 2016

Member

@yohgaki Thank you for clarifying, the session module does seem a mess at some parts and I was just trying to understand it a bit better. I will let you know if I spot anything obvious.

@KalleZ

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

@yohgaki hmm it may be me then but as I read the code then there is at least one such case where PS(id) can be NULL and php_session_start() be a SUCCESS. I added an inline comment

Yasuo Ohgaki added some commits Oct 17, 2016

Yasuo Ohgaki
Yasuo Ohgaki
Yasuo Ohgaki
Yasuo Ohgaki
STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateString, extern_referer_chk, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cache_limiter", "nocache", PHP_INI_ALL, OnUpdateString, cache_limiter, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cache_expire", "180", PHP_INI_ALL, OnUpdateLong, cache_expire, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cookie_lifetime", "0", PHP_INI_ALL, OnUpdateSessionLongGEZero, cookie_lifetime, php_ps_globals, ps_globals)

This comment has been minimized.

Copy link
@pierrejoye

pierrejoye Oct 18, 2016

Contributor

OnUpdateSessionLongGEZero makes little sense, OnUpdateSessionLifetime sounds more obvious as name.

This comment has been minimized.

Copy link
@pierrejoye

pierrejoye Oct 18, 2016

Contributor

or even better OnUpdateSessionTTL (TTL is only > 0)

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 18, 2016

Author Contributor

It's not reused, so it would be better to use dedicated name. I'll

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 18, 2016

Author Contributor

It kept TTL for other usage and used Lifetime for this.
i.e. Time stamped session management.

@yohgaki yohgaki changed the base branch from master to PHP-7.1 Oct 18, 2016

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2016

Why does travis not fail to build always, but on occasion when base branch is wrong??

It seems multiple tests are running on build env...

@@ -1353,15 +1402,17 @@ static void ppid2sid(zval *ppid) {
}
}

PHPAPI void php_session_reset_id(void) /* {{{ */

/* Made to return int from 7.1, previously void */

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

We don't need to comment on this in the code, this should be in the manual and UPGRADING.

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 19, 2016

Author Contributor

OK. I'll.

static PHP_INI_MH(OnUpdateCookieLifetime) /* {{{ */
{
SESSION_CHECK_OUTPUT_STATE;
return OnUpdateLongGEZero(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

It was OnUpdateLong before, why it changed?

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 19, 2016

Author Contributor

Negative lifetime is treated the same as 0. It is mitigation for integer overflow that user would like to set very long lifetime, but it overflowed by computation. It's unlikely, so it could be OnUpdateLong. Should I?

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 20, 2016

Contributor

PHP numbers don't work this way - if you add two large integers, it converts to float, not negative number.

PHP_INI_ENTRY("session.use_trans_sid", "0", PHP_INI_ALL, OnUpdateTransSid)
PHP_INI_ENTRY("session.sid_length", "32", PHP_INI_ALL, OnUpdateSidLength)
PHP_INI_ENTRY("session.sid_bits_per_character", "4", PHP_INI_ALL, OnUpdateSidBits)
STD_PHP_INI_BOOLEAN("session.lazy_write", "1", PHP_INI_ALL, OnUpdateBool, lazy_write, php_ps_globals, ps_globals)
STD_PHP_INI_BOOLEAN("session.lazy_write", "1", PHP_INI_ALL, OnUpdateSessionBool, lazy_write, php_ps_globals, ps_globals)

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Why I can't change lazy write after session start? Write happens at the end, not at the start.

This comment has been minimized.

Copy link
@yohgaki

yohgaki Oct 19, 2016

Author Contributor

lazy_write stores read session data, so this setting should be enabled before session_start(), otherwise it does not take the effect. I should fix this! Thank you!

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 20, 2016

Contributor

Well, I think still not much of a problem enabling lazy writes - worst thing, it would not be lazy if the data to do lazy part is not there.

php_error_docref(NULL, E_WARNING, "Cannot set cookie parameters - headers already sent");
RETURN_FALSE;
}

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Shouldn't we also check session state? If session state is wrong, INI functions will fail anyway, so why not check it if we're already checking this?

@@ -1675,6 +1770,11 @@ static PHP_FUNCTION(session_module_name)
return;
}

if (name && PS(session_status) == php_session_active) {
php_error_docref(NULL, E_WARNING, "Cannot change save handler module when session is active");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Why we need this check? According to test bug73100.phpt session_module_name already fails when session is active.

@@ -1867,6 +1968,11 @@ static PHP_FUNCTION(session_save_path)
return;
}

if (PS(session_status) == php_session_active) {
php_error_docref(NULL, E_WARNING, "Cannot change save path when session is active");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Same here - why not?

@@ -1710,7 +1810,8 @@ static PHP_FUNCTION(session_set_save_handler)
zend_string *name;
zend_string *ini_name, *ini_val;

if (PS(session_status) != php_session_none) {
if (PS(session_status) == php_session_active) {
php_error_docref(NULL, E_WARNING, "Cannot change save handler when session is active");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Why we can't change it here, actually? Would something work wrong? Yes, we may use different handlers - so what? Changing write handler before write happens should not be a problem, should it?

@@ -2074,6 +2182,11 @@ static PHP_FUNCTION(session_cache_limiter)
zend_string *limiter = NULL;
zend_string *ini_name;

if (PS(session_status) == php_session_active) {
php_error_docref(NULL, E_WARNING, "Cannot change cache limiter when session is active");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

Here I would rather tell about headers since session being active is not the problem here - the problem here would be we already sent the headers, so we can't take them back.

@@ -2177,6 +2295,11 @@ static PHP_FUNCTION(session_start)
RETURN_FALSE;
}

if (PS(session_status) == php_session_active) {
php_error_docref(NULL, E_NOTICE, "A session had already been started - ignoring");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

I'm not sure this is right. Certainly a BC break. You may have legitimate case for replacing session data from third party source. Weird, but people do weird things. Is there a reason to prohibit it?

@@ -2227,14 +2356,23 @@ static PHP_FUNCTION(session_destroy)
return;
}

if (PS(session_status) != php_session_active) {
php_error_docref(NULL, E_WARNING, "Trying to destroy uninitialized session");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Oct 19, 2016

Contributor

I'm not sure about this one... There might be code that calls destroy repeatedly, just in case - e.g. error handlers, etc. - and putting warning there does not help much. Since the session stays destroyed, there's not real reason to react with a failure error. I'd just silently return, maybe return false at worst. Warning should be when whatever you tried to do failed, but here it didn't actually fail - it was just unnecessary.

yohgaki added some commits Oct 23, 2016

@yohgaki yohgaki changed the base branch from PHP-7.1 to master Oct 23, 2016

yohgaki added some commits Oct 23, 2016

Add doc. Check both output and active status. Changes after header se…
…nt will never work. Should not change setting while session is active because it does not work.
@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2016

All issue should be resolved. Please let me know if you find any.

All INI changes should not be done while active. Once output is started, both cookie and trans sid session will not work correctly.

php-pulls pushed a commit that referenced this pull request Nov 17, 2016

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2016

@yohgaki yohgaki closed this Nov 17, 2016

dstogov added a commit to zendtech/php-src that referenced this pull request Nov 17, 2016

Merge branch 'master' into jit-dynasm
* master:
  Fix bug #71038 - session_start() returns true even when it failed PR php#2167
  Remove PDOStatement::activeQueryString()
@SailorMax

This comment has been minimized.

Copy link

commented Jun 28, 2017

@yohgaki
Looks like you broke getting session name via session_name() without arguments. Now it output "Cannot change session name when headers already sent"
https://bugs.php.net/bug.php?id=74514

Can you add check on no arguments before output warning?
"if (SG(headers_sent)) {" -> "if (name && SG(headers_sent)) {"
in "static PHP_FUNCTION(session_name)"
and "static PHP_FUNCTION(session_module_name)"

thank you.

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

@SailorMax
I might not understand the issue well yet, but it seems something wrong.

With current master

[yohgaki@dev PHP-master]$ ./sapi/cli/php -r 'var_dump(session_name("foo"));var_dump(session_name());'
string(9) "PHPSESSID"

Warning: session_name(): Cannot change session name when headers already sent in Command line code on line 1
bool(false)

2nd session_name() should not try to send HTTP Cookie header. The referred bug report problem seems this is the cause.

Following what it should do. (tested with 7.0)

$ php -r 'var_dump($n=session_name("foo"));var_dump(session_name());'
string(9) "PHPSESSID"
string(3) "foo"

Let's continue discussion on the
https://bugs.php.net/bug.php?id=74514

@SailorMax

This comment has been minimized.

Copy link

commented Jun 30, 2017

@yohgaki
2nd session_name() did not send any headers. It just has wrong "if":
at ext/session/session.c :: static PHP_FUNCTION(session_name)
"if (SG(headers_sent)) {" instead of "if (name && SG(headers_sent)) {"
It has to check headers only when got new name.

Same problem with session_module_name()
Also think about session_save_path(), session_cache_limiter(), and session_cache_expire()

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

@SailorMax
You mean
"if (name && SG(headers_sent)) {" instead of "if (SG(headers_sent)) {"
I should have considered non output buffered environment. Thanks.

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

@SailorMax
Simply adding non null parameters check have problems still on CLI which ignores HTTP headers at all.
For instance,

var_dump(session_name('foo'));
var_dump(session_module_name('files')); <-- Headers are already sent on Web SAPI w/o buffering, but not with CLI.

SG(headers_sent) should return false always with CLI. However, many test scripts may rely on SG(headers_sent) being true.

I thought about disabling the checks for CLI build, but we have CLI server...

"if (name && SG(headers_sent)) {", this change is good, but it fixes issue partially for CLI. It may be better leave this as documentation issue for 7.2 because it does not make much sense to optimize behavior for non output buffered CLI, i.e. Web SAPI should enable output buffer to work.

Anyway, the "name" check should be added. I'll commit the changes for 5 session functions.

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.