Guard L2C regions against corrupted data

[umpirsky]

Fixes #7266.

[greg0ire]

Please improve your commit message according to the contributing guide.

[umpirsky]

@greg0ire Improved, thanks.

[greg0ire]

Your now exemplary commit message says:

On cache miss the parameter $entityEntry sometimes will be false.

But apparently, false is not supposed to happen at all:

orm/lib/Doctrine/ORM/Cache/MultiGetRegion.php

Line 20 in a896421

* @return CacheEntry[]|null The cached entries or NULL if one or more entries can not be found

IMO the right fix would be to make sure you cannot get false instead of handling it afterwards. Make the implementation conform to what the contract specified in the phpdoc says.

[umpirsky]

I agree, this was a quick way for me to turn error into cache miss. But the root of the problem is in getMultiple() in deed, and I don't see the way to fix that.

[greg0ire]

getMultiple()

Which implementation are we talking about here? Whatever it is, shouldn't we ensure it never returns false as an element of the returned array?

[umpirsky]

Which implementation are we talking about here? Whatever it is, shouldn't we ensure it never returns false as an element of the returned array?

It's RedisCache. It looks like this condition can allow false values if $this->redis->exists($key) is true.

[greg0ire]

RedisCache does not implement MultiGetRegion, does it? We're looking for an implementation of MultiGetRegion that returns a collection of CacheEntry objects, and lets a false slip in.

[greg0ire]

I think the issue might be here:

orm/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php

Lines 53 to 55 in 2726636

	foreach ($keysToRetrieve as $index => $key) {
	$returnableItems[$index] = $items[$key];
	}

See the difference with the other implementation:

orm/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php

Lines 85 to 90 in 2726636

	$entryKey = $this->getCacheEntryKey($key);
	$entryValue = $this->cache->fetch($entryKey);
	if ($entryValue === false) {
	return null;
	}

[umpirsky]

Yes, that looks wrong, it's worth trying.

[umpirsky]

I can't simulate it in the test case:

--- a/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php
+++ b/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php
@@ -38,4 +38,26 @@ class MultiGetRegionTest extends AbstractRegionTest
         self::assertEquals($value1, $actual[0]);
         self::assertEquals($value2, $actual[1]);
     }
+
+    public function testGetMultiCacheMiss() : void
+    {
+        $key1   = new CacheKeyMock('key.1');
+        $value1 = new CacheEntryMock(['id' => 1, 'name' => 'bar']);
+
+        $key2   = new CacheKeyMock('key.2');
+        $value2 = new CacheEntryMock(['id' => 2, 'name' => 'bar']);
+
+        self::assertFalse($this->region->contains($key1));
+        self::assertFalse($this->region->contains($key2));
+
+        $this->region->put($key1, $value1);
+        $this->region->put($key2, $value2);
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertTrue($this->region->contains($key2));
+
+        $actual = $this->region->getMultiple(new CollectionCacheEntry([$key1, $key2]));
+
+        self::assertNull($actual);
+    }

I don't know how to create CacheEntryMock in a way to simulate a cache miss. It also looks like this is prevented in https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php#L89-L91 so theoretically DefaultMultiGetRegion can't receive false value.

Am I missing something?

[lcobucci]

@umpirsky I think we're looking this from the wrong angle... I mean, if the key exists in Redis it means that it was stored as false instead of the expected object and we should look into the write side instead.

[lcobucci]

Also, please send the bugfix to 2.6 branch instead of master. We'll handle the porting :)

[umpirsky]

That's the problem, when I try to create new CacheEntryMock(false) I get InvalidArgumentException: Passed variable is not an array or object. That's why I can't create failing test.

[lcobucci]

@umpirsky I was looking at the write side and had some thoughts that might be related to max value size for strings or serialisation issues... which redis and redis client versions are you using and do you have igbinary installed?

[lcobucci]

@umpirsky I just rebased your branch so that we can make this part of the next bugfix release. I'll continue investigating things here.

[lcobucci]

I'd say things are alright now, although this change doesn't fix the generation of such corrupted data (which I couldn't really find 😞).

It would be nice to get another pair of eyes here, just to avoid potential issues.

@umpirsky thanks for your effort and time 👍

[guilhermeblanco]

Patch seems fine for me aside from the asserts. They should be either removed or try/catch'ed.

[lcobucci]

@guilhermeblanco these are meant for development only and replace the not so useful doc blocks for variables. PHP production config doesn't even generate opcodes for them.

There's an extensive discussion on that in doctrine/coding-standard#47

As discussed IRL these assertions are fine

[umpirsky]

Will this be released in 2.6.4 or 2.7?

[lcobucci]

2.6.4 since it's a bug fix. I plan to release it this weekend.

[umpirsky]

Next weekend I guess? :)

[lcobucci]

@umpirsky sorry, life happened and didn't allow me to handle things. Will do it ASAP (but won't promise any date 😞)