From 440ae2e0e345ef3e75492af02bafd8cf2baaf0d0 Mon Sep 17 00:00:00 2001 From: Daryl Metzler Date: Tue, 7 Apr 2026 13:00:11 -0700 Subject: [PATCH 1/4] Scope: Fix TOCTOU race in prefix initialization Use add() instead of set() when initializing the scope prefix so the first writer wins. If add() fails, re-get() to pick up the winner's value. The $reset path (deleteScope) still uses set() for intentional overwrites. --- library/iFixit/Matryoshka/Scope.php | 30 +++++++++--- tests/ScopeTest.php | 76 +++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/library/iFixit/Matryoshka/Scope.php b/library/iFixit/Matryoshka/Scope.php index bbea6b2..c94e524 100644 --- a/library/iFixit/Matryoshka/Scope.php +++ b/library/iFixit/Matryoshka/Scope.php @@ -23,21 +23,37 @@ public function getPrefix() { public function getScopePrefix(bool $reset = false, bool $generateOnMiss = true) { if ($this->scopePrefix === null || $reset) { - $scopeValue = $reset ? self::MISS : $this->backend->get($this->getScopeKey()); - if ($scopeValue === self::MISS) { - if ($generateOnMiss) { - $scopeValue = substr(md5(microtime() . $this->scopeName), 0, 16); - $this->backend->set($this->getScopeKey(), $scopeValue); - } else { - return self::MISS; + $key = $this->getScopeKey(); + + if ($reset) { + // Intentional overwrite (used by deleteScope()) + $scopeValue = $this->generateScopeValue(); + $this->backend->set($key, $scopeValue); + } else { + $scopeValue = $this->backend->get($key); + if ($scopeValue === self::MISS) { + if (!$generateOnMiss) { + return self::MISS; + } + $scopeValue = $this->generateScopeValue(); + // Use add() for first-writer-wins atomicity. + // If another process already wrote the key, use their value. + if (!$this->backend->add($key, $scopeValue)) { + $scopeValue = $this->backend->get($key); + } } } + $this->scopePrefix = "{$scopeValue}-"; } return $this->scopePrefix; } + private function generateScopeValue(): string { + return substr(md5(microtime() . $this->scopeName), 0, 16); + } + public function getScopeName() { return $this->scopeName; } diff --git a/tests/ScopeTest.php b/tests/ScopeTest.php index 49bb109..50aeb33 100644 --- a/tests/ScopeTest.php +++ b/tests/ScopeTest.php @@ -4,6 +4,28 @@ use iFixit\Matryoshka; +/** + * Simulates a concurrent writer on a shared backend (APCu, Memcached) + * by injecting behavior between get() returning and the caller acting + * on the result. + */ +class RacingBackend extends Matryoshka\BackendWrap { + private $afterNextGet = null; + + public function afterNextGet(callable $fn) { + $this->afterNextGet = $fn; + } + + public function get($key) { + $result = $this->backend->get($key); + if ($fn = $this->afterNextGet) { + $this->afterNextGet = null; + $fn($key); + } + return $result; + } +} + class ScopeTest extends AbstractBackendTest { protected function getBackend() { return new Matryoshka\Scope(new Matryoshka\Ephemeral(), 'scope'); @@ -98,4 +120,58 @@ public function testAbsoluteKey() { $this->assertEquals($scopedCache->getScopePrefix() . $key, $scopedCache->getAbsoluteKey($key)); } + + public function testConcurrentPrefixInitUsesFirstWriter() { + $racing = new RacingBackend(new Matryoshka\Ephemeral()); + $scope = new Matryoshka\Scope($racing, 'test-scope'); + + $competitorPrefix = 'competitor-won'; + + $racing->afterNextGet(function($key) use ($racing, $competitorPrefix) { + $racing->set($key, $competitorPrefix); + }); + + $prefix = $scope->getScopePrefix(); + + $this->assertSame("{$competitorPrefix}-", $prefix); + $this->assertSame($competitorPrefix, $racing->get('scope-test-scope')); + } + + public function testScopePrefixInitializationNoRace() { + $inner = new Matryoshka\Ephemeral(); + $scope = new Matryoshka\Scope($inner, 'test-scope'); + + $prefix = $scope->getScopePrefix(); + + $this->assertNotEmpty($prefix); + $this->assertStringEndsWith('-', $prefix); + $this->assertSame($prefix, $scope->getScopePrefix()); + } + + public function testDeleteScopeOverwritesIntentionally() { + $inner = new Matryoshka\Ephemeral(); + $scope = new Matryoshka\Scope($inner, 'test-scope'); + + $originalPrefix = $scope->getScopePrefix(); + $scope->deleteScope(); + $newPrefix = $scope->getScopePrefix(); + + $this->assertNotSame($originalPrefix, $newPrefix); + } + + public function testConcurrentPrefixInitPreservesFirstWriterData() { + $racing = new RacingBackend(new Matryoshka\Ephemeral()); + $scope = new Matryoshka\Scope($racing, 'test-scope'); + + $competitorPrefix = 'competitor-won'; + + $racing->afterNextGet(function($key) use ($racing, $competitorPrefix) { + $racing->set($key, $competitorPrefix); + $racing->set("{$competitorPrefix}-user-data", 'competitor-data'); + }); + + $scope->getScopePrefix(); + + $this->assertSame('competitor-data', $scope->get('user-data')); + } } From 913b6d7e3ee7b4173ba23f720bf5498eaee7acae Mon Sep 17 00:00:00 2001 From: Daryl Metzler Date: Tue, 7 Apr 2026 13:44:11 -0700 Subject: [PATCH 2/4] getAndSet: first-writer-wins when concurrent A get() then set() allows a slow callback's result to be overwritten by a concurrent caller. Use add() so the first writer wins. Try to fetch the first writers value, if possible, but fall back to the computed value if the get() returns a miss. --- library/iFixit/Matryoshka/Backend.php | 6 ++- tests/AbstractBackendTest.php | 54 +++++++++++++++++++++++++++ tests/ScopeTest.php | 22 ----------- 3 files changed, 59 insertions(+), 23 deletions(-) diff --git a/library/iFixit/Matryoshka/Backend.php b/library/iFixit/Matryoshka/Backend.php index 166940d..f9e74b5 100644 --- a/library/iFixit/Matryoshka/Backend.php +++ b/library/iFixit/Matryoshka/Backend.php @@ -137,7 +137,11 @@ public function getAndSet($key, callable $callback, int $expiration = 0, $value = $callback(); if ($value !== self::MISS) { - $this->set($key, $value, $expiration); + if ($reset) { + $this->set($key, $value, $expiration); + } else if (!$this->add($key, $value, $expiration)) { + $value = $this->get($key) ?? $value; + } } } diff --git a/tests/AbstractBackendTest.php b/tests/AbstractBackendTest.php index d12c56b..7b795cc 100644 --- a/tests/AbstractBackendTest.php +++ b/tests/AbstractBackendTest.php @@ -284,6 +284,39 @@ function() { return null; }, 0, $reset = true); $this->assertSame($value, $backend->get($key)); } + public function testGetAndSetReturnsComputedValue() { + $enable = new Matryoshka\Enable($this->getBackend()); + $enable->writesEnabled = false; + + [$key] = $this->getRandomKeyValue(); + $computedValue = 'computed'; + + $result = $enable->getAndSet($key, function() use ($computedValue) { + return $computedValue; + }); + + $this->assertSame($computedValue, $result); + } + + public function testGetAndSetConcurrentInitUsesFirstWriter() { + $racing = new RacingBackend($this->getBackend()); + [$key] = $this->getRandomKeyValue(); + + $firstWriterValue = 'first-writer'; + $secondWriterValue = 'second-writer'; + + $racing->afterNextGet(function($key) use ($racing, $firstWriterValue) { + $racing->set($key, $firstWriterValue); + }); + + $result = $racing->getAndSet($key, function() use ($secondWriterValue) { + return $secondWriterValue; + }); + + $this->assertSame($firstWriterValue, $result); + $this->assertSame($firstWriterValue, $racing->get($key)); + } + public function testgetAndSetMultiple() { $backend = $this->getBackend(); list($key1, $value1, $id1) = $this->getRandomKeyValueId(); @@ -490,6 +523,27 @@ protected function isCharExemptFromKeyEquivalence($char) { } } +/** + * Simulates a concurrent writer on a shared backend by injecting + * behavior between get() returning and the caller acting on the result. + */ +class RacingBackend extends Matryoshka\BackendWrap { + private $afterNextGet = null; + + public function afterNextGet(callable $fn) { + $this->afterNextGet = $fn; + } + + public function get($key) { + $result = $this->backend->get($key); + if ($fn = $this->afterNextGet) { + $this->afterNextGet = null; + $fn($key); + } + return $result; + } +} + // Exposes the array of cached values. class TestEphemeral extends Matryoshka\Ephemeral { public function getCache() { diff --git a/tests/ScopeTest.php b/tests/ScopeTest.php index 50aeb33..909576a 100644 --- a/tests/ScopeTest.php +++ b/tests/ScopeTest.php @@ -4,28 +4,6 @@ use iFixit\Matryoshka; -/** - * Simulates a concurrent writer on a shared backend (APCu, Memcached) - * by injecting behavior between get() returning and the caller acting - * on the result. - */ -class RacingBackend extends Matryoshka\BackendWrap { - private $afterNextGet = null; - - public function afterNextGet(callable $fn) { - $this->afterNextGet = $fn; - } - - public function get($key) { - $result = $this->backend->get($key); - if ($fn = $this->afterNextGet) { - $this->afterNextGet = null; - $fn($key); - } - return $result; - } -} - class ScopeTest extends AbstractBackendTest { protected function getBackend() { return new Matryoshka\Scope(new Matryoshka\Ephemeral(), 'scope'); From b83dd65fc9d57b9cef089c5fa444f37c4ffd2eda Mon Sep 17 00:00:00 2001 From: Daryl Metzler Date: Tue, 7 Apr 2026 14:04:52 -0700 Subject: [PATCH 3/4] Handle failed `add` in Scope as well If we cannot `add`, we try to fetch the first writers value. If that fails, make sure to always return a scope prefix. This is the same failure mode that was fixed for `getAndSet`, which also always needs to return a value. Note: we can clean up the getAndSet test to be a bit more intent revealing, using the same pattern found for the prefix test. --- library/iFixit/Matryoshka/Scope.php | 2 +- tests/AbstractBackendTest.php | 9 ++++++--- tests/ScopeTest.php | 18 ++++++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/library/iFixit/Matryoshka/Scope.php b/library/iFixit/Matryoshka/Scope.php index c94e524..78494de 100644 --- a/library/iFixit/Matryoshka/Scope.php +++ b/library/iFixit/Matryoshka/Scope.php @@ -39,7 +39,7 @@ public function getScopePrefix(bool $reset = false, bool $generateOnMiss = true) // Use add() for first-writer-wins atomicity. // If another process already wrote the key, use their value. if (!$this->backend->add($key, $scopeValue)) { - $scopeValue = $this->backend->get($key); + $scopeValue = $this->backend->get($key) ?? $scopeValue; } } } diff --git a/tests/AbstractBackendTest.php b/tests/AbstractBackendTest.php index 7b795cc..e5fd867 100644 --- a/tests/AbstractBackendTest.php +++ b/tests/AbstractBackendTest.php @@ -285,13 +285,16 @@ function() { return null; }, 0, $reset = true); } public function testGetAndSetReturnsComputedValue() { - $enable = new Matryoshka\Enable($this->getBackend()); - $enable->writesEnabled = false; + $backend = new class extends Matryoshka\Ephemeral { + public function add($key, $value, $expiration = 0) { + return false; + } + }; [$key] = $this->getRandomKeyValue(); $computedValue = 'computed'; - $result = $enable->getAndSet($key, function() use ($computedValue) { + $result = $backend->getAndSet($key, function() use ($computedValue) { return $computedValue; }); diff --git a/tests/ScopeTest.php b/tests/ScopeTest.php index 909576a..234988f 100644 --- a/tests/ScopeTest.php +++ b/tests/ScopeTest.php @@ -152,4 +152,22 @@ public function testConcurrentPrefixInitPreservesFirstWriterData() { $this->assertSame('competitor-data', $scope->get('user-data')); } + + /** + * If add() fails and the re-fetch also misses (e.g. the backend lost + * the key between add and get), the generated prefix is still used. + */ + public function testScopePrefixNotEmpty() { + $backend = new class extends Matryoshka\Ephemeral { + public function add($key, $value, $expiration = 0) { + return false; + } + }; + $scope = new Matryoshka\Scope($backend, 'test-scope'); + + $prefix = $scope->getScopePrefix(); + + $this->assertNotSame('-', $prefix); + $this->assertStringEndsWith('-', $prefix); + } } From 05cc5e2c60c01014b9b4f3650adbb0668c0f09b1 Mon Sep 17 00:00:00 2001 From: Daryl Metzler Date: Tue, 7 Apr 2026 15:32:50 -0700 Subject: [PATCH 4/4] Resume using `getAndSet` for getScopePrefix With the race condition also addressed in `getAndSet`, we can now safely rely on it for concurrent scope initialization. Revert back to the original version. --- library/iFixit/Matryoshka/Scope.php | 33 +++++++++++------------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/library/iFixit/Matryoshka/Scope.php b/library/iFixit/Matryoshka/Scope.php index 78494de..9abeab5 100644 --- a/library/iFixit/Matryoshka/Scope.php +++ b/library/iFixit/Matryoshka/Scope.php @@ -23,25 +23,20 @@ public function getPrefix() { public function getScopePrefix(bool $reset = false, bool $generateOnMiss = true) { if ($this->scopePrefix === null || $reset) { - $key = $this->getScopeKey(); - - if ($reset) { - // Intentional overwrite (used by deleteScope()) - $scopeValue = $this->generateScopeValue(); - $this->backend->set($key, $scopeValue); - } else { - $scopeValue = $this->backend->get($key); + if (!$reset && !$generateOnMiss) { + $scopeValue = $this->backend->get($this->getScopeKey()); if ($scopeValue === self::MISS) { - if (!$generateOnMiss) { - return self::MISS; - } - $scopeValue = $this->generateScopeValue(); - // Use add() for first-writer-wins atomicity. - // If another process already wrote the key, use their value. - if (!$this->backend->add($key, $scopeValue)) { - $scopeValue = $this->backend->get($key) ?? $scopeValue; - } + return self::MISS; } + } else { + $scopeValue = $this->backend->getAndSet( + $this->getScopeKey(), + function() { + return substr(md5(microtime() . $this->scopeName), 0, 16); + }, + 0, + $reset + ); } $this->scopePrefix = "{$scopeValue}-"; @@ -50,10 +45,6 @@ public function getScopePrefix(bool $reset = false, bool $generateOnMiss = true) return $this->scopePrefix; } - private function generateScopeValue(): string { - return substr(md5(microtime() . $this->scopeName), 0, 16); - } - public function getScopeName() { return $this->scopeName; }