Fixed bug #69111 (Crash in SessionHandler::read()).

Made session save handler abuse much harder than before.

diff --git a/NEWS b/NEWS
index b5771a4f695d3f49f81b72caa16963f30de9132a..e02cde1edd0105ee0dd57b9159dcf91762f03b96 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -23,6 +23,7 @@ PHP                                                                        NEWS
 
 - Session:
   . Improved fix for bug #68063 (Empty session IDs do still start sessions). (Yasuo)
+  . Fixed bug #69111 (Crash in SessionHandler::read()). (Yasuo)
   . Fixed bug #71122 (Session GC may not remove obsolete session data). (Yasuo)
   . Fixed bug #71038 (session_start() returns TRUE on failure).
     It's fixed partially on PHP 5.6. It still returns TRUE on session read

diff --git a/ext/session/mod_user.c b/ext/session/mod_user.c
index 0b6fb626fd74df519ddb6d8f4834701b53a0cf88..de2df9d6a7f73978213012eb1ec2b8f972b94670 100644 (file)
--- a/ext/session/mod_user.c
+++ b/ext/session/mod_user.c
@@ -91,7 +91,16 @@ PS_OPEN_FUNC(user)
        SESS_ZVAL_STRING((char*)save_path, args[0]);
        SESS_ZVAL_STRING((char*)session_name, args[1]);
 
-       retval = ps_call_handler(PSF(open), 2, args TSRMLS_CC);
+       zend_try {
+               retval = ps_call_handler(PSF(open), 2, args TSRMLS_CC);
+       } zend_catch {
+               PS(session_status) = php_session_none;
+               if (retval) {
+                       zval_ptr_dtor(&retval);
+               }
+               zend_bailout();
+       } zend_end_try();
+
        PS(mod_user_implemented) = 1;
 
        FINISH;

diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c
index 392f09eda10982960db7f242a44884c2757b9218..6768bec867e42bc3bd4de7cd5d41fc0b45635b07 100644 (file)
--- a/ext/session/mod_user_class.c
+++ b/ext/session/mod_user_class.c
@@ -22,6 +22,10 @@
 #include "php_session.h"
 
 #define PS_SANITY_CHECK                                                \
+       if (PS(session_status) != php_session_active) { \
+               php_error_docref(NULL TSRMLS_CC, E_WARNING, "Session is not active"); \
+               RETURN_FALSE; \
+       } \
        if (PS(default_mod) == NULL) {                          \
                php_error_docref(NULL TSRMLS_CC, E_CORE_ERROR, "Cannot call default session handler"); \
                RETURN_FALSE;                                           \
@@ -39,7 +43,7 @@
 PHP_METHOD(SessionHandler, open)
 {
        char *save_path = NULL, *session_name = NULL;
-       int save_path_len, session_name_len;
+       int ret, save_path_len, session_name_len;
 
        PS_SANITY_CHECK;
 
@@ -48,7 +52,14 @@ PHP_METHOD(SessionHandler, open)
        }
 
        PS(mod_user_is_open) = 1;
-       RETVAL_BOOL(SUCCESS == PS(default_mod)->s_open(&PS(mod_data), save_path, session_name TSRMLS_CC));
+       zend_try {
+               ret =PS(default_mod)->s_open(&PS(mod_data), save_path, session_name TSRMLS_CC);
+       } zend_catch {
+               PS(session_status) = php_session_none;
+               zend_bailout();
+       } zend_end_try();
+
+       RETVAL_BOOL(SUCCESS == ret);
 }
 /* }}} */
 
@@ -56,6 +67,8 @@ PHP_METHOD(SessionHandler, open)
    Wraps the old close handler */
 PHP_METHOD(SessionHandler, close)
 {
+       int ret;
+
        PS_SANITY_CHECK_IS_OPEN;
 
        // don't return on failure, since not closing the default handler
@@ -63,7 +76,15 @@ PHP_METHOD(SessionHandler, close)
        zend_parse_parameters_none();
        
        PS(mod_user_is_open) = 0;
-       RETVAL_BOOL(SUCCESS == PS(default_mod)->s_close(&PS(mod_data) TSRMLS_CC));
+
+       zend_try {
+               ret = PS(default_mod)->s_close(&PS(mod_data) TSRMLS_CC);
+       } zend_catch {
+               PS(session_status) = php_session_none;
+               zend_bailout();
+       } zend_end_try();
+
+       RETVAL_BOOL(SUCCESS == ret);
 }
 /* }}} */
 

