bug #62199 [Workflow] State Contamination in Marking Stores due to Class-based Getter Cache (siganushka)

nicolas-grekas · nicolas-grekas · commit 99bbae923135 · 2025-10-29T14:16:25.000+01:00
This PR was merged into the 7.4 branch. Discussion ---------- [Workflow] State Contamination in Marking Stores due to Class-based Getter Cache State Contamination in Marking Stores due to Class-based Getter Cache | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT The issue manifests in Marking Store configurations (e.g., `MethodMarkingStore`), tracing back to a logic flaw in the core Workflow component's state-accessing mechanism. The `getMarking()` method caches the state-accessing Getter Closure based solely on the class name (`Subject::class`). In a concurrent, multi-subject environment (e.g., Messenger Workers), this leads to state contamination: A Worker processes Subject 1, successfully transitioning its state from first_place to second_place. The cached Getter Closure is created or updated, capturing a reference or memory state related to second_place. When the same Worker processes Subject 2 (a different instance, whose actual state is first_place in the database), the cached Getter is used. This Getter incorrectly returns the state related to Subject 1's final marking (second_place), despite Subject 2's data being correct. Consequently, `getMarking()` reports a marking of ["second_place":1], causing valid transitions from first_place to be incorrectly blocked. ```php $subject1 = new Subject('first_place'); $subject2 = new Subject('second_place'); $markingStore = new MethodMarkingStore(true); $marking1 = $markingStore->getMarking($subject1); $marking2 = $markingStore->getMarking($subject2); // Expected: ["first_place" => 1] // Actual: ["first_place" => 1] $marking1 ->getPlaces(); // Expected: ["second_place" => 1] // Actual: ["first_place" => 1] $marking2 ->getPlaces(); ``` Commits ------- 160608d fix: fixed State contamination in marking stores due to class-based getter cache
diff --git a/src/Symfony/Component/Workflow/MarkingStore/MethodMarkingStore.php b/src/Symfony/Component/Workflow/MarkingStore/MethodMarkingStore.php
@@ -51,7 +51,7 @@ public function getMarking(object $subject): Marking
     {
         $marking = null;
         try {
-            $marking = ($this->getters[$subject::class] ??= $this->getGetter($subject))();
+            $marking = ($this->getters[$subject::class] ??= $this->getGetter($subject))($subject);
         } catch (\Error $e) {
             $unInitializedPropertyMessage = \sprintf('Typed property %s::$%s must not be accessed before initialization', get_debug_type($subject), $this->property);
             if ($e->getMessage() !== $unInitializedPropertyMessage) {
@@ -93,8 +93,8 @@ private function getGetter(object $subject): callable
         $method = 'get'.ucfirst($property);
 
         return match (self::getType($subject, $property, $method)) {
-            MarkingStoreMethod::METHOD => $subject->{$method}(...),
-            MarkingStoreMethod::PROPERTY => static fn () => $subject->{$property},
+            MarkingStoreMethod::METHOD => static fn ($subject) => $subject->{$method}(),
+            MarkingStoreMethod::PROPERTY => static fn ($subject) => $subject->{$property},
         };
     }
 
diff --git a/src/Symfony/Component/Workflow/Tests/MarkingStore/MethodMarkingStoreTest.php b/src/Symfony/Component/Workflow/Tests/MarkingStore/MethodMarkingStoreTest.php
@@ -125,6 +125,23 @@ public function testGetMarkingWithUninitializedProperty2()
         $markingStore->getMarking($subject);
     }
 
+    public function testGetMarkingWithSameSubjectMultipleTimes()
+    {
+        $subject1 = new Subject('first_place');
+        $subject2 = new Subject('second_place');
+        $subject3 = new Subject('third_place');
+
+        $markingStore = new MethodMarkingStore(true);
+
+        $marking1 = $markingStore->getMarking($subject1);
+        $marking2 = $markingStore->getMarking($subject2);
+        $marking3 = $markingStore->getMarking($subject3);
+
+        $this->assertSame(['first_place' => 1], $marking1->getPlaces());
+        $this->assertSame(['second_place' => 1], $marking2->getPlaces());
+        $this->assertSame(['third_place' => 1], $marking3->getPlaces());
+    }
+
     private function createValueObject(string $markingValue): object
     {
         return new class($markingValue) {
