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

calling set_userdata() or unset_userdata() after session_destroy() throws PHP Warnings #2752

Closed
el-timm opened this Issue Dec 4, 2013 · 11 comments

Comments

Projects
None yet
4 participants
@el-timm
Copy link

el-timm commented Dec 4, 2013

CI 2.1.4, PHP 5.3.27

If code is trying to set/unset session userdata after the session has been destroyed it causes PHP Warnings.

(While the above behavior is not proper nor recommended, it may be performed because there is not much opportunity to check if the session, and session key, exist.)

The CodeIgniter framework should at least be defensive and perform checks as to avoid throwing these PHP warnings when trying to access the reserved session keys (ip_address, last_activity, etc)

E_NOTICE Undefined index: session_id in sess_write (Session.php:272)
E_NOTICE Undefined index: ip_address in sess_write (Session.php:272)
E_NOTICE Undefined index: user_agent in sess_write (Session.php:272)
E_NOTICE Undefined index: last_activity in sess_write (Session.php:272)
E_NOTICE Undefined index: session_id in sess_write (Session.php:288)
E_NOTICE Undefined index: last_activity in sess_write (Session.php:289)

To recreate:

error_reporting(E_ALL);
ini_set('display_errors', '1');

$this->load->library('session');

$this->session->sess_destroy();

// E_NOTICE Undefined index: ip_address in sess_write (Session.php:272)
// ...
// E_NOTICE Undefined index: last_activity in sess_write (Session.php:289)
$this->session->set_userdata('foo', 'bar');

// E_NOTICE Undefined index: ip_address in sess_write (Session.php:272)
// ...
// E_NOTICE Undefined index: last_activity in sess_write (Session.php:289)
$this->session->unset_userdata('foo');
@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 15, 2014

Is this not fixed in develop?

@el-timm

This comment has been minimized.

Copy link
Author

el-timm commented Jan 15, 2014

Sorry, I don't quite understand your question.

Regardless, this is a minor issue, but is still reproducible with 2.1.4.

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 15, 2014

'develop' is the branch here that has the code for 3.0-dev.

@el-timm

This comment has been minimized.

Copy link
Author

el-timm commented Jan 15, 2014

Unfortunately, it takes us a non-trivial effort to pull and a CI base and migrate our base to work with it. I would simply copy over the Session.php file but the driver implementation makes me think that wouldn't work. If you have other ideas on how to backport the 3.0 Session.php file to 2.1.4 I'd be happy to test it for you.

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 15, 2014

I'm not suggesting that you backport it, just asked if anybody can confirm that this is fixed in 3.0-dev.

@el-timm

This comment has been minimized.

Copy link
Author

el-timm commented Jan 15, 2014

Gotcha. A brief look makes me think that no, it is not fixed.

in 2.1.4, the following lines are not defensive (they assume $val and 'session_id' are existing keys in $this->userdata), so when the session is destroyed (and userdata[] emptied), and these lines are executed they throw the warnings listed above

// CI 2.1.4 Session.php
// line 272
$cookie_userdata[$val] = $this->userdata[$val];
// line 288
$this->CI->db->where('session_id', $this->userdata['session_id']);

While not an apple:apple comparison, the 3.0-dev branch appears to be non-defensive in the same regard, some examples:

// CI 3.0-dev Session_cookie.php 4ea76cc221
// line 543
if ( ! $force && ($this->userdata['last_activity'] + $this->sess_time_to_update) >= $this->now)
// line 552
$old_sessid = $this->userdata['session_id'];
// line 582
'session_id' => $this->userdata['session_id']
// line 612
'last_activity' => $this->userdata['last_activity'],
// line 633
$this->CI->db->where('session_id', $this->userdata['session_id']);
@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 15, 2014

These lines don't say anything, I went through the code myself and don't see a reason for the notices to appear ... hence why I'm asking for confirmation from somebody who uses it.
Don't get me wrong, but us two chatting about it won't resolve the issue, nor it will find the problem. Let's just wait for somebody else who uses cookie Sessions to comment.

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 15, 2014

Related: #2182

@daparky Could you confirm if the issue is still present or if it is fixed?

@daparky

This comment has been minimized.

Copy link
Contributor

daparky commented Jan 20, 2014

I will test it tomorrow night and get back to you.

@mpmont

This comment has been minimized.

Copy link
Contributor

mpmont commented Jan 22, 2014

I just tested this on develop branch. This does not happen in CI 3.0

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 22, 2014

Great! Closing it then.
Thanks @mpmont.

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.