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

Use preallocated permanent zvals instead of null, true, false, 0 and 1 #1302

Merged
merged 6 commits into from Sep 28, 2013
Merged

Use preallocated permanent zvals instead of null, true, false, 0 and 1 #1302

merged 6 commits into from Sep 28, 2013

Conversation

ghost
Copy link

@ghost ghost commented Sep 27, 2013

This is what was implemented in 2.0.0 / Zephir and appears to be very useful optimization. In some cases it helps avoid creation of the memory frame because we don't need PHALCON_INIT_VAR anymore.

In addition, this PR fixes the bug when a function calls another function (passing it its return_value and return_value_ptr) and the called function throws an exception:

PHP_FUNCTION(test1)
{
    phalcon_call_function_p0_ex(return_value, return_value_ptr, "test2");
}

PHP_FUNCTION(test2)
{
    zend_throw_exception(...);
}

In this scenario, Zend (zend_call_function()) detects the exception and frees the variable return_value_ptr points to. But because test1 passed to test2 its own return_value/return_value_ptr, test2 frees test1's return value. Then, after test2 and test1 return, Zend sees there is an active exception and tries to free the return value of test1 which was already freed on return from test2 and segmentation fault happens.

We tried to address this issue in the past (in phalcon_alt_call_method) but that fix is not applicable to all kinds of static calls because they are dispatched via zend_call_function.

phalcon pushed a commit that referenced this pull request Sep 28, 2013
Use preallocated permanent zvals instead of null, true, false, 0 and 1
@phalcon phalcon merged commit 5e249c1 into phalcon:1.3.0 Sep 28, 2013
@phalcon
Copy link
Collaborator

phalcon commented Sep 28, 2013

Great!

@ghost ghost deleted the perm-zvals branch September 28, 2013 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants