Problem/Motivation

Some cache backends including the database implementation rely on serialization to store cached data:

    // Unserialize and return the cached data.
    if ($cache->serialized) {
      $cache->data = unserialize($cache->data);
    }

I had this situation where the container definition (that is by default stored in the database cache) could not be unserialized (there are several reasons that could lead to such a situation. I.e. a change in the environment/runtime can make something that could be previously unserialized not unserializable anymore).

In such a situation, the cache backend FAILS to provide reliable information, because it will return the stored data as FALSE (that is a completely valid cache data value).

The FALSE value propagates leading to a completely dead Drupal application:

[25-Apr-2017 15:13:32 Europe/Madrid] TypeError: Argument 1 passed to Drupal\Component\DependencyInjection\Container::__construct() must be of the type array, boolean given, called in D:\_Webs\chf_envivoui_FZd\app\web\core\lib\Drupal\Core\DrupalKernel.php on line 883 in D:\_Webs\chf_envivoui_FZd\app\web\core\lib\Drupal\Component\DependencyInjection\Container.php on line 119 #0 D:\_Webs\chf_envivoui_FZd\app\web\core\lib\Drupal\Core\DrupalKernel.php(883): Drupal\Component\DependencyInjection\Container->__construct(false)
#1 D:\_Webs\chf_envivoui_FZd\app\web\core\lib\Drupal\Core\DrupalKernel.php(461): Drupal\Core\DrupalKernel->initializeContainer()
#2 D:\_Webs\chf_envivoui_FZd\app\web\core\lib\Drupal\Core\DrupalKernel.php(651): Drupal\Core\DrupalKernel->boot()
#3 D:\_Webs\chf_envivoui_FZd\app\web\index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#4 {main}

Of course, in such a situation, a container rebuild will save they day. But I can imagine many other places where a corrupt serialized cache item could render the system unusable, and there would be no other solution but to completely wipe all caches - and there is no warranty that Drupal itself will be able to wipe the caches using regular mechanisms as it could be broken as in the previous situation - so you will need to do this in a more manual way.

Proposed resolution

When retrieving cache items using DatabaseCacheBackend, make sure that corrupt items are properly identified and treated as cache misses.

Currently broken items are treated as cache hits that return a FALSE as the cached data.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

david_garcia created an issue. See original summary.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

izaaksom’s picture

Hi, I'm really glad (?) that I'm not the only one having issues with this. There should be a normalized way in Drupal to handle caches and verify that when we return FALSE we are really a getting a FALSE and not corrupt data . Temporally I add a possible fix to this.

david_garcia’s picture

Issue summary: View changes
Status: Active » Reviewed & tested by the community

Excelent patch! Not the most elegant approach, but does the job in a concise and cost-effective manner.

Time to update the issue summary.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -135,7 +135,13 @@ protected function prepareItem($cache, $allow_invalid) {
-      $cache->data = unserialize($cache->data);
+      $unserialized = unserialize($cache->data);
+      if($unserialized === FALSE) {
+        if($cache->data != serialize(FALSE)){
+          return FALSE;
+        }
+      }
+      $cache->data = $unserialized;

This could use some test coverage - for example inserting some data that can't be unserialized into a cache table directly, then retrieving it via the API.

Also there's some minor code style issues, like missing spaces before parenthesis/brackets. Could use a short explanatory comment as to why we're doing this as well.

unserialize() will issue an E_NOTICE when it can't unserialize data, so I think just the comparison above is fine and patch makes sense to me overall.

izaaksom’s picture

Here is another patch that solves the code-style issues mentioned by catch on #5

david_garcia’s picture

Status: Needs work » Reviewed & tested by the community

Style issues seem to be fixed.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
izaaksom’s picture

Wim Leers’s picture

Issue tags: -Needs tests

Thanks!

  1. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -29,6 +30,16 @@ protected function createCacheBackend($bin) {
    +   * Gets the cache tag validator.
    

    Comment is wrong.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -48,4 +59,29 @@ public function testSetGet() {
    +   * Tests getting a FALSE when requesting the corrupt object.
    

    s/the corrupt object/a corrupt cache item/

  3. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -48,4 +59,29 @@ public function testSetGet() {
    +    // We insert a corrupted object into the cache table.
    

    s/object/cache item/

Please also post a failing test-only patch, so that we know that the new test coverage is indeed reproducing the original problem :)

(Post both a failing test-only patch and the patch in #9 in the same comment. Then it'll be very clear what patch you want a committer to commit.)

After that, this is RTBC!

The last submitted patch, 11: test-only.patch, failed testing. View results

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Thanks!

Sorry, one last round of nits (which I think a committer would also raise):

  1. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -29,6 +30,16 @@ protected function createCacheBackend($bin) {
    +   * Gets a CacheTagsChecksum tag validator.
    ...
    +   *   A CacheTagChecksum tag validator.
    

    s/a/the/

  2. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -29,6 +30,16 @@ protected function createCacheBackend($bin) {
    +  protected function getCacheTagsChecksum() {
    

    The method name is missing the "validator" bit.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -48,4 +59,29 @@ public function testSetGet() {
    +    $cache_tags_checksum = $this->getCacheTagsChecksum();
    

    This is the only place this is being used. This is a test. Let's just inline the $this->container->get(…) call.

izaaksom’s picture

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Thanks!

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

An observation and a nit

  1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
    @@ -135,7 +135,16 @@ protected function prepareItem($cache, $allow_invalid) {
    +      if ($unserialized === FALSE) {
    +        if ($cache->data != serialize(FALSE)) {
    

    We can combine these into a single if statement

    if ($unserialized === FALSE && $cache->data !== serialize(FALSE)) {
    
  2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
    @@ -135,7 +135,16 @@ protected function prepareItem($cache, $allow_invalid) {
    +        if ($cache->data != serialize(FALSE)) {
    

    nit - because they're both strings - we should use !== here

  3. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -48,4 +49,30 @@ public function testSetGet() {
    +  public function testCorruptCacheReturnsFalse() {
    

    Nice work here

  4. +++ b/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php
    @@ -48,4 +49,30 @@ public function testSetGet() {
    +      'tags' => implode(' ', []),
    

    nit: Can't we just use ''?

dawehner’s picture

There are many other places where we might have issues with serialize and we should have a look at? It might be worth looking at some followups ...

index 747186b9b7..c55a02710f 100644
--- a/core/lib/Drupal/Core/Cache/DatabaseBackend.php

--- a/core/lib/Drupal/Core/Cache/DatabaseBackend.php
+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -163,7 +163,14 @@ protected function prepareItem($cache, $allow_invalid) {

In an ideal world we would document on CacheBackendInterface that each backend ideally should have such a code ...

david_garcia’s picture

Status: Needs review » Reviewed & tested by the community
david_garcia’s picture

It might be worth looking at some followups ...

Actually, every single call to unserialize() in Drupal's codebase should be revised to properly consider unserialization failure. I'd rather have a custom unserialize() that throws an exception upon failure, than having an "FALSE" being propagated as a successful result of unserialization.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -163,7 +163,14 @@ protected function prepareItem($cache, $allow_invalid) {
+      $unserialized = @unserialize($cache->data);

We shouldn't suppress the error from unserialize() with @ - that notice might be useful (for example if somehow a cache entry was always corrupted so never got successfully returned). So I'd remove that and just fix the return here.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.