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

Made session save handler abuse much harder than before.

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

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;

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);
 }
 /* }}} */
 

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;
 	}
 }
 /* }}} */

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

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
 

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

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

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
+

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

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!