[CRASH] Bug in phalcon_read_property_XXX()

[ghost]

The code of phalcon_read_property_read_quick (reformatted a bit):

if (
    UNEXPECTED(!property_info) || (property_info->offset >= 0) 
        ? (zobj->properties
            ? ((zv = (zval**)zobj->properties_table[property_info->offset]) == NULL)
            : (*(zv = &zobj->properties_table[property_info->offset]) == NULL)
        )
        : 1
) {

I am not sure it works as expected: due to operator precedence UNEXPECTED(!property_info) || (property_info->offset >= 0) will be evaluated first.

Then, if we (unlikely) have property_info == NULL and zobj->properties != NULL then property_info->offset will dereference a NULL pointer which is probably not the expected behavior.

Looking at how this is implemented in zend_std_read_property() (reformatted code):

if (
    UNEXPECTED(!property_info) ||
    (
        (EXPECTED((property_info->flags & ZEND_ACC_STATIC) == 0) &&  property_info->offset >= 0)
            ? (zobj->properties
                ? ((retval = (zval**)zobj->properties_table[property_info->offset]) == NULL)
                : (*(retval = &zobj->properties_table[property_info->offset]) == NULL)
            )
            : (
                UNEXPECTED(!zobj->properties) || 
                UNEXPECTED(zend_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &retval) == FAILURE)
            )
    )
) {

We are not reading a static property, therefore property_info->flags & ZEND_ACC_STATIC) == 0. Then,

if (
    UNEXPECTED(!property_info) ||
    (
        (property_info->offset >= 0)
            ? (zobj->properties
                ? ((retval = (zval**)zobj->properties_table[property_info->offset]) == NULL)
                : (*(retval = &zobj->properties_table[property_info->offset]) == NULL)
            )
            : (
                UNEXPECTED(!zobj->properties) || 
                UNEXPECTED(zend_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &retval) == FAILURE)
            )
    )
) {

There are extra parentheses around the second operand to ||. In Zend code property_info->offset is checked only when property_info != NULL && property_info->offset >= 0

In Phalcon when (property_info != NULL) || (property_info->offset >= 0) which may lead to NULL pointer dereferencing.

The other places where the similar thing happens are

• phalcon_update_property_this_quick()
• phalcon_update_property_this()

[phalcon]

Thanks, can we split that line into several lines to avoid codification errors?

[ghost]

OK, let's have it like this then?

if (
    UNEXPECTED(!property_info) || 
    (
        (property_info->offset >= 0) 
            ? (zobj->properties
                ? ((zv = (zval**)zobj->properties_table[property_info->offset]) == NULL)
                : (*(zv = &zobj->properties_table[property_info->offset]) == NULL)
            )
            : 1
    )
) {

BTW, sould we replace 1 with

UNEXPECTED(!zobj->properties) || 
UNEXPECTED(zend_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &retval) == FAILURE)

?

I did not dive deep into Zend Engine internals

[phalcon]

update_this and update_this_quick are only used to access internal properties that are known to be declared in the internal class, accessing dynamic properties is something unexpected.

[ghost]

OK, will send a pull request in a few hours

[ghost]

Looks like there is another issue:

zval *zv;
if (
    UNEXPECTED(!property_info) || 
    (
        (
            EXPECTED((property_info->flags & ZEND_ACC_STATIC) == 0) && 
            property_info->offset >= 0
        )
            ? (
                zobj->properties 
                    ? ((zv = (zval**) zobj->properties_table[property_info->offset]) == NULL) 
                    : (*(zv = &zobj->properties_table[property_info->offset]) == NULL)
            ) 
            : 1
    )
) {
    if (zobj->properties && property_info && phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE) {
        *result = *zv;
        Z_ADDREF_PP(result);
        EG(scope) = old_scope;
        return SUCCESS;
    }
} else {
    *result = *zv;
    Z_ADDREF_PP(result);
    EG(scope) = old_scope;
    return SUCCESS;
}

If property_info is NULL, zv is never initialized and we are dereferencing garbage in *result = *zv;

[phalcon]

It's the same as update_this and update_this_quick, these functions only muyst be used if the property is defined using some of the zend_declareproperty* functions

[ghost]

Yes, I understand this; the above code is from Phalcon (namely from phalcon_read_property_this()).

The issue is that:

• we either perform superfluous checks and we have dead code which will never be executed (not a big deal if so);
• we miss a check and under rare circumstances we might have a crash.

For example, if you look at phalcon_read_property_this(), those conditions can be rewritten in a more human friendly form like this (please double check!):

int flag;
if (property_info) {
    if ((property_info->flags & ZEND_ACC_STATIC) == 0 && property_info->offset >= 0) {
        if (zobj->properties) {
            zv = (zval**) zobj->properties_table[property_info->offset];
            flag = (zv == NULL) ? 1 : 0;
        }
        else {
            zv = &zobj->properties_table[property_info->offset];
            flag = (*zv == NULL) ? 1 : 0;
        }
    }
    else {
        flag = 1;
    }
}
else {
    flag = 1;
}

if (flag) {
    if (
            zobj->properties 
         && property_info /* This check was not there originally; without it, could dereference NULL */
         && phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE
    ) {
/* Now, when (flag == 1 && zobj->properties != NULL), this means that zv IS NULL
We are dereferencing a NULL pointer */
        *result = *zv;
        Z_ADDREF_PP(result);
        EG(scope) = old_scope;
        return SUCCESS;
    }
}
else {
/* Check this branch: when property_info is NULL, zv is garbage */
    *result = *zv;
    Z_ADDREF_PP(result);
    EG(scope) = old_scope;
    return SUCCESS;
}

[ghost]

OK, I think I now know how it all works :-)

