php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #63379 Warning when using session_regenerate_id(TRUE) with a SessionHandler
Submitted: 2012-10-29 01:04 UTC Modified: 2012-11-25 13:26 UTC
Votes:13
Avg. Score:4.7 ± 0.6
Reproduced:11 of 11 (100.0%)
Same Version:10 (90.9%)
Same OS:0 (0.0%)
From: avatar2004-php at yahoo dot fr Assigned: arpad (profile)
Status: Closed Package: Session related
PHP Version: 5.4.8 OS: Gentoo
Private report: No CVE-ID: None
 [2012-10-29 01:04 UTC] avatar2004-php at yahoo dot fr
Description:
------------
It seems there's an issue with the SessionHandler implementation and
the way the destroy handler is invoked when using
session_regenerate_id(TRUE)

The problem seems to come from using the global "mod_user_is_open" in
the PS_SANITY_CHECK_IS_OPEN macro (in ext/session/mod_user_class.c).

The test script generates an undue warning:

Test script:
---------------
$handler = new SessionHandler();
session_set_save_handler($handler, TRUE); // or FALSE, doesn't matter

session_start();
session_regenerate_id(TRUE);

//session_write_close();

Expected result:
----------------
No warning

Actual result:
--------------
PHP Warning:  Unknown: Parent session handler is not open in Unknown on line 0

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-10-29 01:14 UTC] avatar2004-php at yahoo dot fr
If I understand correctly and "mod_user_is_open" is just a global state parameter used by the SessionHandler instance, I was wondering why the check wasn't being done on "session_status" instead to conform with the rest of the session_* API ?
 [2012-10-29 02:00 UTC] laruence@php.net
-Assigned To: +Assigned To: arpad
 [2012-10-29 03:07 UTC] laruence@php.net
is the reseting of user_is_open necessary?

diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c
index 70d2f40..4edac28 100644
--- a/ext/session/mod_user_class.c
+++ b/ext/session/mod_user_class.c
@@ -121,7 +121,6 @@ PHP_METHOD(SessionHandler, destroy)
 		return;
 	}
 	
-	PS(mod_user_is_open) = 0;
 	RETVAL_BOOL(SUCCESS == PS(default_mod)->s_destroy(&PS(mod_data), key 
TSRMLS_CC));
 }
 /* }}} */
 [2012-10-29 03:07 UTC] laruence@php.net
I mean, maybe only reset it in close handler?
 [2012-10-29 04:21 UTC] avatar2004-php at yahoo dot fr
Looking at the code, it feels like the very semantics of the mod_user_is_open flag are not exactly consistent.

The flag is a global, yet it is manipulated by instance code. Meaning if the user space code uses several handlers interchangeably, the result can quickly become confusing.

I suggest a decision should be made as to whether the SessionHandler is a stateless, thin wrapper for the handler calls in which case it should default to the same checks as the procedural API. Otherwise, the flag should really be an instance variable used to track the proper invocation sequence of the different callbacks and make sure THIS handler is open before calling dependent routines.

Adding an instance variable to the (base) class though is probably not worth it. The session management semantics are already defined by the procedural API which uses a global session state flag (session_status). If required for a handler implementation, the flag can simply be implemented in user space.

Just my 2c.
 [2012-12-14 00:07 UTC] arpad@php.net
Automatic comment on behalf of arraypad@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=6566ea61732a1ab42c1a57e60adc96788cb0feb2
Log: Fix #63379 - Don't reset mod_user_is_open in destroy
 [2012-12-14 00:07 UTC] arpad@php.net
-Status: Assigned +Status: Closed
 [2012-12-14 00:25 UTC] arpad@php.net
Laruence you were correct, the reset wasn't necessary :)

avatar2004, the flag is only meant to keep the parent session handler in a sane state. If a user implementation calls the parent handler in close() but not in open(), we need to be able to prevent the close() call on the parent handler even though session_status tells us that the session is open.

The flag is a global because having more than one session handler instance active in the same request is nonsensical.
 [2012-12-19 17:54 UTC] derick@php.net
Automatic comment on behalf of arraypad@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=6566ea61732a1ab42c1a57e60adc96788cb0feb2
Log: Fix #63379 - Don't reset mod_user_is_open in destroy
 [2014-10-07 23:20 UTC] stas@php.net
Automatic comment on behalf of arraypad@gmail.com
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=6566ea61732a1ab42c1a57e60adc96788cb0feb2
Log: Fix #63379 - Don't reset mod_user_is_open in destroy
 [2014-10-07 23:31 UTC] stas@php.net
Automatic comment on behalf of arraypad@gmail.com
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=6566ea61732a1ab42c1a57e60adc96788cb0feb2
Log: Fix #63379 - Don't reset mod_user_is_open in destroy
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Sat Jul 31 04:01:24 2021 UTC