Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segfault on Model::save() with circular relations #13354

Merged
merged 1 commit into from May 24, 2019

Conversation

@stecman
Copy link
Contributor

commented Apr 23, 2018

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

Save A
 \_ A hasMany B (post-save related records)
  \_ Save B
   \_ B belongsTo A (pre-save related records)
    \_ Save A
     \_ ...

For me this was causing a segfault, though with xdebug installed it would error before crashing at the max_nesting_level.

I ran into this while writing a model where relationships are traversed in both directions as part of some pre-save processing (with a little plumbing to use relations before saving).

@stecman stecman changed the title Fix segfault on infinite recursion into circular model relations Fix segfault on Model::save() with circular relations Apr 23, 2018

@sergeyklay sergeyklay changed the base branch from 3.3.x to 3.4.x Apr 23, 2018

@stecman stecman force-pushed the stecman:fix_save_circular_relations branch 2 times, most recently from 0a8b358 to 8657bb5 Apr 23, 2018

@sergeyklay sergeyklay added this to the 3.4.x milestone Apr 24, 2018

@virgofx

This comment has been minimized.

Copy link
Member

commented May 24, 2018

@stecman The change impacts such a core area of the Phalcon ORM which will have drastic implications if not thoroughly tested.

Can you provide exact model setup to reproduce the issue in your case (where it crashed) and then before this can get integrated there will definitely need to be updates into the model tests that incorporate this.

@sergeyklay sergeyklay removed this from the 3.4.x milestone May 27, 2018

@Jurigag

This comment has been minimized.

Copy link
Member

commented May 28, 2018

Wouldn't this break saving when we change something on relations of saved model, like it wouldn't save them again?

@stecman

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2018

@virgofx Here's a minimal setup to replicate this problem:

Database and model:

CREATE TABLE `robot` (
    id INT NOT NULL AUTO_INCREMENT,
    name VARCHAR(255), -- stop ORM throwing when no fields to save
    PRIMARY KEY (id)
);

CREATE TABLE `part` (
    id INT NOT NULL AUTO_INCREMENT,
    robot_id INT NOT NULL,
    PRIMARY KEY (id)
);
class Robot extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasMany('id', 'Part', 'robot_id', [
            'alias' => 'Parts'
        ]);
    }
}
class Part extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->belongsTo('robot_id', 'Robot', 'id');
    }
}

Repro:

$robot = new Robot();
$part = new Part();

$robot->parts = [$part];
$part->robot = $robot;

$robot->save(); // <-- segfaults
@Jurigag
Copy link
Member

left a comment

I guess we need to test it.

@stecman stecman force-pushed the stecman:fix_save_circular_relations branch from 8657bb5 to 11d0edc Oct 14, 2018

@CameronHall

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I tried reproducing this myself a while ago on 3.4.1 and couldn't. Can anyone else reproduce?

@stecman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@CameronHall - curious, I can still reproduce this with v3.4.1 and v3.4.3. Can you try this all-in-one script?

If you don't have the SQLite driver installed, there's a MySQL variant here.

<?php

$di = new \Phalcon\Di\FactoryDefault();

$di->setShared('db', function () {
    return new \Phalcon\Db\Adapter\Pdo\Sqlite([
        'dbname' => 'issue13354.sqlite'
    ]);
});

$di['db']->execute(<<<SQL

DROP TABLE IF EXISTS `robot`;
DROP TABLE IF EXISTS `part`;

CREATE TABLE `robot` (
    id INT,
    name VARCHAR(255), -- stop ORM throwing when no fields to save
    PRIMARY KEY (id)
);

CREATE TABLE `part` (
    id INT,
    robot_id INT,
    PRIMARY KEY (id)
);
SQL);

class Robot extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasMany('id', 'Part', 'robot_id', [
            'alias' => 'Parts'
        ]);
    }
}

class Part extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->belongsTo('robot_id', 'Robot', 'id');
    }
}

// Repro
$robot = new Robot();
$part = new Part();

$robot->parts = [$part];
$part->robot = $robot;

echo "Going to save..\n";

$robot->save(); // <-- segfaults

echo "Finished executing\n";

Running under v3.4.3 I see:

$ php issue13354.php 
Going to save..
Segmentation fault

Running with this patch I see:

$ php issue13354.php 
Going to save..
Finished executing
@CameronHall

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@stecman beautiful! Just what I needed - the SQLite driver was a great idea. I managed to reproduce the issue.

I'll give the patch a whirl later tonight and we can go from there.

@niden

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@stecman can you rebase this for the 4.0.x branch?

As for the tests you can add them in integration/Mvc/Model and if you need to add fixtures you can add them to tests/_data/fixtures

@niden niden added Bug - Low and removed Not verified labels Apr 16, 2019

@niden niden added this to In progress in 4.0 Release via automation Apr 16, 2019

@niden

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

And we need the changelog updated please :)

@stecman stecman force-pushed the stecman:fix_save_circular_relations branch from 11d0edc to 915b53d Apr 28, 2019

@stecman stecman changed the base branch from 3.4.x to 4.0.x Apr 28, 2019

@stecman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Rebased against 4.0.x and changelog updated 👍 I'll get the test case added in the next couple of days, but I've confirmed the issue still exists on 4.0.x and this patch fixes it.

The original patch targeting 3.4.x can be found here if anyone needs it.

@niden

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@stecman Thank you for that. Ping me when this is fully ready.

@niden

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@stecman nudge for the tests on this and if you don't mind rebasing the code.

I would like to get this in the resolved column

@stecman stecman force-pushed the stecman:fix_save_circular_relations branch from 915b53d to 43bf1cd May 22, 2019

@stecman

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@niden Rebased again and test case added!

@niden

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@stecman can you have a look at the tests please? also if you don't mind rebase this

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.

@stecman stecman force-pushed the stecman:fix_save_circular_relations branch from 86542ea to 62136f3 May 24, 2019

@stecman

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Aha, thanks - missed that the leading underscore had been dropped from _dirtyState. Should be good to go now

@codecov

This comment has been minimized.

Copy link

commented May 24, 2019

Codecov Report

Merging #13354 into 4.0.x will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            4.0.x   #13354      +/-   ##
==========================================
+ Coverage   72.23%   72.26%   +0.03%     
==========================================
  Files         464      464              
  Lines       96056    96061       +5     
==========================================
+ Hits        69386    69422      +36     
+ Misses      26670    26639      -31
Impacted Files Coverage Δ
ext/phalcon/mvc/model.zep.c 71.99% <0%> (+0.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86075c0...62136f3. Read the comment docs.

@niden

niden approved these changes May 24, 2019

@niden niden merged commit f74ba2b into phalcon:4.0.x May 24, 2019

4 checks passed

codecov/patch Coverage not affected when comparing 86075c0...62136f3
Details
codecov/project 72.26% (+0.03%) compared to 86075c0
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

4.0 Release automation moved this from In progress to Done May 24, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Thank you @stecman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.