bug #19567 [Cache] Handle unserialize() failures gracefully (nicolas-…

…grekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Handle unserialize() failures gracefully | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This makes fetching cached items more resilient: `__PHP_Incomplete_Class` "objects" and other errors triggered by unserialize should be turned to cache misses. Commits ------- 47db638 [Cache] Handle unserialize() failures gracefully
symfony · Aug 13, 2016 · 74b8561 · 74b8561
2 parents 8e3967d + 47db638
commit 74b8561
Show file tree

Hide file tree

Showing 8 changed files with 144 additions and 23 deletions.
diff --git a/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php b/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
@@ -350,6 +350,33 @@ public function __destruct()
         }
     }
 
+    /**
+     * Like the native unserialize() function but throws an exception if anything goes wrong.
+     *
+     * @param string $value
+     *
+     * @return mixed
+     *
+     * @throws \Exception
+     */
+    protected static function unserialize($value)
+    {
+        if ('b:0;' === $value) {
+            return false;
+        }
+        $unserializeCallbackHandler = ini_set('unserialize_callback_func', __CLASS__.'::handleUnserializeCallback');
+        try {
+            if (false !== $value = unserialize($value)) {
+                return $value;
+            }
+            throw new \DomainException('Failed to unserialize cached value');
+        } catch (\Error $e) {
+            throw new \ErrorException($e->getMessage(), $e->getCode(), E_ERROR, $e->getFile(), $e->getLine());
+        } finally {
+            ini_set('unserialize_callback_func', $unserializeCallbackHandler);
+        }
+    }
+
     private function getId($key)
     {
         CacheItem::validateKey($key);
@@ -361,13 +388,26 @@ private function generateItems($items, &$keys)
     {
         $f = $this->createCacheItem;
 
-        foreach ($items as $id => $value) {
-            yield $keys[$id] => $f($keys[$id], $value, true);
-            unset($keys[$id]);
+        try {
+            foreach ($items as $id => $value) {
+                $key = $keys[$id];
+                unset($keys[$id]);
+                yield $key => $f($key, $value, true);
+            }
+        } catch (\Exception $e) {
+            CacheItem::log($this->logger, 'Failed to fetch requested items', array('keys' => array_values($keys), 'exception' => $e));
         }
 
         foreach ($keys as $key) {
             yield $key => $f($key, null, false);
         }
     }
+
+    /**
+     * @internal
+     */
+    public static function handleUnserializeCallback($class)
+    {
+        throw new \DomainException('Class not found: '.$class);
+    }
 }
diff --git a/src/Symfony/Component/Cache/Adapter/ApcuAdapter.php b/src/Symfony/Component/Cache/Adapter/ApcuAdapter.php
@@ -49,7 +49,11 @@ public function __construct($namespace = '', $defaultLifetime = 0, $version = nu
      */
     protected function doFetch(array $ids)
     {
-        return apcu_fetch($ids);
+        try {
+            return apcu_fetch($ids);
+        } catch (\Error $e) {
+            throw new \ErrorException($e->getMessage(), $e->getCode(), E_ERROR, $e->getFile(), $e->getLine());
+        }
     }
 
     /**

diff --git a/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php b/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php
@@ -55,12 +55,22 @@ function ($key, $value, $isHit) use ($defaultLifetime) {
      */
     public function getItem($key)
     {
-        if (!$isHit = $this->hasItem($key)) {
+        $isHit = $this->hasItem($key);
+        try {
+            if (!$isHit) {
+                $value = null;
+            } elseif (!$this->storeSerialized) {
+                $value = $this->values[$key];
+            } elseif ('b:0;' === $value = $this->values[$key]) {
+                $value = false;
+            } elseif (false === $value = unserialize($value)) {
+                $value = null;
+                $isHit = false;
+            }
+        } catch (\Exception $e) {
+            CacheItem::log($this->logger, 'Failed to unserialize key "{key}"', array('key' => $key, 'exception' => $e));
             $value = null;
-        } elseif ($this->storeSerialized) {
-            $value = unserialize($this->values[$key]);
-        } else {
-            $value = $this->values[$key];
+            $isHit = false;
         }
         $f = $this->createCacheItem;
 
@@ -181,16 +191,30 @@ private function generateItems(array $keys, $now)
     {
         $f = $this->createCacheItem;
 
-        foreach ($keys as $key) {
-            if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= $now || !$this->deleteItem($key))) {
+        foreach ($keys as $i => $key) {
+            try {
+                if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= $now || !$this->deleteItem($key))) {
+                    $value = null;
+                } elseif (!$this->storeSerialized) {
+                    $value = $this->values[$key];
+                } elseif ('b:0;' === $value = $this->values[$key]) {
+                    $value = false;
+                } elseif (false === $value = unserialize($value)) {
+                    $value = null;
+                    $isHit = false;
+                }
+            } catch (\Exception $e) {
+                CacheItem::log($this->logger, 'Failed to unserialize key "{key}"', array('key' => $key, 'exception' => $e));
                 $value = null;
-            } elseif ($this->storeSerialized) {
-                $value = unserialize($this->values[$key]);
-            } else {
-                $value = $this->values[$key];
+                $isHit = false;
             }
+            unset($keys[$i]);
 
             yield $key => $f($key, $value, $isHit);
         }
+
+        foreach ($keys as $key) {
+            yield $key => $f($key, null, false);
+        }
     }
 }
diff --git a/src/Symfony/Component/Cache/Adapter/DoctrineAdapter.php b/src/Symfony/Component/Cache/Adapter/DoctrineAdapter.php
@@ -32,7 +32,25 @@ public function __construct(CacheProvider $provider, $namespace = '', $defaultLi
      */
     protected function doFetch(array $ids)
     {
-        return $this->provider->fetchMultiple($ids);
+        $unserializeCallbackHandler = ini_set('unserialize_callback_func', parent::class.'::handleUnserializeCallback');
+        try {
+            return $this->provider->fetchMultiple($ids);
+        } catch (\Error $e) {
+            $trace = $e->getTrace();
+
+            if (isset($trace[0]['function']) && !isset($trace[0]['class'])) {
+                switch ($trace[0]['function']) {
+                    case 'unserialize':
+                    case 'apcu_fetch':
+                    case 'apc_fetch':
+                        throw new \ErrorException($e->getMessage(), $e->getCode(), E_ERROR, $e->getFile(), $e->getLine());
+                }
+            }
+
+            throw $e;
+        } finally {
+            ini_set('unserialize_callback_func', $unserializeCallbackHandler);
+        }
     }
 
     /**

diff --git a/src/Symfony/Component/Cache/Adapter/FilesystemAdapter.php b/src/Symfony/Component/Cache/Adapter/FilesystemAdapter.php
@@ -60,7 +60,7 @@ protected function doFetch(array $ids)
 
         foreach ($ids as $id) {
             $file = $this->getFile($id);
-            if (!$h = @fopen($file, 'rb')) {
+            if (!file_exists($file) || !$h = @fopen($file, 'rb')) {
                 continue;
             }
             if ($now >= (int) $expiresAt = fgets($h)) {
@@ -73,7 +73,7 @@ protected function doFetch(array $ids)
                 $value = stream_get_contents($h);
                 fclose($h);
                 if ($i === $id) {
-                    $values[$id] = unserialize($value);
+                    $values[$id] = parent::unserialize($value);
                 }
             }
         }

diff --git a/src/Symfony/Component/Cache/Adapter/RedisAdapter.php b/src/Symfony/Component/Cache/Adapter/RedisAdapter.php
@@ -134,19 +134,15 @@ public static function createConnection($dsn, array $options = array())
      */
     protected function doFetch(array $ids)
     {
-        $result = array();
-
         if ($ids) {
             $values = $this->redis->mGet($ids);
             $index = 0;
             foreach ($ids as $id) {
                 if ($value = $values[$index++]) {
-                    $result[$id] = unserialize($value);
+                    yield $id => parent::unserialize($value);
                 }
             }
         }
-
-        return $result;
     }
 
     /**

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/AdapterTestCase.php b/src/Symfony/Component/Cache/Tests/Adapter/AdapterTestCase.php
@@ -46,4 +46,42 @@ public function testDefaultLifeTime()
         $item = $cache->getItem('key.dlt');
         $this->assertFalse($item->isHit());
     }
+
+    public function testNotUnserializable()
+    {
+        if (isset($this->skippedTests[__FUNCTION__])) {
+            $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
+
+            return;
+        }
+
+        $cache = $this->createCachePool();
+
+        $item = $cache->getItem('foo');
+        $cache->save($item->set(new NotUnserializable()));
+
+        $item = $cache->getItem('foo');
+        $this->assertFalse($item->isHit());
+
+        foreach ($cache->getItems(array('foo')) as $item) {
+        }
+        $cache->save($item->set(new NotUnserializable()));
+
+        foreach ($cache->getItems(array('foo')) as $item) {
+        }
+        $this->assertFalse($item->isHit());
+    }
+}
+
+class NotUnserializable implements \Serializable
+{
+    public function serialize()
+    {
+        return serialize(123);
+    }
+
+    public function unserialize($ser)
+    {
+        throw new \Exception(__CLASS__);
+    }
 }
diff --git a/src/Symfony/Component/Cache/Tests/Adapter/DoctrineAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/DoctrineAdapterTest.php
@@ -22,6 +22,7 @@ class DoctrineAdapterTest extends AdapterTestCase
     protected $skippedTests = array(
         'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayCache is not.',
         'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayCache is not.',
+        'testNotUnserializable' => 'ArrayCache does not use serialize/unserialize',
     );
 
     public function createCachePool($defaultLifetime = 0)