if (phalcon_hash_find(&ce->properties_info, property_name, property_length + 1, (void **) &property_info) == SUCCESS) {
/* At this point we can safely assume that property_info is not NULL
   because properties_info contains only valid data */
    int flag;

    if ((property_info->flags & ZEND_ACC_STATIC) == 0 && property_info->offset >= 0) {
        if (zobj->properties) {
        /* We get here if someone added a dynamic property OR
           rebuild_object_properties() has been called */
            zv = (zval**) zobj->properties_table[property_info->offset];
            flag = (zv == NULL) ? 1 : 0;
        }
        else {
            zv = &zobj->properties_table[property_info->offset];
            flag = (*zv == NULL) ? 1 : 0;
        }
    }
    else {
    /* Either property is static or property_info->offset < 0 */
        flag = 1;
    }

    if (flag) {
    /* Either offset is negative or zv is NULL or *zv is NULL 
       or property is static */
        if (zobj->properties && phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE) {
    /* We have dynamic properties but the property
       was not found there, COULD RETURN GARBAGE! */
            *result = *zv;
            Z_ADDREF_PP(result);
            EG(scope) = old_scope;
            return SUCCESS;
        }
    } else {
/* As far as I can tell, we usually go this route */
        *result = *zv;
        Z_ADDREF_PP(result);
        EG(scope) = old_scope;
        return SUCCESS;
    }
}

[ghost]

And here is the proof that the current implementation is not reliable and can crash:

<?php

class MyCache extends \Phalcon\Cache\Backend\Memory
{
        public function bork()
        {
                unset($this->_prefix);
        }
}

$front = new Phalcon\Cache\Frontend\Data();
$cache = new MyCache($front);
$cache->save('test-data', 'xxx');
$cache->bork();
print_r($cache);
$cache->get('test-data');