diff --git a/ext/session/session.c b/ext/session/session.c
index 21053a574dcc6c013f733f9e9827f2f36dc86d69..cbcd20d63abc55f7cd08a037e748f14f5b3c5309 100644 (file)
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -92,12 +92,14 @@ static void php_session_abort(TSRMLS_D);
 /* Dispatched by RINIT and by php_session_destroy */
 static inline void php_rinit_session_globals(TSRMLS_D) /* {{{ */
 {
+       /* Do NOT init PS(mod_user_names) here! */
+
+       /* TODO: These could be moved to MINIT and removed. These should be initialized by php_rshutdown_session_globals() always when execution is finished. */
        PS(id) = NULL;
        PS(session_status) = php_session_none;
        PS(mod_data) = NULL;
        PS(mod_user_is_open) = 0;
        PS(define_sid) = 1;
-       /* Do NOT init PS(mod_user_names) here! */
        PS(http_session_vars) = NULL;
 }
 /* }}} */
@@ -119,6 +121,9 @@ static inline void php_rshutdown_session_globals(TSRMLS_D) /* {{{ */
                efree(PS(id));
                PS(id) = NULL;
        }
+       /* User save handlers may end up directly here by misuse, bugs in user script, etc. */
+       /* Set session status to prevent error while restoring save handler INI value. */
+       PS(session_status) = php_session_none;
 }
 /* }}} */
 
@@ -1651,8 +1656,8 @@ PHPAPI void php_session_start(TSRMLS_D) /* {{{ */
 static void php_session_flush(TSRMLS_D) /* {{{ */
 {
        if (PS(session_status) == php_session_active) {
-               PS(session_status) = php_session_none;
                php_session_save_current_state(TSRMLS_C);
+               PS(session_status) = php_session_none;
        }
 }
 /* }}} */
@@ -1660,10 +1665,10 @@ static void php_session_flush(TSRMLS_D) /* {{{ */
 static void php_session_abort(TSRMLS_D) /* {{{ */
 {
        if (PS(session_status) == php_session_active) {
-               PS(session_status) = php_session_none;
                if (PS(mod_data) || PS(mod_user_implemented)) {
                        PS(mod)->s_close(&PS(mod_data) TSRMLS_CC);
                }
+               PS(session_status) = php_session_none;
        }
 }
 /* }}} */

diff --git a/ext/session/tests/bug55688.phpt b/ext/session/tests/bug55688.phpt
index 8db48384af0c0b7defa110ce99cea95d96630486..b073dc3c5c64d8aa8f50d60e2ca2083e78562a75 100644 (file)
--- a/ext/session/tests/bug55688.phpt
+++ b/ext/session/tests/bug55688.phpt
@@ -12,4 +12,4 @@ $x = new SessionHandler;
 $x->gc(1);
 ?>
 --EXPECTF--
-Warning: SessionHandler::gc(): Parent session handler is not open in %s on line %d
+Warning: SessionHandler::gc(): Session is not active in %s on line %d

diff --git a/ext/session/tests/bug60634.phpt b/ext/session/tests/bug60634.phpt
index 86dcb115268c410afc3c8a401c0bc60b22a1acc2..b2f507628729f5364e69dd443961baefac43d930 100644 (file)
--- a/ext/session/tests/bug60634.phpt
+++ b/ext/session/tests/bug60634.phpt
@@ -39,8 +39,17 @@ session_start();
 session_write_close();
 echo "um, hi\n";
 
+/*
+FIXME: Since session module try to write/close session data in
+RSHUTDOWN, write() is executed twices. This is caused by undefined
+function error and zend_bailout(). Current session module codes
+depends on this behavior. These codes should be modified to remove
+multiple write().
+*/
+
 ?>
 --EXPECTF--
 write: goodbye cruel world
+write: goodbye cruel world
 close: goodbye cruel world
 

diff --git a/ext/session/tests/bug60634_error_1.phpt b/ext/session/tests/bug60634_error_1.phpt
index e41592f18dd095ef1a5a08890724208291a536ca..7388ba0123ea8c351dd76cf4c49cf7dabedaf37e 100644 (file)
--- a/ext/session/tests/bug60634_error_1.phpt
+++ b/ext/session/tests/bug60634_error_1.phpt
@@ -40,9 +40,20 @@ session_start();
 session_write_close();
 echo "um, hi\n";
 
