Skip to content

Conversation

yohgaki
Copy link
Contributor

@yohgaki yohgaki commented Nov 10, 2016

This patch disables any invalid save handler calls such as recursive save handler calls. This disables many kinds of save handler abuses.

7.1 and up has session_create_id(). This patch allows to use session_create_id() in user session save handler also. i.e. Allows to call session_carete_id() to make custom session id by session ID creation handler.

https://bugs.php.net/bug.php?id=73461

This patch disables any invalid save handler calls.

if (PS(in_save_handler)) {
PS(in_save_handler) = 0;
php_error_docref(NULL, E_WARNING, "Cannot call save handler function recursive manner");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

"Cannot call session save handler recursively or "Cannot call session save handler in a recursive manner"

I took the liberty to add 'session', as it eases up where to look during debugging quickly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in any further error messages :)

Copy link
Contributor Author

@yohgaki yohgaki Nov 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KalleZ
Thanks for feedback! I'll update error message to
Cannot call session save handler in a recursive manner
because invalid calls do not have to be recursive call.

@krakjoe
Copy link
Member

krakjoe commented Nov 13, 2016

@yohgaki what is the status of this PR ?

@yohgaki
Copy link
Contributor Author

yohgaki commented Nov 15, 2016

@krakjoe
I refactored and cleaned up the code. It's ready to be merged, but I'll update error message according to @KalleZ suggestion. Please wait a little.

@krakjoe
Copy link
Member

krakjoe commented Nov 15, 2016

The bug report says that 7.x is affected, but git can't apply the patch to 7.0

It may need rebasing, or there may need to be a PR for 7.0 ?

@php-pulls
Copy link

Comment on behalf of krakjoe at php.net:

Adding comment

@php-pulls php-pulls added the Bug label Nov 15, 2016
@yohgaki
Copy link
Contributor Author

yohgaki commented Nov 16, 2016

@krakjoe
I made patch for 7.1 and up because this patch requires additional global var. Patch for 7.0 could be made if additional global var is OK for 7.0. Since the bug is due to user's session save handler abuse, I don't think this patch is required for 7.0 strictly. I'll prepare patch if this fix is required for 7.0.

@yohgaki
Copy link
Contributor Author

yohgaki commented Nov 16, 2016

@krakjoe This patch is ready to be merged to 7.1/master, unless there should be patch for 7.0, please merge this to 7.1 and master. Thank you.

php-pulls pushed a commit that referenced this pull request Nov 16, 2016
php-pulls pushed a commit that referenced this pull request Nov 16, 2016
* PHP-7.1:
  new entry for #2196
  Improve error message
  Fix test
  Refactor and cleanup implementation.
  Revert "Fix Bug #73461"
  Revert "Protect class based session save handler"
  Protect class based session save handler
  Fix Bug #73461
php-pulls pushed a commit that referenced this pull request Nov 16, 2016
@krakjoe
Copy link
Member

krakjoe commented Nov 16, 2016

Merged. Thanks ;)

php-pulls pushed a commit that referenced this pull request Nov 16, 2016
* 'master' of git.php.net:/php-src:
  news entry for #2196
  new entry for #2196
  Improve error message
  Fix test
  Refactor and cleanup implementation.
  Revert "Fix Bug #73461"
  Revert "Protect class based session save handler"
  Protect class based session save handler
  Fix Bug #73461
php-pulls pushed a commit that referenced this pull request Nov 16, 2016
* 'PHP-7.1' of git.php.net:/php-src:
  new entry for #2196
  Improve error message
  Fix test
  Refactor and cleanup implementation.
  Revert "Fix Bug #73461"
  Revert "Protect class based session save handler"
  Protect class based session save handler
  Fix Bug #73461
dstogov added a commit to zendtech/php-src that referenced this pull request Nov 16, 2016
* master:
  Fixed bug #73532 (Null pointer dereference in mb_eregi)
  news entry for php#2196
  new entry for php#2196
  Improve error message
  Fix test
  Refactor and cleanup implementation.
  Revert "Fix Bug #73461"
  Revert "Protect class based session save handler"
  Protect class based session save handler
  Fix Bug #73461
  update NEWS
  Add PDOStatement::activeQueryString()
@weltling
Copy link
Contributor

@yohgaki the new globals member should be moved into the gap, please see 5eeec01. That way a patch for 7.0 were fine, given there's no other BC breach. Could you please ping, when the new PR is there?

Thanks.

@weltling weltling closed this Nov 16, 2016
@krakjoe
Copy link
Member

krakjoe commented Nov 16, 2016

Good catch @weltling, ta ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants