From 7dad54ca085e3ca43b85270bacb8449cf9f4b9e1 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sat, 17 May 2014 20:23:24 +0200 Subject: [PATCH 1/8] [HttpFoundation] remove base64 encoding of session data --- .../Storage/Handler/PdoSessionHandler.php | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index d6f3704e961b8..a0cc434c061ca 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -19,14 +19,15 @@ * This means requests for the same session will wait until the other one finished. * PHPs internal files session handler also works this way. * - * Session data is a binary string that can contain non-printable characters like the null byte. - * For this reason this handler base64 encodes the data to be able to save it in a character column. - * * Attention: Since SQLite does not support row level locks but locks the whole database, * it means only one session can be accessed at a time. Even different sessions would wait * for another to finish. So saving session in SQLite should only be considered for * development or prototypes. * + * Session data is a binary string that can contain non-printable characters like the null byte. + * For this reason it must be saved in a binary column in the database like BLOB in MySQL. + * Saving it in a character column could corrupt the data. + * * @see http://php.net/sessionhandlerinterface * * @author Fabien Potencier @@ -145,11 +146,7 @@ public function read($sessionId) // We use fetchAll instead of fetchColumn to make sure the DB cursor gets closed $sessionRows = $stmt->fetchAll(\PDO::FETCH_NUM); - if ($sessionRows) { - return base64_decode($sessionRows[0][0]); - } - - return ''; + return $sessionRows ? $sessionRows[0][0] : ''; } catch (\PDOException $e) { $this->rollback(); @@ -195,8 +192,6 @@ public function destroy($sessionId) */ public function write($sessionId, $data) { - $encoded = base64_encode($data); - // The session ID can be different from the one previously received in read() // when the session ID changed due to session_regenerate_id(). So we have to // do an insert or update even if we created a row in read() for locking. @@ -208,7 +203,7 @@ public function write($sessionId, $data) if (null !== $mergeSql) { $mergeStmt = $this->pdo->prepare($mergeSql); $mergeStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $mergeStmt->bindParam(':data', $encoded, \PDO::PARAM_STR); + $mergeStmt->bindParam(':data', $data, \PDO::PARAM_LOB); $mergeStmt->bindValue(':time', time(), \PDO::PARAM_INT); $mergeStmt->execute(); @@ -219,7 +214,7 @@ public function write($sessionId, $data) "UPDATE $this->table SET $this->dataCol = :data, $this->timeCol = :time WHERE $this->idCol = :id" ); $updateStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $updateStmt->bindParam(':data', $encoded, \PDO::PARAM_STR); + $updateStmt->bindParam(':data', $data, \PDO::PARAM_LOB); $updateStmt->bindValue(':time', time(), \PDO::PARAM_INT); $updateStmt->execute(); @@ -236,7 +231,7 @@ public function write($sessionId, $data) "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)" ); $insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $insertStmt->bindParam(':data', $encoded, \PDO::PARAM_STR); + $insertStmt->bindParam(':data', $encoded, \PDO::PARAM_LOB); $insertStmt->bindValue(':time', time(), \PDO::PARAM_INT); $insertStmt->execute(); } catch (\PDOException $e) { From 251238d9a631a974b26855848ec9b5266c7b4626 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 18 May 2014 03:41:33 +0200 Subject: [PATCH 2/8] [HttpFoundation] implement lazy connect for pdo session handler --- .../Storage/Handler/PdoSessionHandler.php | 72 ++++++++++++++---- .../Storage/Handler/PdoSessionHandlerTest.php | 75 +++++++++++++++++-- 2 files changed, 127 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index a0cc434c061ca..9d5fa8f4850ab 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -37,10 +37,15 @@ class PdoSessionHandler implements \SessionHandlerInterface { /** - * @var \PDO PDO instance + * @var \PDO|null PDO instance or null when not connected yet */ private $pdo; + /** + * @var string|null|false DNS string or null for session.save_path or false when lazy connection disabled + */ + private $dns = false; + /** * @var string Database driver */ @@ -66,6 +71,21 @@ class PdoSessionHandler implements \SessionHandlerInterface */ private $timeCol; + /** + * @var string Username when lazy-connect + */ + private $username; + + /** + * @var string Password when lazy-connect + */ + private $password; + + /** + * @var array Connection options when lazy-connect + */ + private $connectionOptions = array(); + /** * @var bool Whether a transaction is active */ @@ -79,37 +99,54 @@ class PdoSessionHandler implements \SessionHandlerInterface /** * Constructor. * + * You can either pass an existing database connection as PDO instance or + * pass a DNS string that will be used to lazy-connect to the database + * when the session is actually used. Furthermore it's possible to pass null + * which will then use the session.save_path ini setting as PDO DNS parameter. + * * List of available options: * * db_table: The name of the table [default: sessions] * * db_id_col: The column where to store the session id [default: sess_id] * * db_data_col: The column where to store the session data [default: sess_data] * * db_time_col: The column where to store the timestamp [default: sess_time] + * * db_username: The username when lazy-connect [default: ''] + * * db_password: The password when lazy-connect [default: ''] + * * db_connection_options: An array of driver-specific connection options [default: array()] * - * @param \PDO $pdo A \PDO instance - * @param array $options An associative array of DB options + * @param \PDO|string|null $pdoOrDns A \PDO instance or DNS string or null + * @param array $options An associative array of DB options * * @throws \InvalidArgumentException When PDO error mode is not PDO::ERRMODE_EXCEPTION */ - public function __construct(\PDO $pdo, array $options = array()) + public function __construct($pdoOrDns, array $options = array()) { - if (\PDO::ERRMODE_EXCEPTION !== $pdo->getAttribute(\PDO::ATTR_ERRMODE)) { - throw new \InvalidArgumentException(sprintf('"%s" requires PDO error mode attribute be set to throw Exceptions (i.e. $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION))', __CLASS__)); - } + if ($pdoOrDns instanceof \PDO) { + if (\PDO::ERRMODE_EXCEPTION !== $pdoOrDns->getAttribute(\PDO::ATTR_ERRMODE)) { + throw new \InvalidArgumentException(sprintf('"%s" requires PDO error mode attribute be set to throw Exceptions (i.e. $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION))', __CLASS__)); + } - $this->pdo = $pdo; - $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); + $this->pdo = $pdoOrDns; + } else { + $this->dns = $pdoOrDns; + } $options = array_replace(array( - 'db_table' => 'sessions', - 'db_id_col' => 'sess_id', - 'db_data_col' => 'sess_data', - 'db_time_col' => 'sess_time', + 'db_table' => 'sessions', + 'db_id_col' => 'sess_id', + 'db_data_col' => 'sess_data', + 'db_time_col' => 'sess_time', + 'db_username' => '', + 'db_password' => '', + 'db_connection_options' => array() ), $options); $this->table = $options['db_table']; $this->idCol = $options['db_id_col']; $this->dataCol = $options['db_data_col']; $this->timeCol = $options['db_time_col']; + $this->username = $options['db_username']; + $this->password = $options['db_password']; + $this->connectionOptions = $options['db_connection_options']; } /** @@ -118,6 +155,11 @@ public function __construct(\PDO $pdo, array $options = array()) public function open($savePath, $sessionName) { $this->gcCalled = false; + if (null === $this->pdo) { + $this->pdo = new \PDO($this->dns ?: $savePath, $this->username, $this->password, $this->connectionOptions); + $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + } + $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); return true; } @@ -270,6 +312,10 @@ public function close() $stmt->execute(); } + if (false !== $this->dns) { + $this->pdo = null; + } + return true; } diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 41b2e753c510b..3356e3dcea34a 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -25,7 +25,7 @@ protected function setUp() $this->pdo = new \PDO('sqlite::memory:'); $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data TEXT, sess_time INTEGER)'; + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; $this->pdo->exec($sql); } @@ -51,17 +51,74 @@ public function testInexistentTable() $storage->close(); } + public function testWithLazyDnsConnection() + { + $dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); + if (file_exists($dbFile)) { + @unlink($dbFile); + } + + $pdo = new \PDO('sqlite:' . $dbFile); + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(255) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; + $pdo->exec($sql); + $pdo = null; + + $storage = new PdoSessionHandler('sqlite:' . $dbFile); + $storage->open('', 'sid'); + $data = $storage->read('id'); + $storage->write('id', 'data'); + $storage->close(); + $this->assertSame('', $data, 'New session returns empty string data'); + + $storage->open('', 'sid'); + $data = $storage->read('id'); + $storage->close(); + $this->assertSame('data', $data, 'Written value can be read back correctly'); + + @unlink($dbFile); + } + + public function testWithLazySavePathConnection() + { + $dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); + if (file_exists($dbFile)) { + @unlink($dbFile); + } + + $pdo = new \PDO('sqlite:' . $dbFile); + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(255) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; + $pdo->exec($sql); + $pdo = null; + + // Open is called with what ini_set('session.save_path', 'sqlite:' . $dbFile) would mean + $storage = new PdoSessionHandler(null); + $storage->open('sqlite:' . $dbFile, 'sid'); + $data = $storage->read('id'); + $storage->write('id', 'data'); + $storage->close(); + $this->assertSame('', $data, 'New session returns empty string data'); + + $storage->open('sqlite:' . $dbFile, 'sid'); + $data = $storage->read('id'); + $storage->close(); + $this->assertSame('data', $data, 'Written value can be read back correctly'); + + @unlink($dbFile); + } + public function testReadWriteRead() { $storage = new PdoSessionHandler($this->pdo); $storage->open('', 'sid'); - $this->assertSame('', $storage->read('id'), 'New session returns empty string data'); + $data = $storage->read('id'); $storage->write('id', 'data'); $storage->close(); + $this->assertSame('', $data, 'New session returns empty string data'); $storage->open('', 'sid'); - $this->assertSame('data', $storage->read('id'), 'Written value can be read back correctly'); + $data = $storage->read('id'); $storage->close(); + $this->assertSame('data', $data, 'Written value can be read back correctly'); } /** @@ -77,8 +134,9 @@ public function testWriteDifferentSessionIdThanRead() $storage->close(); $storage->open('', 'sid'); - $this->assertSame('data_of_new_session_id', $storage->read('new_id'), 'Data of regenerated session id is available'); + $data = $storage->read('new_id'); $storage->close(); + $this->assertSame('data_of_new_session_id', $data, 'Data of regenerated session id is available'); } /** @@ -109,8 +167,9 @@ public function testSessionDestroy() $this->assertEquals(0, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); $storage->open('', 'sid'); - $this->assertSame('', $storage->read('id'), 'Destroyed session returns empty string'); + $data = $storage->read('id'); $storage->close(); + $this->assertSame('', $data, 'Destroyed session returns empty string'); } public function testSessionGC() @@ -125,12 +184,14 @@ public function testSessionGC() $this->assertEquals(1, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); $storage->open('', 'sid'); - $this->assertSame('', $storage->read('id'), 'Session already considered garbage, so not returning data even if it is not pruned yet'); + $data = $storage->read('id'); $storage->gc(0); $storage->close(); - $this->assertEquals(0, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); ini_set('session.gc_maxlifetime', $previousLifeTime); + + $this->assertSame('', $data, 'Session already considered garbage, so not returning data even if it is not pruned yet'); + $this->assertEquals(0, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); } public function testGetConnection() From af1bb1f6e7e66322323eb8fa036d77b1b52d1c06 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 18 May 2014 04:10:22 +0200 Subject: [PATCH 3/8] add test for null byte in session data --- .../Storage/Handler/PdoSessionHandlerTest.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 3356e3dcea34a..9b29309ad4884 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -106,19 +106,21 @@ public function testWithLazySavePathConnection() @unlink($dbFile); } - public function testReadWriteRead() + public function testReadWriteReadWithNullByte() { + $sessionData = 'da' . "\0" . 'ta'; + $storage = new PdoSessionHandler($this->pdo); $storage->open('', 'sid'); - $data = $storage->read('id'); - $storage->write('id', 'data'); + $readData = $storage->read('id'); + $storage->write('id', $sessionData); $storage->close(); - $this->assertSame('', $data, 'New session returns empty string data'); + $this->assertSame('', $readData, 'New session returns empty string data'); $storage->open('', 'sid'); - $data = $storage->read('id'); + $readData = $storage->read('id'); $storage->close(); - $this->assertSame('data', $data, 'Written value can be read back correctly'); + $this->assertSame($sessionData, $readData, 'Written value can be read back correctly'); } /** From e79229d4d3eaf2143e8497d1608ad66fb726d0c9 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 18 May 2014 23:56:35 +0200 Subject: [PATCH 4/8] [HttpFoundation] allow different lifetime per session --- .../Storage/Handler/PdoSessionHandler.php | 98 ++++++++++--------- .../Storage/Handler/PdoSessionHandlerTest.php | 42 +++++--- 2 files changed, 79 insertions(+), 61 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index 9d5fa8f4850ab..e5365dd31e22f 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -42,9 +42,9 @@ class PdoSessionHandler implements \SessionHandlerInterface private $pdo; /** - * @var string|null|false DNS string or null for session.save_path or false when lazy connection disabled + * @var string|null|false DSN string or null for session.save_path or false when lazy connection disabled */ - private $dns = false; + private $dsn = false; /** * @var string Database driver @@ -66,6 +66,11 @@ class PdoSessionHandler implements \SessionHandlerInterface */ private $dataCol; + /** + * @var string Column for lifetime + */ + private $lifetimeCol; + /** * @var string Column for timestamp */ @@ -100,40 +105,43 @@ class PdoSessionHandler implements \SessionHandlerInterface * Constructor. * * You can either pass an existing database connection as PDO instance or - * pass a DNS string that will be used to lazy-connect to the database + * pass a DSN string that will be used to lazy-connect to the database * when the session is actually used. Furthermore it's possible to pass null - * which will then use the session.save_path ini setting as PDO DNS parameter. + * which will then use the session.save_path ini setting as PDO DSN parameter. * * List of available options: * * db_table: The name of the table [default: sessions] * * db_id_col: The column where to store the session id [default: sess_id] * * db_data_col: The column where to store the session data [default: sess_data] + * * db_lifetime_col: The column where to store the lifetime [default: sess_lifetime] * * db_time_col: The column where to store the timestamp [default: sess_time] * * db_username: The username when lazy-connect [default: ''] * * db_password: The password when lazy-connect [default: ''] * * db_connection_options: An array of driver-specific connection options [default: array()] * - * @param \PDO|string|null $pdoOrDns A \PDO instance or DNS string or null + * @param \PDO|string|null $pdoOrDsn A \PDO instance or DSN string or null * @param array $options An associative array of DB options * * @throws \InvalidArgumentException When PDO error mode is not PDO::ERRMODE_EXCEPTION */ - public function __construct($pdoOrDns, array $options = array()) + public function __construct($pdoOrDsn, array $options = array()) { - if ($pdoOrDns instanceof \PDO) { - if (\PDO::ERRMODE_EXCEPTION !== $pdoOrDns->getAttribute(\PDO::ATTR_ERRMODE)) { + if ($pdoOrDsn instanceof \PDO) { + if (\PDO::ERRMODE_EXCEPTION !== $pdoOrDsn->getAttribute(\PDO::ATTR_ERRMODE)) { throw new \InvalidArgumentException(sprintf('"%s" requires PDO error mode attribute be set to throw Exceptions (i.e. $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION))', __CLASS__)); } - $this->pdo = $pdoOrDns; + $this->pdo = $pdoOrDsn; + $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); } else { - $this->dns = $pdoOrDns; + $this->dsn = $pdoOrDsn; } $options = array_replace(array( 'db_table' => 'sessions', 'db_id_col' => 'sess_id', 'db_data_col' => 'sess_data', + 'db_lifetime_col' => 'sess_lifetime', 'db_time_col' => 'sess_time', 'db_username' => '', 'db_password' => '', @@ -143,6 +151,7 @@ public function __construct($pdoOrDns, array $options = array()) $this->table = $options['db_table']; $this->idCol = $options['db_id_col']; $this->dataCol = $options['db_data_col']; + $this->lifetimeCol = $options['db_lifetime_col']; $this->timeCol = $options['db_time_col']; $this->username = $options['db_username']; $this->password = $options['db_password']; @@ -156,10 +165,10 @@ public function open($savePath, $sessionName) { $this->gcCalled = false; if (null === $this->pdo) { - $this->pdo = new \PDO($this->dns ?: $savePath, $this->username, $this->password, $this->connectionOptions); + $this->pdo = new \PDO($this->dsn ?: $savePath, $this->username, $this->password, $this->connectionOptions); $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); } - $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); return true; } @@ -176,13 +185,12 @@ public function read($sessionId) // We need to make sure we do not return session data that is already considered garbage according // to the session.gc_maxlifetime setting because gc() is called after read() and only sometimes. - $maxlifetime = (int) ini_get('session.gc_maxlifetime'); - $sql = "SELECT $this->dataCol FROM $this->table WHERE $this->idCol = :id AND $this->timeCol > :time"; + $sql = "SELECT $this->dataCol FROM $this->table WHERE $this->idCol = :id AND $this->lifetimeCol + $this->timeCol >= :time"; $stmt = $this->pdo->prepare($sql); $stmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $stmt->bindValue(':time', time() - $maxlifetime, \PDO::PARAM_INT); + $stmt->bindValue(':time', time(), \PDO::PARAM_INT); $stmt->execute(); // We use fetchAll instead of fetchColumn to make sure the DB cursor gets closed @@ -239,6 +247,8 @@ public function write($sessionId, $data) // do an insert or update even if we created a row in read() for locking. // We use a single MERGE SQL query when supported by the database. + $maxlifetime = (int) ini_get('session.gc_maxlifetime'); + try { $mergeSql = $this->getMergeSql(); @@ -246,6 +256,7 @@ public function write($sessionId, $data) $mergeStmt = $this->pdo->prepare($mergeSql); $mergeStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); $mergeStmt->bindParam(':data', $data, \PDO::PARAM_LOB); + $mergeStmt->bindParam(':lifetime', $maxlifetime, \PDO::PARAM_INT); $mergeStmt->bindValue(':time', time(), \PDO::PARAM_INT); $mergeStmt->execute(); @@ -253,10 +264,11 @@ public function write($sessionId, $data) } $updateStmt = $this->pdo->prepare( - "UPDATE $this->table SET $this->dataCol = :data, $this->timeCol = :time WHERE $this->idCol = :id" + "UPDATE $this->table SET $this->dataCol = :data, $this->lifetimeCol = :lifetime, $this->timeCol = :time WHERE $this->idCol = :id" ); $updateStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); $updateStmt->bindParam(':data', $data, \PDO::PARAM_LOB); + $updateStmt->bindParam(':lifetime', $maxlifetime, \PDO::PARAM_INT); $updateStmt->bindValue(':time', time(), \PDO::PARAM_INT); $updateStmt->execute(); @@ -270,10 +282,11 @@ public function write($sessionId, $data) if (!$updateStmt->rowCount()) { try { $insertStmt = $this->pdo->prepare( - "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)" + "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time)" ); $insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $insertStmt->bindParam(':data', $encoded, \PDO::PARAM_LOB); + $insertStmt->bindParam(':data', $data, \PDO::PARAM_LOB); + $insertStmt->bindParam(':lifetime', $maxlifetime, \PDO::PARAM_INT); $insertStmt->bindValue(':time', time(), \PDO::PARAM_INT); $insertStmt->execute(); } catch (\PDOException $e) { @@ -302,17 +315,15 @@ public function close() $this->commit(); if ($this->gcCalled) { - $maxlifetime = (int) ini_get('session.gc_maxlifetime'); - // delete the session records that have expired - $sql = "DELETE FROM $this->table WHERE $this->timeCol <= :time"; + $sql = "DELETE FROM $this->table WHERE $this->lifetimeCol + $this->timeCol < :time"; $stmt = $this->pdo->prepare($sql); - $stmt->bindValue(':time', time() - $maxlifetime, \PDO::PARAM_INT); + $stmt->bindValue(':time', time(), \PDO::PARAM_INT); $stmt->execute(); } - if (false !== $this->dns) { + if (false !== $this->dsn) { $this->pdo = null; } @@ -329,20 +340,14 @@ public function close() */ private function beginTransaction() { - if ($this->inTransaction) { - $this->rollback(); - - throw new \BadMethodCallException( - 'Session handler methods have been invoked in wrong sequence. '. - 'Expected sequence: open() -> read() -> destroy() / write() -> close()'); - } - - if ('sqlite' === $this->driver) { - $this->pdo->exec('BEGIN IMMEDIATE TRANSACTION'); - } else { - $this->pdo->beginTransaction(); + if (!$this->inTransaction) { + if ('sqlite' === $this->driver) { + $this->pdo->exec('BEGIN IMMEDIATE TRANSACTION'); + } else { + $this->pdo->beginTransaction(); + } + $this->inTransaction = true; } - $this->inTransaction = true; } /** @@ -400,20 +405,20 @@ private function lockSession($sessionId) switch ($this->driver) { case 'mysql': // will also lock the row when actually nothing got updated (id = id) - $sql = "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". + $sql = "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". "ON DUPLICATE KEY UPDATE $this->idCol = $this->idCol"; break; case 'oci': // DUAL is Oracle specific dummy table $sql = "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". + "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". "WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol"; break; // todo: implement locking for SQL Server < 2008 case 'sqlsrv' === $this->driver && version_compare($this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION), '10', '>='): // MS SQL Server requires MERGE be terminated by semicolon $sql = "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". + "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". "WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol;"; break; case 'pgsql': @@ -434,6 +439,7 @@ private function lockSession($sessionId) $stmt = $this->pdo->prepare($sql); $stmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); $stmt->bindValue(':data', '', \PDO::PARAM_STR); + $stmt->bindValue(':lifetime', 0, \PDO::PARAM_INT); $stmt->bindValue(':time', time(), \PDO::PARAM_INT); $stmt->execute(); } @@ -447,21 +453,21 @@ private function getMergeSql() { switch ($this->driver) { case 'mysql': - return "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". - "ON DUPLICATE KEY UPDATE $this->dataCol = VALUES($this->dataCol), $this->timeCol = VALUES($this->timeCol)"; + return "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". + "ON DUPLICATE KEY UPDATE $this->dataCol = VALUES($this->dataCol), $this->lifetimeCol = VALUES($this->lifetimeCol), $this->timeCol = VALUES($this->timeCol)"; case 'oci': // DUAL is Oracle specific dummy table return "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". - "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time"; + "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". + "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->lifetimeCol = :lifetime, $this->timeCol = :time"; case 'sqlsrv' === $this->driver && version_compare($this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION), '10', '>='): // MERGE is only available since SQL Server 2008 and must be terminated by semicolon // It also requires HOLDLOCK according to http://weblogs.sqlteam.com/dang/archive/2009/01/31/UPSERT-Race-Condition-With-MERGE.aspx return "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time) ". - "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->timeCol = :time;"; + "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". + "WHEN MATCHED THEN UPDATE SET $this->dataCol = :data, $this->lifetimeCol = :lifetime, $this->timeCol = :time;"; case 'sqlite': - return "INSERT OR REPLACE INTO $this->table ($this->idCol, $this->dataCol, $this->timeCol) VALUES (:id, :data, :time)"; + return "INSERT OR REPLACE INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time)"; } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 9b29309ad4884..36da3645c5560 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -25,7 +25,7 @@ protected function setUp() $this->pdo = new \PDO('sqlite::memory:'); $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; $this->pdo->exec($sql); } @@ -59,7 +59,7 @@ public function testWithLazyDnsConnection() } $pdo = new \PDO('sqlite:' . $dbFile); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(255) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; $pdo->exec($sql); $pdo = null; @@ -86,7 +86,7 @@ public function testWithLazySavePathConnection() } $pdo = new \PDO('sqlite:' . $dbFile); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(255) PRIMARY KEY, sess_data BLOB, sess_time INTEGER)'; + $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; $pdo->exec($sql); $pdo = null; @@ -138,18 +138,24 @@ public function testWriteDifferentSessionIdThanRead() $storage->open('', 'sid'); $data = $storage->read('new_id'); $storage->close(); + $this->assertSame('data_of_new_session_id', $data, 'Data of regenerated session id is available'); } - /** - * @expectedException \BadMethodCallException - */ - public function testWrongUsage() + public function testWrongUsageStillWorks() { + // wrong method sequence that should no happen, but still works $storage = new PdoSessionHandler($this->pdo); + $storage->write('id', 'data'); + $storage->write('other_id', 'other_data'); + $storage->destroy('inexistent'); $storage->open('', 'sid'); - $storage->read('id'); - $storage->read('id'); + $data = $storage->read('id'); + $otherData = $storage->read('other_id'); + $storage->close(); + + $this->assertSame('data', $data); + $this->assertSame('other_data', $otherData); } public function testSessionDestroy() @@ -176,29 +182,35 @@ public function testSessionDestroy() public function testSessionGC() { - $previousLifeTime = ini_set('session.gc_maxlifetime', 0); + $previousLifeTime = ini_set('session.gc_maxlifetime', 1000); $storage = new PdoSessionHandler($this->pdo); $storage->open('', 'sid'); $storage->read('id'); $storage->write('id', 'data'); $storage->close(); - $this->assertEquals(1, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); $storage->open('', 'sid'); - $data = $storage->read('id'); - $storage->gc(0); + $storage->read('gc_id'); + ini_set('session.gc_maxlifetime', -1); // test that you can set lifetime of a session after it has been read + $storage->write('gc_id', 'data'); + $storage->close(); + $this->assertEquals(2, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'No session pruned because gc not called'); + + $storage->open('', 'sid'); + $data = $storage->read('gc_id'); + $storage->gc(-1); $storage->close(); ini_set('session.gc_maxlifetime', $previousLifeTime); $this->assertSame('', $data, 'Session already considered garbage, so not returning data even if it is not pruned yet'); - $this->assertEquals(0, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); + $this->assertEquals(1, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'Expired session is pruned'); } public function testGetConnection() { - $storage = new PdoSessionHandler($this->pdo, array('db_table' => 'sessions'), array()); + $storage = new PdoSessionHandler($this->pdo); $method = new \ReflectionMethod($storage, 'getConnection'); $method->setAccessible(true); From 182a5d39dff3318cee477a88d0621eb93032090a Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 19 May 2014 01:56:41 +0200 Subject: [PATCH 5/8] [HttpFoundation] add create table method to pdo session handler --- .../Storage/Handler/PdoSessionHandler.php | 77 +++++++++++- .../Storage/Handler/PdoSessionHandlerTest.php | 114 +++++++++++------- 2 files changed, 140 insertions(+), 51 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index e5365dd31e22f..dea848238000d 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -26,7 +26,8 @@ * * Session data is a binary string that can contain non-printable characters like the null byte. * For this reason it must be saved in a binary column in the database like BLOB in MySQL. - * Saving it in a character column could corrupt the data. + * Saving it in a character column could corrupt the data. You can use createTable() + * to initialize a correctly defined table. * * @see http://php.net/sessionhandlerinterface * @@ -158,16 +159,58 @@ public function __construct($pdoOrDsn, array $options = array()) $this->connectionOptions = $options['db_connection_options']; } + /** + * Creates the table to store sessions which can be called once for setup. + * + * Session ID is saved in a VARCHAR(128) column because that is enough even for + * a 512 bit configured session.hash_function like Whirlpool. Session data is + * saved in a BLOB. One could also use a shorter inlined varbinary column + * if one was sure the data fits into it. + * + * @throws \PDOException When the table already exists + * @throws \DomainException When an unsupported PDO driver is used + */ + public function createTable() + { + // connect if we are not yet + $this->getConnection(); + + switch ($this->driver) { + case 'mysql': + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL) ENGINE = InnoDB"; + break; + case 'sqlite': + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL)"; + break; + case 'pgsql': + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BYTEA NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; + break; + case 'oci': + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR2(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; + break; + case 'sqlsrv': + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol VARBINARY(MAX) NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; + break; + default: + throw new \DomainException(sprintf('"%s" does not currently support PDO driver "%s".', __CLASS__, $this->driver)); + } + + try { + $this->pdo->exec($sql); + } catch (\PDOException $e) { + $this->rollback(); + + throw $e; + } + } + /** * {@inheritdoc} */ public function open($savePath, $sessionName) { - $this->gcCalled = false; if (null === $this->pdo) { - $this->pdo = new \PDO($this->dsn ?: $savePath, $this->username, $this->password, $this->connectionOptions); - $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); - $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); + $this->connect($this->dsn ?: $savePath); } return true; @@ -315,6 +358,8 @@ public function close() $this->commit(); if ($this->gcCalled) { + $this->gcCalled = false; + // delete the session records that have expired $sql = "DELETE FROM $this->table WHERE $this->lifetimeCol + $this->timeCol < :time"; @@ -330,6 +375,18 @@ public function close() return true; } + /** + * Lazy-connects to the database. + * + * @param string $dsn DSN string + */ + private function connect($dsn) + { + $this->pdo = new \PDO($dsn, $this->username, $this->password, $this->connectionOptions); + $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + $this->driver = $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME); + } + /** * Helper method to begin a transaction. * @@ -399,6 +456,8 @@ private function rollback() * INSERT when not found can result in a deadlock for one connection. * * @param string $sessionId Session ID + * + * @throws \DomainException When an unsupported PDO driver is used */ private function lockSession($sessionId) { @@ -429,8 +488,10 @@ private function lockSession($sessionId) $stmt->execute(); return; + case 'sqlite': + return; // we already locked when starting transaction default: - return; + throw new \DomainException(sprintf('"%s" does not currently support PDO driver "%s".', __CLASS__, $this->driver)); } // We create a DML lock for the session by inserting empty data or updating the row. @@ -478,6 +539,10 @@ private function getMergeSql() */ protected function getConnection() { + if (null === $this->pdo) { + $this->connect($this->dsn ?: ini_get('session.save_path')); + } + return $this->pdo; } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 36da3645c5560..3fa6cd58afe4b 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -15,18 +15,38 @@ class PdoSessionHandlerTest extends \PHPUnit_Framework_TestCase { - private $pdo; + private $dbFile; protected function setUp() { if (!class_exists('PDO') || !in_array('sqlite', \PDO::getAvailableDrivers())) { $this->markTestSkipped('This test requires SQLite support in your environment'); } + } + + protected function tearDown() + { + // make sure the temporary database file is deleted when it has been created (even when a test fails) + if ($this->dbFile) { + @unlink($this->dbFile); + } + } + + protected function getPersistentSqliteDsn() + { + $this->dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); - $this->pdo = new \PDO('sqlite::memory:'); - $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; - $this->pdo->exec($sql); + return 'sqlite:' . $this->dbFile; + } + + protected function getMemorySqlitePdo() + { + $pdo = new \PDO('sqlite::memory:'); + $pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + $storage = new PdoSessionHandler($pdo); + $storage->createTable(); + + return $pdo; } /** @@ -34,9 +54,10 @@ protected function setUp() */ public function testWrongPdoErrMode() { - $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_SILENT); + $pdo = $this->getMemorySqlitePdo(); + $pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_SILENT); - $storage = new PdoSessionHandler($this->pdo); + $storage = new PdoSessionHandler($pdo); } /** @@ -44,26 +65,28 @@ public function testWrongPdoErrMode() */ public function testInexistentTable() { - $storage = new PdoSessionHandler($this->pdo, array('db_table' => 'inexistent_table')); + $storage = new PdoSessionHandler($this->getMemorySqlitePdo(), array('db_table' => 'inexistent_table')); $storage->open('', 'sid'); $storage->read('id'); $storage->write('id', 'data'); $storage->close(); } - public function testWithLazyDnsConnection() + /** + * @expectedException \RuntimeException + */ + public function testCreateTableTwice() { - $dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); - if (file_exists($dbFile)) { - @unlink($dbFile); - } + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); + $storage->createTable(); + } - $pdo = new \PDO('sqlite:' . $dbFile); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; - $pdo->exec($sql); - $pdo = null; + public function testWithLazyDsnConnection() + { + $dsn = $this->getPersistentSqliteDsn(); - $storage = new PdoSessionHandler('sqlite:' . $dbFile); + $storage = new PdoSessionHandler($dsn); + $storage->createTable(); $storage->open('', 'sid'); $data = $storage->read('id'); $storage->write('id', 'data'); @@ -74,43 +97,32 @@ public function testWithLazyDnsConnection() $data = $storage->read('id'); $storage->close(); $this->assertSame('data', $data, 'Written value can be read back correctly'); - - @unlink($dbFile); } public function testWithLazySavePathConnection() { - $dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); - if (file_exists($dbFile)) { - @unlink($dbFile); - } + $dsn = $this->getPersistentSqliteDsn(); - $pdo = new \PDO('sqlite:' . $dbFile); - $sql = 'CREATE TABLE sessions (sess_id VARCHAR(128) PRIMARY KEY, sess_data BLOB, sess_lifetime MEDIUMINT, sess_time INTEGER)'; - $pdo->exec($sql); - $pdo = null; - - // Open is called with what ini_set('session.save_path', 'sqlite:' . $dbFile) would mean + // Open is called with what ini_set('session.save_path', $dsn) would mean $storage = new PdoSessionHandler(null); - $storage->open('sqlite:' . $dbFile, 'sid'); + $storage->open($dsn, 'sid'); + $storage->createTable(); $data = $storage->read('id'); $storage->write('id', 'data'); $storage->close(); $this->assertSame('', $data, 'New session returns empty string data'); - $storage->open('sqlite:' . $dbFile, 'sid'); + $storage->open($dsn, 'sid'); $data = $storage->read('id'); $storage->close(); $this->assertSame('data', $data, 'Written value can be read back correctly'); - - @unlink($dbFile); } public function testReadWriteReadWithNullByte() { $sessionData = 'da' . "\0" . 'ta'; - $storage = new PdoSessionHandler($this->pdo); + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); $storage->open('', 'sid'); $readData = $storage->read('id'); $storage->write('id', $sessionData); @@ -128,7 +140,7 @@ public function testReadWriteReadWithNullByte() */ public function testWriteDifferentSessionIdThanRead() { - $storage = new PdoSessionHandler($this->pdo); + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); $storage->open('', 'sid'); $storage->read('id'); $storage->destroy('id'); @@ -139,13 +151,13 @@ public function testWriteDifferentSessionIdThanRead() $data = $storage->read('new_id'); $storage->close(); - $this->assertSame('data_of_new_session_id', $data, 'Data of regenerated session id is available'); + $this->assertSame('data_of_new_session_id', $data, 'Data of regenerated session id is available'); } public function testWrongUsageStillWorks() { // wrong method sequence that should no happen, but still works - $storage = new PdoSessionHandler($this->pdo); + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); $storage->write('id', 'data'); $storage->write('other_id', 'other_data'); $storage->destroy('inexistent'); @@ -160,19 +172,20 @@ public function testWrongUsageStillWorks() public function testSessionDestroy() { - $storage = new PdoSessionHandler($this->pdo); + $pdo = $this->getMemorySqlitePdo(); + $storage = new PdoSessionHandler($pdo); $storage->open('', 'sid'); $storage->read('id'); $storage->write('id', 'data'); $storage->close(); - $this->assertEquals(1, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); + $this->assertEquals(1, $pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); $storage->open('', 'sid'); $storage->read('id'); $storage->destroy('id'); $storage->close(); - $this->assertEquals(0, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); + $this->assertEquals(0, $pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn()); $storage->open('', 'sid'); $data = $storage->read('id'); @@ -183,7 +196,8 @@ public function testSessionDestroy() public function testSessionGC() { $previousLifeTime = ini_set('session.gc_maxlifetime', 1000); - $storage = new PdoSessionHandler($this->pdo); + $pdo = $this->getMemorySqlitePdo(); + $storage = new PdoSessionHandler($pdo); $storage->open('', 'sid'); $storage->read('id'); @@ -195,7 +209,7 @@ public function testSessionGC() ini_set('session.gc_maxlifetime', -1); // test that you can set lifetime of a session after it has been read $storage->write('gc_id', 'data'); $storage->close(); - $this->assertEquals(2, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'No session pruned because gc not called'); + $this->assertEquals(2, $pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'No session pruned because gc not called'); $storage->open('', 'sid'); $data = $storage->read('gc_id'); @@ -205,12 +219,22 @@ public function testSessionGC() ini_set('session.gc_maxlifetime', $previousLifeTime); $this->assertSame('', $data, 'Session already considered garbage, so not returning data even if it is not pruned yet'); - $this->assertEquals(1, $this->pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'Expired session is pruned'); + $this->assertEquals(1, $pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(), 'Expired session is pruned'); } public function testGetConnection() { - $storage = new PdoSessionHandler($this->pdo); + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); + + $method = new \ReflectionMethod($storage, 'getConnection'); + $method->setAccessible(true); + + $this->assertInstanceOf('\PDO', $method->invoke($storage)); + } + + public function testGetConnectionConnectsIfNeeded() + { + $storage = new PdoSessionHandler('sqlite::memory:'); $method = new \ReflectionMethod($storage, 'getConnection'); $method->setAccessible(true); From 5978fcfb089e72aded43052e57de8a563e0ce8f0 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 25 May 2014 15:52:29 +0200 Subject: [PATCH 6/8] added upgrade and changelog notes for PdoSessionHandler --- UPGRADE-2.6.md | 24 ++++++++++++++++++- .../Component/HttpFoundation/CHANGELOG.md | 11 +++++++++ .../Storage/Handler/PdoSessionHandler.php | 3 +++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/UPGRADE-2.6.md b/UPGRADE-2.6.md index 03a7da49dd9bc..5fbdc90743e80 100644 --- a/UPGRADE-2.6.md +++ b/UPGRADE-2.6.md @@ -1,4 +1,4 @@ -UPGRADE FROM 2.5 to 2.6 +UPGRADE FROM 2.5 to 2.6 ======================= Form @@ -101,3 +101,25 @@ Security @security.token_storage => getToken() @security.token_storage => setToken() ``` + +HttpFoundation +-------------- + + * The PdoSessionHandler to store sessions in a database changed significantly. + - It now implements session locking to prevent loss of data by concurrent access to the same session. + - It does so using a transaction between opening and closing a session. For this reason, it's not + recommended to use the same database connection that you also use for your application logic. + Otherwise you have to make sure to access your database after the session is closed and committed. + Instead of passing an existing connection to the handler, you can now also pass a DSN string which + will be used to lazy-connect when a session is started. + - Since accessing a session now blocks when the same session is still open, it is best practice to + save the session as soon as you don't need to write to it anymore. For example, read-only AJAX + request to a session can save the session immediately after opening it to increase concurrency. + - The expected schema of the table changed. + - Session data is binary text that can contain null bytes and thus should also be saved as-is in a + binary column like BLOB. For this reason, the handler does not base64_encode the data anymore. + - A new column to store the lifetime of a session is required. This allows to have different + lifetimes per session configured via session.gc_maxlifetime ini setting. + - You would need to migrate the table manually if you want to keep session information of your users. + - You could use PdoSessionHandler::createTable to initialize a correctly defined table depending on + the used database vendor. diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md index 0da734bce2f1c..0b25cf89d2bb4 100644 --- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md +++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md @@ -1,6 +1,17 @@ CHANGELOG ========= +2.6.0 +----- + + * PdoSessionHandler changes + - implemented session locking to prevent loss of data by concurrent access to the same session + - save session data in a binary column without base64_encode + - added lifetime column to the session table which allows to have different lifetimes for each session + - implemented lazy connections that are only opened when a session is used by either passing a dsn string + explicitly or falling back to session.save_path ini setting + - added a createTable method that initializes a correctly defined table depending on the database vendor + 2.5.0 ----- diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index dea848238000d..2ed3dee7eb90e 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -109,6 +109,9 @@ class PdoSessionHandler implements \SessionHandlerInterface * pass a DSN string that will be used to lazy-connect to the database * when the session is actually used. Furthermore it's possible to pass null * which will then use the session.save_path ini setting as PDO DSN parameter. + * Since locking uses a transaction between opening and closing a session, + * it's not recommended to use the same database connection that you also use + * for your application logic. * * List of available options: * * db_table: The name of the table [default: sessions] From 6f5748e05745578171dde19039fadb9d87e8382c Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 4 Jun 2014 23:06:34 +0200 Subject: [PATCH 7/8] adjust sqlite table definition --- .../Storage/Handler/PdoSessionHandler.php | 4 ++-- .../Storage/Handler/PdoSessionHandlerTest.php | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index 2ed3dee7eb90e..f8cc28ff7b7fe 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -180,10 +180,10 @@ public function createTable() switch ($this->driver) { case 'mysql': - $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL) ENGINE = InnoDB"; + $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER UNSIGNED NOT NULL) ENGINE = InnoDB"; break; case 'sqlite': - $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER NOT NULL)"; + $sql = "CREATE TABLE $this->table ($this->idCol TEXT NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; break; case 'pgsql': $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BYTEA NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 3fa6cd58afe4b..418444766e8c6 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -135,6 +135,28 @@ public function testReadWriteReadWithNullByte() $this->assertSame($sessionData, $readData, 'Written value can be read back correctly'); } + public function testReadingRequiresExactlySameId() + { + $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); + $storage->open('', 'sid'); + $storage->write('id', 'data'); + $storage->write('test', 'data'); + $storage->write('space ', 'data'); + $storage->close(); + + $storage->open('', 'sid'); + $readDataCaseSensitive = $storage->read('ID'); + $readDataNoCharFolding = $storage->read('tést'); + $readDataKeepSpace = $storage->read('space '); + $readDataExtraSpace = $storage->read('space '); + $storage->close(); + + $this->assertSame('', $readDataCaseSensitive, 'Retrieval by ID should be case-sensitive (collation setting)'); + $this->assertSame('', $readDataNoCharFolding, 'Retrieval by ID should not do character folding (collation setting)'); + $this->assertSame('data', $readDataKeepSpace, 'Retrieval by ID requires spaces as-is'); + $this->assertSame('', $readDataExtraSpace, 'Retrieval by ID requires spaces as-is'); + } + /** * Simulates session_regenerate_id(true) which will require an INSERT or UPDATE (replace) */ From 1bc6680928b5601ecef29f358d8311f847280dae Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 8 Jun 2014 00:54:38 +0200 Subject: [PATCH 8/8] [HttpFoundation] implement different locking strategies for sessions --- UPGRADE-2.6.md | 11 +- .../Component/HttpFoundation/CHANGELOG.md | 6 +- .../Storage/Handler/PdoSessionHandler.php | 360 ++++++++++++------ .../Storage/Handler/PdoSessionHandlerTest.php | 4 +- 4 files changed, 263 insertions(+), 118 deletions(-) diff --git a/UPGRADE-2.6.md b/UPGRADE-2.6.md index 5fbdc90743e80..817d35f5e7b9a 100644 --- a/UPGRADE-2.6.md +++ b/UPGRADE-2.6.md @@ -105,8 +105,8 @@ Security HttpFoundation -------------- - * The PdoSessionHandler to store sessions in a database changed significantly. - - It now implements session locking to prevent loss of data by concurrent access to the same session. + * The `PdoSessionHandler` to store sessions in a database changed significantly. + - By default, it now implements session locking to prevent loss of data by concurrent access to the same session. - It does so using a transaction between opening and closing a session. For this reason, it's not recommended to use the same database connection that you also use for your application logic. Otherwise you have to make sure to access your database after the session is closed and committed. @@ -115,11 +115,16 @@ HttpFoundation - Since accessing a session now blocks when the same session is still open, it is best practice to save the session as soon as you don't need to write to it anymore. For example, read-only AJAX request to a session can save the session immediately after opening it to increase concurrency. + - As alternative to transactional locking you can also use advisory locks which do not require a transaction. + Additionally, you can also revert back to no locking in case you have custom logic to deal with race conditions + like an optimistic concurrency control approach. The locking strategy can be chosen by passing the corresponding + constant as `lock_mode` option, e.g. `new PdoSessionHandler($pdoOrDsn, array('lock_mode' => PdoSessionHandler::LOCK_NONE))`. + For more information please read the class documentation. - The expected schema of the table changed. - Session data is binary text that can contain null bytes and thus should also be saved as-is in a binary column like BLOB. For this reason, the handler does not base64_encode the data anymore. - A new column to store the lifetime of a session is required. This allows to have different lifetimes per session configured via session.gc_maxlifetime ini setting. - You would need to migrate the table manually if you want to keep session information of your users. - - You could use PdoSessionHandler::createTable to initialize a correctly defined table depending on + - You could use `PdoSessionHandler::createTable` to initialize a correctly defined table depending on the used database vendor. diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md index 0b25cf89d2bb4..dcdeb4ebf9664 100644 --- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md +++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md @@ -5,9 +5,9 @@ CHANGELOG ----- * PdoSessionHandler changes - - implemented session locking to prevent loss of data by concurrent access to the same session - - save session data in a binary column without base64_encode - - added lifetime column to the session table which allows to have different lifetimes for each session + - implemented different session locking strategies to prevent loss of data by concurrent access to the same session + - [BC BREAK] save session data in a binary column without base64_encode + - [BC BREAK] added lifetime column to the session table which allows to have different lifetimes for each session - implemented lazy connections that are only opened when a session is used by either passing a dsn string explicitly or falling back to session.save_path ini setting - added a createTable method that initializes a correctly defined table depending on the database vendor diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index f8cc28ff7b7fe..26c5f8bc0ee77 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -15,9 +15,12 @@ * Session handler using a PDO connection to read and write data. * * It works with MySQL, PostgreSQL, Oracle, SQL Server and SQLite and implements - * locking of sessions to prevent loss of data by concurrent access to the same session. - * This means requests for the same session will wait until the other one finished. - * PHPs internal files session handler also works this way. + * different locking strategies to handle concurrent access to the same session. + * Locking is necessary to prevent loss of data due to race conditions and to keep + * the session data consistent between read() and write(). With locking, requests + * for the same session will wait until the other one finished writing. For this + * reason it's best practice to close a session as early as possible to improve + * concurrency. PHPs internal files session handler also implements locking. * * Attention: Since SQLite does not support row level locks but locks the whole database, * it means only one session can be accessed at a time. Even different sessions would wait @@ -37,6 +40,31 @@ */ class PdoSessionHandler implements \SessionHandlerInterface { + /** + * No locking is done. This means sessions are prone to loss of data due to + * race conditions of concurrent requests to the same session. The last session + * write will win in this case. It might be useful when you implement your own + * logic to deal with this like an optimistic approach. + */ + const LOCK_NONE = 0; + + /** + * Creates an application-level lock on a session. The disadvantage is that the + * lock is not enforced by the database and thus other, unaware parts of the + * application could still concurrently modify the session. The advantage is it + * does not require a transaction. + * This mode is not available for SQLite and not yet implemented for oci and sqlsrv. + */ + const LOCK_ADVISORY = 1; + + /** + * Issues a real row lock. Since it uses a transaction between opening and + * closing a session, you have to be careful when you use same database connection + * that you also use for your application logic. This mode is the default because + * it's the only reliable solution across DBMSs. + */ + const LOCK_TRANSACTIONAL = 2; + /** * @var \PDO|null PDO instance or null when not connected yet */ @@ -55,43 +83,60 @@ class PdoSessionHandler implements \SessionHandlerInterface /** * @var string Table name */ - private $table; + private $table = 'sessions'; /** * @var string Column for session id */ - private $idCol; + private $idCol = 'sess_id'; /** * @var string Column for session data */ - private $dataCol; + private $dataCol = 'sess_data'; /** * @var string Column for lifetime */ - private $lifetimeCol; + private $lifetimeCol = 'sess_lifetime'; /** * @var string Column for timestamp */ - private $timeCol; + private $timeCol = 'sess_time'; /** * @var string Username when lazy-connect */ - private $username; + private $username = ''; /** * @var string Password when lazy-connect */ - private $password; + private $password = ''; /** * @var array Connection options when lazy-connect */ private $connectionOptions = array(); + /** + * @var int The strategy for locking, see constants + */ + private $lockMode = self::LOCK_TRANSACTIONAL; + + /** + * It's an array to support multiple reads before closing which is manual, non-standard usage + * + * @var \PDOStatement[] An array of statements to release advisory locks + */ + private $unlockStatements = array(); + + /** + * @var bool True when the current session exists but expired according to session.gc_maxlifetime + */ + private $sessionExpired = false; + /** * @var bool Whether a transaction is active */ @@ -109,9 +154,6 @@ class PdoSessionHandler implements \SessionHandlerInterface * pass a DSN string that will be used to lazy-connect to the database * when the session is actually used. Furthermore it's possible to pass null * which will then use the session.save_path ini setting as PDO DSN parameter. - * Since locking uses a transaction between opening and closing a session, - * it's not recommended to use the same database connection that you also use - * for your application logic. * * List of available options: * * db_table: The name of the table [default: sessions] @@ -122,13 +164,14 @@ class PdoSessionHandler implements \SessionHandlerInterface * * db_username: The username when lazy-connect [default: ''] * * db_password: The password when lazy-connect [default: ''] * * db_connection_options: An array of driver-specific connection options [default: array()] + * * lock_mode: The strategy for locking, see constants [default: LOCK_TRANSACTIONAL] * * @param \PDO|string|null $pdoOrDsn A \PDO instance or DSN string or null - * @param array $options An associative array of DB options + * @param array $options An associative array of options * * @throws \InvalidArgumentException When PDO error mode is not PDO::ERRMODE_EXCEPTION */ - public function __construct($pdoOrDsn, array $options = array()) + public function __construct($pdoOrDsn = null, array $options = array()) { if ($pdoOrDsn instanceof \PDO) { if (\PDO::ERRMODE_EXCEPTION !== $pdoOrDsn->getAttribute(\PDO::ATTR_ERRMODE)) { @@ -141,32 +184,22 @@ public function __construct($pdoOrDsn, array $options = array()) $this->dsn = $pdoOrDsn; } - $options = array_replace(array( - 'db_table' => 'sessions', - 'db_id_col' => 'sess_id', - 'db_data_col' => 'sess_data', - 'db_lifetime_col' => 'sess_lifetime', - 'db_time_col' => 'sess_time', - 'db_username' => '', - 'db_password' => '', - 'db_connection_options' => array() - ), $options); - - $this->table = $options['db_table']; - $this->idCol = $options['db_id_col']; - $this->dataCol = $options['db_data_col']; - $this->lifetimeCol = $options['db_lifetime_col']; - $this->timeCol = $options['db_time_col']; - $this->username = $options['db_username']; - $this->password = $options['db_password']; - $this->connectionOptions = $options['db_connection_options']; + $this->table = isset($options['db_table']) ? $options['db_table'] : $this->table; + $this->idCol = isset($options['db_id_col']) ? $options['db_id_col'] : $this->idCol; + $this->dataCol = isset($options['db_data_col']) ? $options['db_data_col'] : $this->dataCol; + $this->lifetimeCol = isset($options['db_lifetime_col']) ? $options['db_lifetime_col'] : $this->lifetimeCol; + $this->timeCol = isset($options['db_time_col']) ? $options['db_time_col'] : $this->timeCol; + $this->username = isset($options['db_username']) ? $options['db_username'] : $this->username; + $this->password = isset($options['db_password']) ? $options['db_password'] : $this->password; + $this->connectionOptions = isset($options['db_connection_options']) ? $options['db_connection_options'] : $this->connectionOptions; + $this->lockMode = isset($options['lock_mode']) ? $options['lock_mode'] : $this->lockMode; } /** * Creates the table to store sessions which can be called once for setup. * - * Session ID is saved in a VARCHAR(128) column because that is enough even for - * a 512 bit configured session.hash_function like Whirlpool. Session data is + * Session ID is saved in a column of maximum length 128 because that is enough even + * for a 512 bit configured session.hash_function like Whirlpool. Session data is * saved in a BLOB. One could also use a shorter inlined varbinary column * if one was sure the data fits into it. * @@ -180,7 +213,12 @@ public function createTable() switch ($this->driver) { case 'mysql': - $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER UNSIGNED NOT NULL) ENGINE = InnoDB"; + // We use varbinary for the ID column because it prevents unwanted conversions: + // - character set conversions between server and client + // - trailing space removal + // - case-insensitivity + // - language processing like é == e + $sql = "CREATE TABLE $this->table ($this->idCol VARBINARY(128) NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol MEDIUMINT NOT NULL, $this->timeCol INTEGER UNSIGNED NOT NULL) COLLATE utf8_bin, ENGINE = InnoDB"; break; case 'sqlite': $sql = "CREATE TABLE $this->table ($this->idCol TEXT NOT NULL PRIMARY KEY, $this->dataCol BLOB NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; @@ -195,7 +233,7 @@ public function createTable() $sql = "CREATE TABLE $this->table ($this->idCol VARCHAR(128) NOT NULL PRIMARY KEY, $this->dataCol VARBINARY(MAX) NOT NULL, $this->lifetimeCol INTEGER NOT NULL, $this->timeCol INTEGER NOT NULL)"; break; default: - throw new \DomainException(sprintf('"%s" does not currently support PDO driver "%s".', __CLASS__, $this->driver)); + throw new \DomainException(sprintf('Creating the session table is currently not implemented for PDO driver "%s".', $this->driver)); } try { @@ -207,6 +245,18 @@ public function createTable() } } + /** + * Returns true when the current session exists but expired according to session.gc_maxlifetime. + * + * Can be used to distinguish between a new session and one that expired due to inactivity. + * + * @return bool Whether current session expired + */ + public function isSessionExpired() + { + return $this->sessionExpired; + } + /** * {@inheritdoc} */ @@ -224,25 +274,8 @@ public function open($savePath, $sessionName) */ public function read($sessionId) { - $this->beginTransaction(); - try { - $this->lockSession($sessionId); - - // We need to make sure we do not return session data that is already considered garbage according - // to the session.gc_maxlifetime setting because gc() is called after read() and only sometimes. - - $sql = "SELECT $this->dataCol FROM $this->table WHERE $this->idCol = :id AND $this->lifetimeCol + $this->timeCol >= :time"; - - $stmt = $this->pdo->prepare($sql); - $stmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $stmt->bindValue(':time', time(), \PDO::PARAM_INT); - $stmt->execute(); - - // We use fetchAll instead of fetchColumn to make sure the DB cursor gets closed - $sessionRows = $stmt->fetchAll(\PDO::FETCH_NUM); - - return $sessionRows ? $sessionRows[0][0] : ''; + return $this->doRead($sessionId); } catch (\PDOException $e) { $this->rollback(); @@ -288,14 +321,10 @@ public function destroy($sessionId) */ public function write($sessionId, $data) { - // The session ID can be different from the one previously received in read() - // when the session ID changed due to session_regenerate_id(). So we have to - // do an insert or update even if we created a row in read() for locking. - // We use a single MERGE SQL query when supported by the database. - $maxlifetime = (int) ini_get('session.gc_maxlifetime'); try { + // We use a single MERGE SQL query when supported by the database. $mergeSql = $this->getMergeSql(); if (null !== $mergeSql) { @@ -319,12 +348,10 @@ public function write($sessionId, $data) $updateStmt->execute(); // When MERGE is not supported, like in Postgres, we have to use this approach that can result in - // duplicate key errors when the same session is written simultaneously. We can just catch such an - // error and re-execute the update. This is similar to a serializable transaction with retry logic - // on serialization failures but without the overhead and without possible false positives due to - // longer gap locking. - // Since we have a lock on the session, the above case should not happen. And if it's a regenerated - // session ID it should be unique anyway. + // duplicate key errors when the same session is written simultaneously (given the LOCK_NONE behavior). + // We can just catch such an error and re-execute the update. This is similar to a serializable + // transaction with retry logic on serialization failures but without the overhead and without possible + // false positives due to longer gap locking. if (!$updateStmt->rowCount()) { try { $insertStmt = $this->pdo->prepare( @@ -360,6 +387,10 @@ public function close() { $this->commit(); + while ($unlockStmt = array_shift($this->unlockStatements)) { + $unlockStmt->execute(); + } + if ($this->gcCalled) { $this->gcCalled = false; @@ -372,7 +403,7 @@ public function close() } if (false !== $this->dsn) { - $this->pdo = null; + $this->pdo = null; // only close lazy-connection } return true; @@ -397,6 +428,10 @@ private function connect($dsn) * on the database immediately. Because of https://bugs.php.net/42766 we have to create * such a transaction manually which also means we cannot use PDO::commit or * PDO::rollback or PDO::inTransaction for SQLite. + * + * Also MySQLs default isolation, REPEATABLE READ, causes deadlock for different sessions + * due to http://www.mysqlperformanceblog.com/2013/12/12/one-more-innodb-gap-lock-to-avoid/ . + * So we change it to READ COMMITTED. */ private function beginTransaction() { @@ -404,6 +439,9 @@ private function beginTransaction() if ('sqlite' === $this->driver) { $this->pdo->exec('BEGIN IMMEDIATE TRANSACTION'); } else { + if ('mysql' === $this->driver) { + $this->pdo->exec('SET TRANSACTION ISOLATION LEVEL READ COMMITTED'); + } $this->pdo->beginTransaction(); } $this->inTransaction = true; @@ -439,8 +477,8 @@ private function rollback() { // We only need to rollback if we are in a transaction. Otherwise the resulting // error would hide the real problem why rollback was called. We might not be - // in a transaction when two callbacks (e.g. destroy and write) are invoked that - // both fail. + // in a transaction when not using the transactional locking behavior or when + // two callbacks (e.g. destroy and write) are invoked that both fail. if ($this->inTransaction) { if ('sqlite' === $this->driver) { $this->pdo->exec('ROLLBACK'); @@ -452,64 +490,166 @@ private function rollback() } /** - * Exclusively locks the row so other concurrent requests on the same session will block. + * Reads the session data in respect to the different locking strategies. * - * This prevents loss of data by keeping the data consistent between read() and write(). - * We do not use SELECT FOR UPDATE because it does not lock non-existent rows. And a following - * INSERT when not found can result in a deadlock for one connection. + * We need to make sure we do not return session data that is already considered garbage according + * to the session.gc_maxlifetime setting because gc() is called after read() and only sometimes. * * @param string $sessionId Session ID * + * @return string The session data + */ + private function doRead($sessionId) + { + $this->sessionExpired = false; + + if (self::LOCK_ADVISORY === $this->lockMode) { + $this->unlockStatements[] = $this->doAdvisoryLock($sessionId); + } + + $selectSql = $this->getSelectSql(); + $selectStmt = $this->pdo->prepare($selectSql); + $selectStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); + $selectStmt->execute(); + + $sessionRows = $selectStmt->fetchAll(\PDO::FETCH_NUM); + + if ($sessionRows) { + if ($sessionRows[0][1] + $sessionRows[0][2] < time()) { + $this->sessionExpired = true; + + return ''; + } + + return $sessionRows[0][0]; + } + + if (self::LOCK_TRANSACTIONAL === $this->lockMode && 'sqlite' !== $this->driver) { + // Exlusive-reading of non-existent rows does not block, so we need to do an insert to block + // until other connections to the session are committed. + try { + $insertStmt = $this->pdo->prepare( + "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time)" + ); + $insertStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); + $insertStmt->bindValue(':data', '', \PDO::PARAM_LOB); + $insertStmt->bindValue(':lifetime', 0, \PDO::PARAM_INT); + $insertStmt->bindValue(':time', time(), \PDO::PARAM_INT); + $insertStmt->execute(); + } catch (\PDOException $e) { + // Catch duplicate key error because other connection created the session already. + // It would only not be the case when the other connection destroyed the session. + if (0 === strpos($e->getCode(), '23')) { + // Retrieve finished session data written by concurrent connection. SELECT + // FOR UPDATE is necessary to avoid deadlock of connection that starts reading + // before we write (transform intention to real lock). + $selectStmt->execute(); + $sessionRows = $selectStmt->fetchAll(\PDO::FETCH_NUM); + + return $sessionRows ? $sessionRows[0][0] : ''; + } + + throw $e; + } + } + + return ''; + } + + /** + * Executes an application-level lock on the database. + * + * @param string $sessionId Session ID + * + * @return \PDOStatement The statement that needs to be executed later to release the lock + * * @throws \DomainException When an unsupported PDO driver is used + * + * @todo implement missing advisory locks + * - for oci using DBMS_LOCK.REQUEST + * - for sqlsrv using sp_getapplock with LockOwner = Session */ - private function lockSession($sessionId) + private function doAdvisoryLock($sessionId) { switch ($this->driver) { case 'mysql': - // will also lock the row when actually nothing got updated (id = id) - $sql = "INSERT INTO $this->table ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". - "ON DUPLICATE KEY UPDATE $this->idCol = $this->idCol"; - break; - case 'oci': - // DUAL is Oracle specific dummy table - $sql = "MERGE INTO $this->table USING DUAL ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". - "WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol"; - break; - // todo: implement locking for SQL Server < 2008 - case 'sqlsrv' === $this->driver && version_compare($this->pdo->getAttribute(\PDO::ATTR_SERVER_VERSION), '10', '>='): - // MS SQL Server requires MERGE be terminated by semicolon - $sql = "MERGE INTO $this->table WITH (HOLDLOCK) USING (SELECT 1 AS dummy) AS src ON ($this->idCol = :id) ". - "WHEN NOT MATCHED THEN INSERT ($this->idCol, $this->dataCol, $this->lifetimeCol, $this->timeCol) VALUES (:id, :data, :lifetime, :time) ". - "WHEN MATCHED THEN UPDATE SET $this->idCol = $this->idCol;"; - break; - case 'pgsql': - // obtain an exclusive transaction level advisory lock - $sql = 'SELECT pg_advisory_xact_lock(:lock_id)'; - $stmt = $this->pdo->prepare($sql); - $stmt->bindValue(':lock_id', hexdec(substr($sessionId, 0, 15)), \PDO::PARAM_INT); + // should we handle the return value? 0 on timeout, null on error + // we use a timeout of 50 seconds which is also the default for innodb_lock_wait_timeout + $stmt = $this->pdo->prepare('SELECT GET_LOCK(:key, 50)'); + $stmt->bindValue(':key', $sessionId, \PDO::PARAM_STR); $stmt->execute(); - return; + $releaseStmt = $this->pdo->prepare('DO RELEASE_LOCK(:key)'); + $releaseStmt->bindValue(':key', $sessionId, \PDO::PARAM_STR); + + return $releaseStmt; + case 'pgsql': + // Obtaining an exclusive session level advisory lock requires an integer key. + // So we convert the HEX representation of the session id to an integer. + // Since integers are signed, we have to skip one hex char to fit in the range. + if (4 === PHP_INT_SIZE) { + $sessionInt1 = hexdec(substr($sessionId, 0, 7)); + $sessionInt2 = hexdec(substr($sessionId, 7, 7)); + + $stmt = $this->pdo->prepare('SELECT pg_advisory_lock(:key1, :key2)'); + $stmt->bindValue(':key1', $sessionInt1, \PDO::PARAM_INT); + $stmt->bindValue(':key2', $sessionInt2, \PDO::PARAM_INT); + $stmt->execute(); + + $releaseStmt = $this->pdo->prepare('SELECT pg_advisory_unlock(:key1, :key2)'); + $releaseStmt->bindValue(':key1', $sessionInt1, \PDO::PARAM_INT); + $releaseStmt->bindValue(':key2', $sessionInt2, \PDO::PARAM_INT); + } else { + $sessionBigInt = hexdec(substr($sessionId, 0, 15)); + + $stmt = $this->pdo->prepare('SELECT pg_advisory_lock(:key)'); + $stmt->bindValue(':key', $sessionBigInt, \PDO::PARAM_INT); + $stmt->execute(); + + $releaseStmt = $this->pdo->prepare('SELECT pg_advisory_unlock(:key)'); + $releaseStmt->bindValue(':key', $sessionBigInt, \PDO::PARAM_INT); + } + + return $releaseStmt; case 'sqlite': - return; // we already locked when starting transaction + throw new \DomainException('SQLite does not support advisory locks.'); default: - throw new \DomainException(sprintf('"%s" does not currently support PDO driver "%s".', __CLASS__, $this->driver)); + throw new \DomainException(sprintf('Advisory locks are currently not implemented for PDO driver "%s".', $this->driver)); + } + } + + /** + * Return a locking or nonlocking SQL query to read session information. + * + * @return string The SQL string + * + * @throws \DomainException When an unsupported PDO driver is used + */ + private function getSelectSql() + { + if (self::LOCK_TRANSACTIONAL === $this->lockMode) { + $this->beginTransaction(); + + switch ($this->driver) { + case 'mysql': + case 'oci': + case 'pgsql': + return "SELECT $this->dataCol, $this->lifetimeCol, $this->timeCol FROM $this->table WHERE $this->idCol = :id FOR UPDATE"; + case 'sqlsrv': + return "SELECT $this->dataCol, $this->lifetimeCol, $this->timeCol FROM $this->table WITH (UPDLOCK, ROWLOCK) WHERE $this->idCol = :id"; + case 'sqlite': + // we already locked when starting transaction + break; + default: + throw new \DomainException(sprintf('Transactional locks are currently not implemented for PDO driver "%s".', $this->driver)); + } } - // We create a DML lock for the session by inserting empty data or updating the row. - // This is safer than an application level advisory lock because it also prevents concurrent modification - // of the session from other parts of the application. - $stmt = $this->pdo->prepare($sql); - $stmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); - $stmt->bindValue(':data', '', \PDO::PARAM_STR); - $stmt->bindValue(':lifetime', 0, \PDO::PARAM_INT); - $stmt->bindValue(':time', time(), \PDO::PARAM_INT); - $stmt->execute(); + return "SELECT $this->dataCol, $this->lifetimeCol, $this->timeCol FROM $this->table WHERE $this->idCol = :id"; } /** - * Returns a merge/upsert (i.e. insert or update) SQL query when supported by the database. + * Returns a merge/upsert (i.e. insert or update) SQL query when supported by the database for writing session data. * * @return string|null The SQL string or null when not supported */ diff --git a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php index 418444766e8c6..e4b8d76778d1c 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/PdoSessionHandlerTest.php @@ -36,7 +36,7 @@ protected function getPersistentSqliteDsn() { $this->dbFile = tempnam(sys_get_temp_dir(), 'sf2_sqlite_sessions'); - return 'sqlite:' . $this->dbFile; + return 'sqlite:'.$this->dbFile; } protected function getMemorySqlitePdo() @@ -120,7 +120,7 @@ public function testWithLazySavePathConnection() public function testReadWriteReadWithNullByte() { - $sessionData = 'da' . "\0" . 'ta'; + $sessionData = 'da'."\0".'ta'; $storage = new PdoSessionHandler($this->getMemorySqlitePdo()); $storage->open('', 'sid');