Scope: Fix TOCTOU race in prefix initialization

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;
+            }
          }
       }
 

library/iFixit/Matryoshka/Scope.php

@@ -23,15 +23,22 @@ 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 {
+         if (!$reset && !$generateOnMiss) {
+            $scopeValue = $this->backend->get($this->getScopeKey());
+            if ($scopeValue === self::MISS) {
                return self::MISS;
             }
+         } else {
+            $scopeValue = $this->backend->getAndSet(
+               $this->getScopeKey(),
+               function() {
+                  return substr(md5(microtime() . $this->scopeName), 0, 16);
+               },
+               0,
+               $reset
+            );
          }
+
          $this->scopePrefix = "{$scopeValue}-";
       }
 

tests/AbstractBackendTest.php

@@ -284,6 +284,42 @@ function() { return null; }, 0, $reset = true);
       $this->assertSame($value, $backend->get($key));
    }
 
+   public function testGetAndSetReturnsComputedValue() {
+      $backend = new class extends Matryoshka\Ephemeral {
+         public function add($key, $value, $expiration = 0) {
+            return false;
+         }
+      };
+
+      [$key] = $this->getRandomKeyValue();
+      $computedValue = 'computed';
+
+      $result = $backend->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 +526,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() {

tests/ScopeTest.php

@@ -98,4 +98,76 @@ 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'));
+   }
+
+   /**
+    * 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);
+   }
 }