[Session] limiting :key for GET_LOCK to 64 chars #27250

oleg-andreyev · 2018-05-13T14:14:29Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT

MySQL 5.7.5 and later enforces a maximum length on lock names of 64 characters. Previously, no limit was enforced.

Cases:

session_id is set by developers manually
session.sid_length is configured

Ref.:

Other issues:

fabpot · 2018-05-14T06:42:01Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -674,14 +674,17 @@ private function doAdvisoryLock(string $sessionId)
    {
        switch ($this->driver) {
            case 'mysql':
+                // MySQL 5.7.5 and later enforces a maximum length on lock names of 64 characters. Previously, no limit was enforced.
+                $lockId = \substr($sessionId, 0, 64);


I'd recommend to hash the session id instead (or doing something similar) as keeping just the first 64 chars could lead to conflicts depending on how the session id is generated.

The session id is just a random string. So hashing that does not reduce the likeliness of collisions compared to just using the first 64 bytes.

@fabian, @Tobion is right, session_id is just a random string, which is result of http://php.net/manual/en/session.configuration.php#ini.session.hash-function and by default it's md5

Tobion · 2018-05-14T14:05:51Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php

@@ -833,7 +836,7 @@ private function getUpdateStatement($sessionId, $sessionData, $maxlifetime)
    /**
     * Returns a merge/upsert (i.e. insert or update) statement when supported by the database for writing session data.
     */
-    private function getMergeStatement(string $sessionId, string $data, int$maxlifetime): ?\PDOStatement
+    private function getMergeStatement(string $sessionId, string $data, int $maxlifetime): ?\PDOStatement


This has already been fixed in 4f3afd5 and will be merge upwards in later branches automatically. So please remove this change.

Will rebase branch

Tobion · 2018-05-14T14:07:56Z

To me this is a bugfix and should be opened against 2.7 branch.

oleg-andreyev · 2018-05-14T17:17:53Z

@Tobion agree, let's rebase on 2.7

…ySQL 5.7.5 and later

fabpot · 2018-05-15T04:51:19Z

Thank you @oleg-andreyev.

…reyev) This PR was merged into the 2.7 branch. Discussion ---------- [Session] limiting :key for GET_LOCK to 64 chars | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT > MySQL 5.7.5 and later enforces a maximum length on lock names of 64 characters. Previously, no limit was enforced. Cases: - `session_id` is set by developers manually - `session.sid_length` is configured Ref.: - https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock - http://php.net/manual/en/session.configuration.php#ini.session.sid-length Other issues: - go-sql-driver/mysql#385 - stefangabos/Zebra_Session#16 Commits ------- 9cda96b #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later

* 2.7: [Security] Fix logout #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later [Profiler] Remove propel & event_listener_loading category identifiers [Filesystem] Fix usages of error_get_last() [Debug] Fix populating error_get_last() for handled silent errors Suppress warnings when open_basedir is non-empty

* 2.8: [Security] Fix logout #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later [Profiler] Remove propel & event_listener_loading category identifiers [Filesystem] Fix usages of error_get_last() [Debug] Fix populating error_get_last() for handled silent errors Suppress warnings when open_basedir is non-empty

* 3.4: fix merge [Security] Fix logout Cleanup 2 tests for the HttpException classes #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later [Config] Fix tests when path contains UTF chars [DI] Shared services should not be inlined in non-shared ones [Profiler] Remove propel & event_listener_loading category identifiers [Filesystem] Fix usages of error_get_last() [Cache][Lock] Fix usages of error_get_last() [Debug] Fix populating error_get_last() for handled silent errors [DI] Display previous error messages when throwing unused bindings Suppress warnings when open_basedir is non-empty

* 4.0: (21 commits) [PropertyInfo] fix resolving parent|self type hints fixed CS fix merge [Security] Fix logout Cleanup 2 tests for the HttpException classes #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later [Config] Fix tests when path contains UTF chars [DI] Shared services should not be inlined in non-shared ones [Profiler] Remove propel & event_listener_loading category identifiers [Filesystem] Fix usages of error_get_last() [Cache][Lock] Fix usages of error_get_last() [Debug] Fix populating error_get_last() for handled silent errors fixed CS fixed CS fixed CS [FrameworkBundle] Fix cache:clear on vagrant [HttpKernel] Handle NoConfigurationException "onKernelException()" Fix misses calculation when calling getItems [DI] Display previous error messages when throwing unused bindings Fixed return type ...

* 4.1: (22 commits) Fix CS [PropertyInfo] fix resolving parent|self type hints fixed CS fix merge [Security] Fix logout Cleanup 2 tests for the HttpException classes #27250 limiting GET_LOCK key up to 64 char due to changes in MySQL 5.7.5 and later [Config] Fix tests when path contains UTF chars [DI] Shared services should not be inlined in non-shared ones [Profiler] Remove propel & event_listener_loading category identifiers [Filesystem] Fix usages of error_get_last() [Cache][Lock] Fix usages of error_get_last() [Debug] Fix populating error_get_last() for handled silent errors fixed CS fixed CS fixed CS [FrameworkBundle] Fix cache:clear on vagrant [HttpKernel] Handle NoConfigurationException "onKernelException()" Fix misses calculation when calling getItems [DI] Display previous error messages when throwing unused bindings ...

carsonbot added the Status: Needs Review label May 13, 2018

oleg-andreyev changed the title ~~limiting :key for GET_LOCK to 64 chars~~ [Session] limiting :key for GET_LOCK to 64 chars May 13, 2018

carsonbot removed the Status: Needs Review label May 14, 2018

fabpot requested changes May 14, 2018

View reviewed changes

carsonbot added the Status: Needs Work label May 14, 2018

Tobion reviewed May 14, 2018

View reviewed changes

oleg-andreyev force-pushed the limiting-get-lock-to-64 branch from 1555e16 to 4e057b8 Compare May 14, 2018 17:21

oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 14, 2018

symfony#27250 limiting GET_LOCK key up to 64 char due to changes in M…

4e057b8

…ySQL 5.7.5 and later

oleg-andreyev force-pushed the limiting-get-lock-to-64 branch from 4e057b8 to d7d4e41 Compare May 14, 2018 17:26

oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 14, 2018

symfony#27250 limiting GET_LOCK key up to 64 char due to changes in M…

4c52bc6

…ySQL 5.7.5 and later

oleg-andreyev changed the base branch from master to 2.7 May 14, 2018 17:27

symfony#27250 limiting GET_LOCK key up to 64 char due to changes in M…

9cda96b

…ySQL 5.7.5 and later

oleg-andreyev force-pushed the limiting-get-lock-to-64 branch from 4c52bc6 to 9cda96b Compare May 14, 2018 17:29

carsonbot added Status: Reviewed and removed Status: Needs Work labels May 14, 2018

nicolas-grekas approved these changes May 14, 2018

View reviewed changes

Tobion approved these changes May 14, 2018

View reviewed changes

fabpot approved these changes May 15, 2018

View reviewed changes

fabpot merged commit 9cda96b into symfony:2.7 May 15, 2018

This was referenced May 21, 2018

Release v2.7.47 #27327

Merged

Release v2.8.40 #27328

Merged

This was referenced May 21, 2018

Release v3.4.10 #27329

Merged

Release v4.0.10 #27330

Merged

Release v4.1.0-BETA2 #27331

Merged

rob006 mentioned this pull request Oct 27, 2018

Fix MySQL lock name length limit yiisoft/yii2#16836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Session] limiting :key for GET_LOCK to 64 chars #27250

[Session] limiting :key for GET_LOCK to 64 chars #27250

oleg-andreyev commented May 13, 2018 •

edited

Loading

fabpot May 14, 2018

Tobion May 14, 2018

oleg-andreyev May 14, 2018

Tobion May 14, 2018 •

edited

Loading

oleg-andreyev May 14, 2018

Tobion commented May 14, 2018

oleg-andreyev commented May 14, 2018

fabpot commented May 15, 2018

[Session] limiting :key for GET_LOCK to 64 chars #27250

[Session] limiting :key for GET_LOCK to 64 chars #27250

Conversation

oleg-andreyev commented May 13, 2018 • edited Loading

fabpot May 14, 2018

Choose a reason for hiding this comment

Tobion May 14, 2018

Choose a reason for hiding this comment

oleg-andreyev May 14, 2018

Choose a reason for hiding this comment

Tobion May 14, 2018 • edited Loading

Choose a reason for hiding this comment

oleg-andreyev May 14, 2018

Choose a reason for hiding this comment

Tobion commented May 14, 2018

oleg-andreyev commented May 14, 2018

fabpot commented May 15, 2018

oleg-andreyev commented May 13, 2018 •

edited

Loading

Tobion May 14, 2018 •

edited

Loading