Core was generated by `php crash.php'.
Program terminated with signal 11, Segmentation fault.
#0  0x00007f2cef1da16a in phalcon_read_property_this (result=0x7fffc11cfb98, object=0x7f2cf8ae6548, property_name=0x7f2cef7648fd "_prefix", property_length=7, silent=0) at /home/vladimir/workspace/cphalcon/ext/kernel/object.c:524
524                                             *result = *zv;
(gdb) bt full
#0  0x00007f2cef1da16a in phalcon_read_property_this (result=0x7fffc11cfb98, object=0x7f2cf8ae6548, property_name=0x7f2cef7648fd "_prefix", property_length=7, silent=0) at /home/vladimir/workspace/cphalcon/ext/kernel/object.c:524
        zv = 0x0
        zobj = 0x7f2cf8ae65a8
        property_info = 0x7f2cf8ae3ac8
        ce = 0x7f2cf8ae3718
        old_scope = 0x0
#1  0x00007f2cef38ed8f in zim_Phalcon_Cache_Backend_Memory_get (ht=1, return_value=0x7f2cf8ae3678, return_value_ptr=0x0, this_ptr=0x7f2cf8ae6548, return_value_used=0) at /home/vladimir/workspace/cphalcon/ext/cache/backend/memory.c:102
        key_name = 0x7f2cf8ae3548
        lifetime = 0x7f2cf8ae36c8
        last_key = 0x0
        prefix = 0x4
        data = 0x7f2cf8ae65a8
        cached_content = 0x7f2cf8ae6ce8
        frontend = 0x3
        processed = 0x7f2cf8aad060
#2  0x000000000075ef52 in zend_do_fcall_common_helper_SPEC (execute_data=0x7f2cf8aad060) at /build/buildd/php5-5.4.9/Zend/zend_vm_execute.h:642
#3  0x0000000000718857 in execute (op_array=0x7f2cf8ae46d8) at /build/buildd/php5-5.4.9/Zend/zend_vm_execute.h:410
#4  0x00000000006b86cc in zend_execute_scripts (type=-122805032, retval=0x300000008, file_count=32556) at /build/buildd/php5-5.4.9/Zend/zend.c:1309
#5  0x0000000000658373 in php_execute_script (primary_file=0x0) at /build/buildd/php5-5.4.9/main/main.c:2482
#6  0x0000000000761583 in do_cli (argc=0, argv=0x7fffc11d521e) at /build/buildd/php5-5.4.9/sapi/cli/php_cli.c:988
#7  0x000000000042c750 in main (argc=32767, argv=0x1e35210) at /build/buildd/php5-5.4.9/sapi/cli/php_cli.c:1364

[phalcon]

You're right I haven't thought in that scenario, users can unset built-in properties... is that patch part of the pull request?

[ghost]

778 is intended to fix that.

I haven't looked into phalcon_update_property_XXX() functions much yet

[ghost]

In your opinion, should we do this as well:

 if (
    (
        EXPECTED((property_info->flags & ZEND_ACC_STATIC) == 0) && 
        property_info->offset >= 0
    )
        ? (
            zobj->properties 
                ? ((zv = (zval**) zobj->properties_table[property_info->offset]) == NULL) 
                : (*(zv = &zobj->properties_table[property_info->offset]) == NULL)
        ) 
-       : 1
+       : (
+           UNEXPECTED(!zobj->properties) || 
+           UNEXPECTED(phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE)
+       )
 ) {

?

[phalcon]

Yep, it seems razonable given the current circumstances

[ghost]

OK, here is the new code:

int flag;
if (EXPECTED((property_info->flags & ZEND_ACC_STATIC) == 0) && property_info->offset >= 0) {
    if (zobj->properties) {
        zv   = (zval**) zobj->properties_table[property_info->offset];
        flag = (zv == NULL) ? 1 : 0;
    }
    else {
        zv   = &zobj->properties_table[property_info->offset];
        flag = (*zv == NULL) ? 1 : 0;
    }
}
else if (UNEXPECTED(!zobj->properties)) {
    flag = 1;
}
else if (UNEXPECTED(phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE)) {
    flag = 2;
}
else {
    flag = 0;
}

if (unlikely(flag)) {
    if (zobj->properties) {
        if (
            (flag == 2 || phalcon_hash_quick_find(zobj->properties, property_info->name, property_info->name_length+1, property_info->h, (void **) &zv) == FAILURE)
            && zv && *zv
        ) {
            flag = 0;
        }
    }
}

if (likely(!flag)) {
    *result = *zv;
    Z_ADDREF_PP(result);
    EG(scope) = old_scope;
    return SUCCESS;
}

[ghost]

BTW, what do you think about this:

int phalcon_update_property_this(zval *object, char *property_name, unsigned int property_length, zval *value TSRMLS_DC)
{
    return phalcon_update_property_this_quick(object, property_name, property_length, value, zend_inline_hash_func(property_name, property_length+1) TSRMLS_CC);
}

and same for phalcon_read_property_this? In this case CI tests will run on approximately the same code (_quick functions are used only by the release code now, with this change _quick functions will be tested by CI as well).

[phalcon]

Yes, I like it, less code to maintain :)