Skip to content

Conversation

@conormcd
Copy link
Contributor

If the previous value of session.name was NULL then any call to
session_name($string) would result in a segmentation fault.

This changes the behaviour to set the value of session.name to
"PHPSESSID" if a blank value is given in php.ini or via -d on the
command line. There is already protection against setting it to NULL via
session_name() or ini_set().

@laruence
Copy link
Member

there are lots codes using PS(session_name) directly, so I don't think this fix is very okey.

I think the problem should be fixed in the "setting" stage. if a empty session name is set. we should restore to default value..

@krakjoe
Copy link
Member

krakjoe commented Jan 14, 2014

Yeah, wat @laruence said ...

@conormcd
Copy link
Contributor Author

I guess that makes sense. I'll have another crack at it in the morning then.

@conormcd
Copy link
Contributor Author

PR updated and description above changed to match that in the commit message.

If the previous value of session.name was NULL then any call to
session_name($string) would result in a segmentation fault.

This changes the behaviour to set the value of session.name to
"PHPSESSID" if a blank value is given in php.ini or via -d on the
command line. There is already protection against setting it to NULL via
session_name() or ini_set().
@conormcd
Copy link
Contributor Author

@laruence @krakjoe Does the above change seem more reasonable?

@yohgaki
Copy link
Contributor

yohgaki commented Jan 16, 2014

Newer patch(?) looks ok to me.

@yohgaki
Copy link
Contributor

yohgaki commented Jan 16, 2014

Merged. Thank you.

@conormcd
Copy link
Contributor Author

Excellent! Thanks. :)

@laruence
Copy link
Member

@conormcd @yohgaki sorry, but the fix is still no completely right, 1. it hard code the default value in the function. 2. we don't need to set the default value manually, we can simply return failure 3. should complain the wrong value set(let the user aware what he did is wrong). I will revert the fix @yohgaki merged, and make another one... thanks

@laruence
Copy link
Member

@conormcd committed, thanks for your help , could you please close this issue :)

@conormcd conormcd closed this Jan 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants