Bug #66481 Segfaults on session_name() #561

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@conormcd
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
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
Member
krakjoe commented Jan 14, 2014

Yeah, wat @laruence said ...

@conormcd
Contributor

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

@conormcd
Contributor

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

@conormcd conormcd Bug #66481 Segfaults on session_name()
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().
e7dca51
@conormcd
Contributor

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

@yohgaki
Contributor
yohgaki commented Jan 16, 2014

Newer patch(?) looks ok to me.

@yohgaki
Contributor
yohgaki commented Jan 16, 2014

Merged. Thank you.

@conormcd
Contributor

Excellent! Thanks. :)

@laruence
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
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