Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 15, 2018

Conversation

Projects
None yet
5 participants
@oleg-andreyev
Copy link
Contributor

commented May 13, 2018

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:

@oleg-andreyev 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

@@ -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);

This comment has been minimized.

Copy link
@fabpot

fabpot May 14, 2018

Member

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.

This comment has been minimized.

Copy link
@Tobion

Tobion May 14, 2018

Member

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.

This comment has been minimized.

Copy link
@oleg-andreyev

oleg-andreyev May 14, 2018

Author Contributor

@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

@@ -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 comment has been minimized.

Copy link
@Tobion

Tobion May 14, 2018

Member

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

This comment has been minimized.

Copy link
@oleg-andreyev

oleg-andreyev May 14, 2018

Author Contributor

Will rebase branch

@Tobion

This comment has been minimized.

Copy link
Member

commented May 14, 2018

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

@oleg-andreyev

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@Tobion agree, let's rebase on 2.7

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

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

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

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

@oleg-andreyev oleg-andreyev changed the base branch from master to 2.7 May 14, 2018

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

@Tobion

Tobion approved these changes May 14, 2018

@fabpot

fabpot approved these changes May 15, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented May 15, 2018

Thank you @oleg-andreyev.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 15, 2018

bug #27250 [Session] limiting :key for GET_LOCK to 64 chars (oleg-and…
…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

nicolas-grekas added a commit that referenced this pull request May 15, 2018

Merge branch '2.7' into 2.8
* 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

nicolas-grekas added a commit that referenced this pull request May 16, 2018

Merge branch '2.8' into 3.4
* 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

nicolas-grekas added a commit that referenced this pull request May 16, 2018

Merge branch '3.4' into 4.0
* 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

nicolas-grekas added a commit that referenced this pull request May 16, 2018

Merge branch '4.0' into 4.1
* 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
  ...

nicolas-grekas added a commit that referenced this pull request May 16, 2018

Merge branch '4.1'
* 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
  ...

This was referenced May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.