Add methods to append certain nodes to session data in order to avoid session saving race conditions. Fixes #1488422

2 files changed, 61 insertions, 13 deletions

diff --git a/program/lib/Roundcube/rcube_session.php b/program/lib/Roundcube/rcube_session.php
index 1aa5d5856..82ff8a804 100644
--- a/program/lib/Roundcube/rcube_session.php
+++ b/program/lib/Roundcube/rcube_session.php
@@ -32,6 +32,7 @@ class rcube_session
     private $ip;
     private $start;
     private $changed;
+    private $reloaded = false;
     private $unsets = array();
     private $gc_handlers = array();
     private $cookiename = 'roundcube_sessauth';
@@ -200,8 +201,13 @@ class rcube_session
         if ($oldvars !== null) {
             $a_oldvars = $this->unserialize($oldvars);
             if (is_array($a_oldvars)) {
-                foreach ((array)$this->unsets as $k)
-                    unset($a_oldvars[$k]);
+                // remove unset keys on oldvars
+                foreach ((array)$this->unsets as $var) {
+                    $path = explode('.', $var);
+                    $k = array_pop($path);
+                    $node = &$this->get_node($path, $a_oldvars);
+                    unset($node[$k]);
+                }
 
                 $newvars = $this->serialize(array_merge(
                     (array)$a_oldvars, (array)$this->unserialize($vars)));
@@ -371,9 +377,32 @@ class rcube_session
 
 
     /**
+     * Append the given value to the certain node in the session data array
+     *
+     * @param string Path denoting the session variable where to append the value
+     * @param string Key name under which to append the new value (use null for appending to an indexed list)
+     * @param mixed  Value to append to the session data array
+     */
+    public function append($path, $key, $value)
+    {
+        // re-read session data from DB because it might be outdated
+        if (!$this->reloaded && microtime(true) - $this->start > 0.5) {
+            $this->reload();
+            $this->reloaded = true;
+            $this->start = microtime(true);
+        }
+
+        $node = &$this->get_node(explode('.', $path), $_SESSION);
+
+        if ($key !== null) $node[$key] = $value;
+        else               $node[] = $value;
+    }
+
+
+    /**
      * Unset a session variable
      *
-     * @param string Varibale name
+     * @param string Varibale name (can be a path denoting a certain node in the session array, e.g. compose.attachments.5)
      * @return boolean True on success
      */
     public function remove($var=null)
@@ -383,7 +412,11 @@ class rcube_session
         }
 
         $this->unsets[] = $var;
-        unset($_SESSION[$var]);
+
+        $path = explode('.', $var);
+        $key = array_pop($path);
+        $node = &$this->get_node($path, $_SESSION);
+        unset($node[$key]);
 
         return true;
     }
@@ -415,6 +448,23 @@ class rcube_session
             session_decode($data);
     }
 
+    /**
+     * Returns a reference to the node in data array referenced by the given path.
+     * e.g. ['compose','attachments'] will return $_SESSION['compose']['attachments']
+     */
+    private function &get_node($path, &$data_arr)
+    {
+        $node = &$data_arr;
+        if (!empty($path)) {
+            foreach ((array)$path as $key) {
+                if (!isset($node[$key]))
+                    $node[$key] = array();
+                $node = &$node[$key];
+            }
+        }
+
+        return $node;
+    }
 
     /**
      * Serialize session data
diff --git a/program/steps/mail/attachments.inc b/program/steps/mail/attachments.inc
index 180fc0bb9..f83f6892e 100644
--- a/program/steps/mail/attachments.inc
+++ b/program/steps/mail/attachments.inc
@@ -27,8 +27,10 @@ if (!empty($_GET['_progress'])) {
 $COMPOSE_ID = get_input_value('_id', RCUBE_INPUT_GPC);
 $COMPOSE    = null;
 
-if ($COMPOSE_ID && $_SESSION['compose_data_'.$COMPOSE_ID])
-  $COMPOSE =& $_SESSION['compose_data_'.$COMPOSE_ID];
+if ($COMPOSE_ID && $_SESSION['compose_data_' . $COMPOSE_ID]) {
+  $SESSION_KEY = 'compose_data_' . $COMPOSE_ID;
+  $COMPOSE =& $_SESSION[$SESSION_KEY];
+}
 
 if (!$COMPOSE) {
   die("Invalid session var!");
@@ -45,7 +47,7 @@ if ($RCMAIL->action=='remove-attachment')
     $attachment = $RCMAIL->plugins->exec_hook('attachment_delete', $attachment);
   if ($attachment['status']) {
     if (is_array($COMPOSE['attachments'][$id])) {
-      unset($COMPOSE['attachments'][$id]);
+      $RCMAIL->session->remove($SESSION_KEY.'.attachments.'.$id);
       $OUTPUT->command('remove_from_attachment_list', "rcmfile$id");
     }
   }
@@ -77,11 +79,7 @@ if ($RCMAIL->action=='display-attachment')
   exit;
 }
 
-// attachment upload action
-
-if (!is_array($COMPOSE['attachments'])) {
-  $COMPOSE['attachments'] = array();
-}
+/*****  attachment upload action  *****/
 
 // clear all stored output properties (like scripts and env vars)
 $OUTPUT->reset();
@@ -112,7 +110,7 @@ if (is_array($_FILES['_attachments']['tmp_name'])) {
 
       // store new attachment in session
       unset($attachment['status'], $attachment['abort']);
-      $COMPOSE['attachments'][$id] = $attachment;
+      $RCMAIL->session->append($SESSION_KEY.'.attachments', $id, $attachment);
 
       if (($icon = $COMPOSE['deleteicon']) && is_file($icon)) {
         $button = html::img(array(