MDL-53713 core: account for session_start returning false

mvaraujo · May 6, 2016 · 1ac585f · 1ac585f
1 parent 3032b16
commit 1ac585f
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 8 deletions.
diff --git a/lib/classes/session/manager.php b/lib/classes/session/manager.php
@@ -72,11 +72,14 @@ public static function start() {
         try {
             self::$handler->init();
             self::prepare_cookies();
-            $newsid = empty($_COOKIE[session_name()]);
+            $isnewsession = empty($_COOKIE[session_name()]);
 
-            self::$handler->start();
+            if (!self::$handler->start()) {
+                // Could not successfully start/recover session.
+                throw new \core\session\exception(get_string('servererror'));
+            }
 
-            self::initialise_user_session($newsid);
+            self::initialise_user_session($isnewsession);
             self::check_security();
 
             // Link global $USER and $SESSION,
@@ -90,7 +93,6 @@ public static function start() {
             $_SESSION['SESSION'] =& $GLOBALS['SESSION'];
 
         } catch (\Exception $ex) {
-            @session_write_close();
             self::init_empty_session();
             self::$sessionactive = false;
             throw $ex;
@@ -438,6 +440,7 @@ protected static function add_session_record($userid) {
      * Do various session security checks.
      *
      * WARNING: $USER and $SESSION are set up later, do not use them yet!
+     * @throws \core\session\exception
      */
     protected static function check_security() {
         global $CFG;
@@ -521,11 +524,23 @@ public static function terminate_current() {
      * Unblocks the sessions, other scripts may start executing in parallel.
      */
     public static function write_close() {
-        if (self::$sessionactive) {
-            session_write_close();
+        if (version_compare(PHP_VERSION, '5.6.0', '>=')) {
+            // More control over whether session data
+            // is persisted or not.
+            if (self::$sessionactive && session_id()) {
+                // Write session and release lock only if
+                // indication session start was clean.
+                session_write_close();
+            } else {
+                // Otherwise, if possibile lock exists want
+                // to clear it, but do not write session.
+                @session_abort();
+            }
         } else {
-            if (session_id()) {
-                @session_write_close();
+            // Any indication session was started, attempt
+            // to close it.
+            if (self::$sessionactive || session_id()) {
+                session_write_close();
             }
         }
         self::$sessionactive = false;

diff --git a/lib/classes/session/memcached.php b/lib/classes/session/memcached.php
@@ -93,8 +93,24 @@ public function start() {
         $default = ini_get('max_execution_time');
         set_time_limit($this->acquiretimeout);
 
+        $isnewsession = empty($_COOKIE[session_name()]);
+        $starttimer = microtime(true);
+
         $result = parent::start();
 
+        // If session_start returned TRUE, but it took as long
+        // as the timeout value, and the $_SESSION returned is
+        // empty when should not have been (isnewsession false)
+        // then assume it did timeout and is invalid.
+        // Add 1 second to elapsed time to account for inexact
+        // timings in php_memcached_session.c.
+        // @TODO Remove this check when php-memcached is fixed
+        // to return false after key lock acquisition timeout.
+        if (!$isnewsession && $result && count($_SESSION) == 0
+            && (microtime(true) - $starttimer + 1) >= floatval($this->acquiretimeout)) {
+            $result = false;
+        }
+
         set_time_limit($default);
         return $result;
     }