+/*
+FIXME: Since session module try to write/close session data in
+RSHUTDOWN, write() is executed twices. This is caused by undefined
+function error and zend_bailout(). Current session module codes
+depends on this behavior. These codes should be modified to remove
+multiple write().
+*/
+
 ?>
 --EXPECTF--
 write: goodbye cruel world
 
+Fatal error: Call to undefined function undefined_function() in %s on line %d
+write: goodbye cruel world
+
 Fatal error: Call to undefined function undefined_function() in %s on line %d
 close: goodbye cruel world

diff --git a/ext/session/tests/bug60634_error_5.phpt b/ext/session/tests/bug60634_error_5.phpt
index 8081ab988a3397254af661ffdc6284588a9d4b20..07d7b3a514742ebe6f25f4e0c691e732c1438f13 100644 (file)
--- a/ext/session/tests/bug60634_error_5.phpt
+++ b/ext/session/tests/bug60634_error_5.phpt
@@ -45,3 +45,5 @@ echo "um, hi\n";
 close: goodbye cruel world
 
 Fatal error: Call to undefined function undefined_function() in %s on line %d
+
+Warning: Unknown: Failed to write session data (user). Please verify that the current setting of session.save_path is correct () in Unknown on line 0

diff --git a/ext/session/tests/bug67972.phpt b/ext/session/tests/bug67972.phpt
index 63ed3a95b8978df9a8daf24a7b9cea184019d03d..92c3044ac599bf7df024001229266ec76b77694d 100644 (file)
--- a/ext/session/tests/bug67972.phpt
+++ b/ext/session/tests/bug67972.phpt
@@ -7,4 +7,5 @@ Bug #67972: SessionHandler Invalid memory read create_sid()
 
 (new SessionHandler)->create_sid();
 --EXPECTF--
-Fatal error: SessionHandler::create_sid(): Cannot call default session handler in %s on line %d
+Warning: SessionHandler::create_sid(): Session is not active in %s on line %d
+

diff --git a/ext/session/tests/bug69111.phpt b/ext/session/tests/bug69111.phpt
new file mode 100644 (file)
index 0000000..f5def0e
--- /dev/null
+++ b/ext/session/tests/bug69111.phpt
@@ -0,0 +1,36 @@
+--TEST--
+Bug #69111 (Crash in SessionHandler::read())
+--INI--
+session.save_path=
+session.save_handler=files
+session.name=PHPSESSID
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+$sh = new SessionHandler;
+session_set_save_handler($sh);
+
+$savePath = ini_get('session.save_path');
+$sessionName = ini_get('session.name');
+
+// session_start(); // Uncommenting this makes it not crash when reading the session (see below), but it will not return any data.
+
+$sh->open($savePath, $sessionName);
+$sh->write("foo", "bar");
+$sh->read($id);
+$sh->gc(1245);
+$sh->close();
+?>
+--EXPECTF--
+Warning: SessionHandler::open(): Session is not active in %s on line 10
+
+Warning: SessionHandler::write(): Session is not active in %s on line 11
+
+Notice: Undefined variable: id in %s on line 12
+
+Warning: SessionHandler::read(): Session is not active in %s on line 12
+
+Warning: SessionHandler::gc(): Session is not active in %s on line 13
+
+Warning: SessionHandler::close(): Session is not active in %s on line 14
\ No newline at end of file

diff --git a/ext/session/tests/sessionhandler_open_001.phpt b/ext/session/tests/sessionhandler_open_001.phpt
index 6ade9e00a598e973f630d83288cb7fd155511cda..e6e913a6a5735db97e66743b119e80c5df432561 100644 (file)
--- a/ext/session/tests/sessionhandler_open_001.phpt
+++ b/ext/session/tests/sessionhandler_open_001.phpt
@@ -16,4 +16,11 @@ print "Done!\n";
 
 ?>
 --EXPECTF--
+Warning: SessionHandler::open(): Session is not active in %s on line 5
+
+Warning: SessionHandler::open(): Session is not active in %s on line 6
+
+Warning: SessionHandler::open(): Session is not active in %s on line 7
+
+Warning: SessionHandler::open(): Session is not active in %s on line 8
 Done!