From ecf80b47a05ca23eacb75d721780cc5b6b2d0e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20Stamenkovi=C4=87?= Date: Tue, 23 Jul 2019 16:18:16 +0200 Subject: [PATCH 1/6] Call to a member function resolveAssociationEntries() on boolean The following mistakes occur occasionally: ``` Call to a member function resolveAssociationEntries() on boolean {"detail":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Call to a member function resolveAssociationEntries() on boolean at /www/vendor/doctrine/orm/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:140)"} ``` On cache miss the parameter `$entityEntry` sometimes will be false. This fixes issue #7266. --- lib/Doctrine/ORM/Cache/DefaultQueryCache.php | 2 +- .../Tests/ORM/Cache/DefaultQueryCacheTest.php | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php index d431313ef0..115b211eef 100644 --- a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php +++ b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php @@ -124,7 +124,7 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = [] foreach ($cacheEntry->result as $index => $entry) { $entityEntry = is_array($entries) && array_key_exists($index, $entries) ? $entries[$index] : null; - if ($entityEntry === null) { + if (! $entityEntry instanceof EntityCacheEntry) { if ($this->cacheLogger !== null) { $this->cacheLogger->entityCacheMiss($regionName, $cacheKeys->identifiers[$index]); } diff --git a/tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php index 58eddd3750..ecfcc5207f 100644 --- 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 = []; From eafc4c5a0c07f8cf476fba63444fee9e79544df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 13 Aug 2019 22:20:59 +0200 Subject: [PATCH 2/6] Remove unnecessary parentheses --- lib/Doctrine/ORM/Cache/DefaultQueryCache.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php index 115b211eef..25fa50cc90 100644 --- a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php +++ b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php @@ -106,7 +106,7 @@ 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(); @@ -267,7 +267,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); } From b9d683421322f0a359e536429eacbe5d2e8145ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 13 Aug 2019 22:21:59 +0200 Subject: [PATCH 3/6] Remove unnecessary function calls --- lib/Doctrine/ORM/Cache/DefaultQueryCache.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php index 25fa50cc90..dfa66c193f 100644 --- a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php +++ b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php @@ -118,11 +118,11 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = [] }; $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 instanceof EntityCacheEntry) { if ($this->cacheLogger !== null) { From b6663733c01e0c7323d31152bf5861f096a837e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 13 Aug 2019 22:22:44 +0200 Subject: [PATCH 4/6] Add type assertion to be more strict about persister type --- lib/Doctrine/ORM/Cache/DefaultQueryCache.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php index dfa66c193f..26f00d1faa 100644 --- a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php +++ b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php @@ -22,6 +22,7 @@ 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; @@ -29,6 +30,7 @@ use Doctrine\Common\Proxy\Proxy; use Doctrine\ORM\Cache; use Doctrine\ORM\Query; +use function assert; /** * Default query cache implementation. @@ -108,8 +110,10 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = [] $entityName = reset($rsm->aliasMap); $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); @@ -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) { From 3577064f8c97dffbe2db9ccd9e8457c8e57a7975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 13 Aug 2019 22:23:37 +0200 Subject: [PATCH 5/6] Make closure static To adhere to our coding standard. --- lib/Doctrine/ORM/Cache/DefaultQueryCache.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php index 26f00d1faa..866ea0db25 100644 --- a/lib/Doctrine/ORM/Cache/DefaultQueryCache.php +++ b/lib/Doctrine/ORM/Cache/DefaultQueryCache.php @@ -117,7 +117,7 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = [] $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']); }; From 80503c483783f94a811e64e4ad1d8bfd6b10b94e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 13 Aug 2019 22:25:05 +0200 Subject: [PATCH 6/6] Guard cache regions against corrupted data For some bizarre reason the underlying cache drivers are returning unexpected values, which are leaking to the cache objects and causing them to error. This makes our cache regions much more strict about the types that are fetched from the cache provider, ensuring that no invalid information is ever sent to the hydrators. --- .../Cache/Region/DefaultMultiGetRegion.php | 6 +++++ .../ORM/Cache/Region/DefaultRegion.php | 10 +++++-- .../Tests/ORM/Cache/DefaultRegionTest.php | 26 +++++++++++++++++++ .../Tests/ORM/Cache/MultiGetRegionTest.php | 13 ++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php index 2cd94292b4..7b5f28ecf3 100644 --- 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 index be62a6fb95..ab069cefb5 100644 --- 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/DefaultRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php index 97365dfcf2..e50c2a55c3 100644 --- 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 index 0d98665d0d..5f45de4714 100644 --- 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]))); + } }