Problem/Motivation

It's currently pretty easy to break a request to acquire a lock with a long name when using the DatabaseLockBackend class. The database column has a max length of 255 but nothing at the application level enforces this. The database cache backend guards against this, the lock should too.

Proposed resolution

Do the same as the DatabaseBackend cache class, use a normalizeCid() method in the Lock backend itself so calls with a longer cid cannot break it.

Remaining tasks

Decide if we should:

- Move the normalizeCid() functionality into a trait or something, so we can reuse it between cache backend and lock backend. Something in the database namespace could be useful.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#10 interdiff-2872276-10.txt1.78 KBdamiankloip
#10 2872276-10.patch3.72 KBdamiankloip
#2 2872276-tests-only-FAIL.patch1.38 KBdamiankloip
#2 2872276.patch3.71 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip created an issue. See original summary.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2872276.patch3.71 KB
2872276-tests-only-FAIL.patch1.38 KB

EDIT: Yes - should have put the patch files the other way round.

Status: Needs review » Needs work

The last submitted patch, 2: 2872276-tests-only-FAIL.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

- Move the normalizeCid() functionality into a trait or something, so we can reuse it between cache backend and lock backend. Something in the database namespace could be useful.

IMHO sharing code is not a requirement here. Sharing code between arbitrary subsystems can lead to more harm than good on the longrun. Maybe its enough to have a @see there and call it a day.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
  1. +++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -204,6 +211,30 @@ protected function catchException(\Exception $e) {
+   * @param string $cid
+   *   The passed in cache ID.
...
+   * @return string
+   *   An ASCII-encoded cache ID that is at most 255 characters long.
...
+  protected function normalizeName($cid) {

    Lier, this is not a cache id :) 

  2. +++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -204,6 +211,30 @@ protected function catchException(\Exception $e) {
+    $cid_is_ascii = mb_check_encoding($cid, 'ASCII');

    It is quite cool that we can use mb_ without worrying about it.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented

Yeah, that's a good point. I am fine with not coupling the two systems together with a shared method somewhere.

The mb_ usage is the same in the database backend. Do we require that PHP is compiled with mbstring or the extension is installed?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

The mb_ usage is the same in the database backend. Do we require that PHP is compiled with mbstring or the extension is installed?

Oh we have a wrapper in place in case its not actually available. This is some symfony package.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented

sweet deal. To be fair, I also prefixed it with 'decide if we should' :P

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented
Issue summary: View changes
FileSize
2872276-10.patch3.72 KB
interdiff-2872276-10.txt1.78 KB

So that means we are done here?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip at Pfizer, Inc. commented
Title: Database lock backend should normalize cache IDs » Database lock backend should normalize the lock name
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

I think personally we are good here. We discussed that sharing this code with the caching backend doesn't make much sense.

  • catch committed f156c3d on 8.4.x 
    Issue #2872276 by damiankloip, dawehner: Database lock backend should...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented 
+++ b/core/tests/Drupal/KernelTests/Core/Lock/LockTest.php
@@ -47,6 +47,21 @@ public function testBackendLockRelease() {
+    // Test acquiring an releasing a lock with a long key (over 255 chars).

Huge nitpick: I'm just noting there's a typo here. It's definitely not worth reverting but if someone can just fix it in a commit against this issue, it is good enough.