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

[BUG]: Segfault caused in Phalcon\Mvc\Model::toArray #16498

Closed
FredericSchwarz opened this issue Jan 9, 2024 · 1 comment · Fixed by #16499
Closed

[BUG]: Segfault caused in Phalcon\Mvc\Model::toArray #16498

FredericSchwarz opened this issue Jan 9, 2024 · 1 comment · Fixed by #16499
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: high High

Comments

@FredericSchwarz
Copy link

Hello everyone,

I noticed a segfault in our Phalcon application which occurs after a call to Phalcon\Mvc\Model::toArray() on a model that doesn't have getters for all of its properties.

Steps to reproduce the behavior:

CREATE TABLE my_model
(
    col1 text null,
    col2 text null,
    col3 text null,
    col4 text null
);
class MyModel extends Model
{
    protected $col1;
    protected $col2;
    protected $col3;
    protected $col4;

    public function initialize(): void
    {
        $this->setSource('my_model');
    }

    public function getCol1()
    {
        return $this->col1;
    }
}
$item = new MyModel([
    'col1' => 'test',
    'col2' => 'test',
    'col3' => 'test',
    'col4' => 'test'
]);

$item->toArray(); // segfault happens at some point after this call

// the actual code here is not important, we just need some
// memory allocations
for($i = 0; $i < 10; $i++) {
    $item->toArray();
}

Expected behavior
The application doesn't segfault.

Details

  • Phalcon version: 5.5.0
  • PHP Version: 8.1.27
  • Operating System: Centos 7
  • Installation type: installing via package manager
  • Zephir version (if any):
  • Server: Nginx (happens also on CLI)

Additional context
The problem seems to be located in this if statement (https://github.com/phalcon/cphalcon/blob/master/phalcon/Mvc/Model.zep#L3324C1-L3330C14):

if method_exists(this, method) {
    let data[attributeField] = this->{method}();
} elseif fetch value, this->{attributeField} {
    let data[attributeField] = value;
} else {
    let data[attributeField] = null;
}

Changing the above code to:

if method_exists(this, method) {
    let data[attributeField] = this->{method}();
} elseif fetch value, this->{attributeField} {
    let data[attributeField] = value;
} else {
    let data[attributeField] = null;
}
let value = null;

fixes the problem.

From looking at the produced C code my guess is that the call to ZEPHIR_OBS_NVAR(&value) introduced by the fetch is causing issues when used in a loop:
(https://github.com/phalcon/cphalcon/blob/master/ext/phalcon/mvc/model.zep.c#L5652C1)

ZEPHIR_OBS_NVAR(&value);
if ((zephir_method_exists(this_ptr, &method)  == SUCCESS)) {
    ZEPHIR_CALL_METHOD_ZVAL(&_18$$22, this_ptr, &method, NULL, 0);
    zephir_check_call_status();
    zephir_array_update_zval(&data, &attributeField, &_18$$22, PH_COPY | PH_SEPARATE);
} else if (zephir_fetch_property_zval(&value, this_ptr, &attributeField, PH_SILENT_CC)) {
    zephir_array_update_zval(&data, &attributeField, &value, PH_COPY | PH_SEPARATE);
} else {
    zephir_array_update_zval(&data, &attributeField, &__$null, PH_COPY | PH_SEPARATE);
}

Hope this helps.

@FredericSchwarz FredericSchwarz added bug A bug report status: unverified Unverified labels Jan 9, 2024
@niden niden linked a pull request Jan 9, 2024 that will close this issue
5 tasks
@niden niden added status: high High 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Jan 9, 2024
@niden
Copy link
Sponsor Member

niden commented Jan 9, 2024

Thank you @FredericSchwarz

Resolved to #16499

@niden niden closed this as completed Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: high High
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants