Merge pull request #7778 from umpirsky/fix/issue-7266

Guard L2C regions against corrupted data
doctrine · Aug 14, 2019 · 642e543 · 642e543
2 parents 5499555 + 80503c4
commit 642e543
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 13 deletions.
diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php
@@ -22,13 +22,15 @@
 
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\ORM\Cache\Persister\CachedPersister;
+use Doctrine\ORM\Cache\Persister\Entity\CachedEntityPersister;
 use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\Query\ResultSetMapping;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\PersistentCollection;
 use Doctrine\Common\Proxy\Proxy;
 use Doctrine\ORM\Cache;
 use Doctrine\ORM\Query;
+use function assert;
 
 /**
  * Default query cache implementation.
@@ -106,25 +108,27 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = []
 
         $result      = [];
         $entityName  = reset($rsm->aliasMap);
-        $hasRelation = ( ! empty($rsm->relationMap));
+        $hasRelation = ! empty($rsm->relationMap);
         $persister   = $this->uow->getEntityPersister($entityName);
-        $region      = $persister->getCacheRegion();
-        $regionName  = $region->getName();
+        assert($persister instanceof CachedEntityPersister);
+
+        $region     = $persister->getCacheRegion();
+        $regionName = $region->getName();
 
         $cm = $this->em->getClassMetadata($entityName);
 
-        $generateKeys = function (array $entry) use ($cm): EntityCacheKey {
+        $generateKeys = static function (array $entry) use ($cm) : EntityCacheKey {
             return new EntityCacheKey($cm->rootEntityName, $entry['identifier']);
         };
 
         $cacheKeys = new CollectionCacheEntry(array_map($generateKeys, $cacheEntry->result));
-        $entries   = $region->getMultiple($cacheKeys);
+        $entries   = $region->getMultiple($cacheKeys) ?? [];
 
         // @TODO - move to cache hydration component
         foreach ($cacheEntry->result as $index => $entry) {
-            $entityEntry = is_array($entries) && array_key_exists($index, $entries) ? $entries[$index] : null;
+            $entityEntry = $entries[$index] ?? null;
 
-            if ($entityEntry === null) {
+            if (! $entityEntry instanceof EntityCacheEntry) {
                 if ($this->cacheLogger !== null) {
                     $this->cacheLogger->entityCacheMiss($regionName, $cacheKeys->identifiers[$index]);
                 }
@@ -145,9 +149,11 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = []
             $data = $entityEntry->data;
 
             foreach ($entry['associations'] as $name => $assoc) {
-                $assocPersister  = $this->uow->getEntityPersister($assoc['targetEntity']);
-                $assocRegion     = $assocPersister->getCacheRegion();
-                $assocMetadata   = $this->em->getClassMetadata($assoc['targetEntity']);
+                $assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
+                assert($assocPersister instanceof CachedEntityPersister);
+
+                $assocRegion   = $assocPersister->getCacheRegion();
+                $assocMetadata = $this->em->getClassMetadata($assoc['targetEntity']);
 
                 if ($assoc['type'] & ClassMetadata::TO_ONE) {
 
@@ -267,7 +273,7 @@ public function put(QueryCacheKey $key, ResultSetMapping $rsm, $result, array $h
         $rootAlias   = key($rsm->aliasMap);
         $persister   = $this->uow->getEntityPersister($entityName);
 
-        if ( ! ($persister instanceof CachedPersister)) {
+        if (! $persister instanceof CachedEntityPersister) {
             throw CacheException::nonCacheableEntity($entityName);
         }
 

diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php
@@ -21,6 +21,7 @@
 namespace Doctrine\ORM\Cache\Region;
 
 use Doctrine\Common\Cache\MultiGetCache;
+use Doctrine\ORM\Cache\CacheEntry;
 use Doctrine\ORM\Cache\CollectionCacheEntry;
 
 /**
@@ -67,7 +68,12 @@ public function getMultiple(CollectionCacheEntry $collection)
         }
 
         $returnableItems = [];
+
         foreach ($keysToRetrieve as $index => $key) {
+            if (! $items[$key] instanceof CacheEntry) {
+                return null;
+            }
+
             $returnableItems[$index] = $items[$key];
         }
 

diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php
@@ -94,7 +94,13 @@ public function contains(CacheKey $key)
      */
     public function get(CacheKey $key)
     {
-        return $this->cache->fetch($this->getCacheEntryKey($key)) ?: null;
+        $entry = $this->cache->fetch($this->getCacheEntryKey($key));
+
+        if (! $entry instanceof CacheEntry) {
+            return null;
+        }
+
+        return $entry;
     }
 
     /**
@@ -108,7 +114,7 @@ public function getMultiple(CollectionCacheEntry $collection)
             $entryKey   = $this->getCacheEntryKey($key);
             $entryValue = $this->cache->fetch($entryKey);
 
-            if ($entryValue === false) {
+            if (! $entryValue instanceof CacheEntry) {
                 return null;
             }
 

diff --git a/tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php
@@ -354,6 +354,34 @@ public function testGetWithAssociation()
         $this->assertEquals('Bar', $result[1]->getName());
     }
 
+    public function testGetWithAssociationCacheMiss() : void
+    {
+        $rsm   = new ResultSetMappingBuilder($this->em);
+        $key   = new QueryCacheKey('query.key1', 0);
+        $entry = new QueryCacheEntry(
+            [
+                ['identifier' => ['id' => 1]],
+                ['identifier' => ['id' => 2]],
+            ]
+        );
+
+        $this->region->addReturn('get', $entry);
+
+        $this->region->addReturn(
+            'getMultiple',
+            [
+                new EntityCacheEntry(Country::class, ['id' => 1, 'name' => 'Foo']),
+                false,
+            ]
+        );
+
+        $rsm->addRootEntityFromClassMetadata(Country::class, 'c');
+
+        $result = $this->queryCache->get($key, $rsm);
+
+        self::assertNull($result);
+    }
+
     public function testCancelPutResultIfEntityPutFails()
     {
         $result   = [];

diff --git a/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php
@@ -95,6 +95,32 @@ public function testGetMulti()
         $this->assertEquals($value1, $actual[0]);
         $this->assertEquals($value2, $actual[1]);
     }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplicationWhenGettingSingleEntry() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->get($key1));
+    }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplicationWhenGettingMultipleEntries() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->getMultiple(new CollectionCacheEntry([$key1])));
+    }
 }
 
 /**

diff --git a/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php
@@ -39,4 +39,17 @@ public function testGetMulti()
         $this->assertEquals($value1, $actual[0]);
         $this->assertEquals($value2, $actual[1]);
     }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplication() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->getMultiple(new CollectionCacheEntry([$key1])));
+    }
 }