Database backends cannot deal with corrupt serialized data: app completey broken

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.

+++ 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.

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

Fixes #8

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!

#10 done

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.

Fixes #14

Thanks!

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 ''?

Fixes #17

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 ...

+++ 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.

I am currently dealing with a site with a corrupt serialized value for the system.theme.data state. The site in question seems to run fine in production, but setting up in a VM for local development runs into this error, so there may be a local environment difference triggering this. But still, it seems like serialization/unserialization should be pretty straightforward.

It seems to happen with a long set of data, maybe it's trying to unserialize a multi-byte string? https://stackoverflow.com/questions/21072727/php-unserialize-strange-iss...

Follow up from my last comment. It seemed that I was pulling a production database that was using a charset and collation for multi-byte strings. The live site is on Acquia, which uses these database settings: https://docs.acquia.com/acquia-cloud/manage/database/utf8mb4/.

So pulling the database to my local site, some serialized settings seemed to contain characters that are inappropraite for the normal utf8/latin1 encoding. When PHP tries to unserialize these stings, it just ends up with a false value.

Re-Rolled The patch For #18 As per the #35 Suggestion .Also add the Inter Diff For The #18 to #37.

Should be MR.

DId not review

MR still appears to have failures.

Fixed #41 , and created MR. Kindly review

MR appears to have failures.