From 62136f35a0996cc1358a5ae22e6070ba99043fb9 Mon Sep 17 00:00:00 2001
From: Stephen Holdaway <stecman@gmail.com>
Date: Sun, 28 Apr 2019 20:28:37 +1200
Subject: [PATCH] Fix segfault on infinite recursion into circular model
 relations

In cases where related models were set before save on *both* sides of a
hasMany/belongsTo relationship, `Model::save()` would go into an infinite
loop as its pre and post-save checks for relations to save never checked
if the related model had already been saved:

    Save A
     \_ A hasMany B
      \_ Save B
       \_ B belongsTo A
        \_ Save A
         \_ ...

For me this was causing a segfault, though with xdebug installed it would
error before crashing at the max_nesting_level.
---
 CHANGELOG-4.0.md                         |  1 +
 phalcon/Mvc/Model.zep                    |  6 +++---
 tests/integration/Mvc/Model/SaveCest.php | 27 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG-4.0.md b/CHANGELOG-4.0.md
index 752a7dfd873..e8cfba6a9b0 100644
--- a/CHANGELOG-4.0.md
+++ b/CHANGELOG-4.0.md
@@ -39,6 +39,7 @@
 - Fixed `method` parameter in `Phalcon\Mvc\Model\Manager::getHasOneRecords()`, it's not always a string, null by default. [#14115](https://github.com/phalcon/cphalcon/issues/14115)
 - Fixed `method` parameter in `Phalcon\Mvc\Model\Manager::getHasManyRecords()`, it's not always a string, null by default. [#14115](https://github.com/phalcon/cphalcon/issues/14115)
 - `handlers` property in `Phalcon\Mvc\Micro\Collection` is now always an array.
+- Fixed crash in `Phalcon\Mvc\Model::save()` when saving a circular model relation. [#13354](https://github.com/phalcon/cphalcon/pull/13354)
 
 ## Removed
 - Removed `Phalcon\Session\Factory`. [#13672](https://github.com/phalcon/cphalcon/issues/13672)
diff --git a/phalcon/Mvc/Model.zep b/phalcon/Mvc/Model.zep
index c5d6b3f6693..d5882a87bca 100644
--- a/phalcon/Mvc/Model.zep
+++ b/phalcon/Mvc/Model.zep
@@ -4625,10 +4625,10 @@ abstract class Model implements EntityInterface, ModelInterface, ResultInterface
                     }
 
                     /**
-                     * If dynamic update is enabled, saving the record must not
-                     * take any action
+                     * If dynamic update is enabled, saving the record must not take any action
+                     * Only save if the model is dirty to prevent circular relations causing an infinite loop
                      */
-                    if !record->save() {
+                    if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->save() {
                         /**
                          * Get the validation messages generated by the
                          * referenced model
diff --git a/tests/integration/Mvc/Model/SaveCest.php b/tests/integration/Mvc/Model/SaveCest.php
index ebfdb36a0a7..d2587911a8a 100644
--- a/tests/integration/Mvc/Model/SaveCest.php
+++ b/tests/integration/Mvc/Model/SaveCest.php
@@ -334,4 +334,31 @@ public function mvcModelSaveAfterWithoutDefaultValues(IntegrationTester $I)
             $robot->delete()
         );
     }
+
+    public function mvcModelSaveCircularRelation(IntegrationTester $I)
+    {
+        $I->wantToTest('Mvc\Model::save() with circular unsaved relations');
+
+        $album = new Albums([
+            'name' => 'Loopback'
+        ]);
+        $artist = new Artists([
+            'name' => 'Evil Robot'
+        ]);
+
+        // Assign relationship in both directions on unsaved models
+        $album->artist = $artist;
+        $artist->albums = [
+            $album
+        ];
+
+        // Save should handle the circular relation without issue
+        $I->assertTrue(
+            $artist->save()
+        );
+
+        // Both should have an ID now
+        $I->assertNotEquals(null, $album->id);
+        $I->assertNotEquals(null, $artist->id);
+    }
 }