Fix #63914 #250

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Contributor

Handle exceptions from the zend_verify_arg_type check in zend_do_fcall_common_helper_SPEC appropriately.

Unfortunately, I can't think of a way to test this 😞

Contributor
lstrojny commented Jan 6, 2013

Could you elaborate on what this patch is trying to do?

Contributor

There's more information here https://bugs.php.net/bug.php?id=63914 and here http://bugs.xdebug.org/view.php?id=897.

The setup for the bug looks something like this:


<?php
class PHPUnit_Util_ErrorHandler
{
    public static function handleError($errno, $errstr, $errfile, $errline)
    {
        throw new Exception;
    }
}

set_error_handler(
  array('PHPUnit_Util_ErrorHandler', 'handleError'),
  E_ALL | E_STRICT
);

$dom = new DOMDocument;
$dom->saveXML('foo');

When zend_verify_arg_type is called in zend_do_fcall_common_helper_SPEC for $dom->saveXML('foo') it triggers the user-defined error handler which throws an uncaught exception. After this, there's really no sense in calling the internal function.

In fact, in the case that you're using an extension that hooks into zend_execute_internal and later calls execute_internal (like xdebug), this can actually cause the internal function to be called with EX(opline) containing exception handling operations from the earlier zend_throw_exception_internal. In PHP <= 5.4, this causes a segfault as the result structure is not initialized properly. In PHP >= 5.5, the segfault is no longer present, but $dom->saveXML('foo') is still executed to completion as if it were called with no arguments.

The best way to follow this is to set up the gdb sessions from https://bugs.php.net/bug.php?id=63914 and step through the execution to see what's happening.

This patch intends to check for an exception after zend_verify_arg_type and avoid the internal function call if it is not necessary.

Contributor
lstrojny commented Jan 6, 2013

Alright, that’s above my head :)

Contributor

A little more context:

This bug is very closely related to https://bugs.php.net/bug.php?id=45805 which was fixed here: 1ff61ab.

The bug exists in the following section: https://github.com/php/php-src/blob/642721b3/Zend/zend_vm_def.h#L1997-L2013.

My patch adds a check to handle the uncaught exception originating from zend_verify_arg_type: https://github.com/whatthejeff/php-src/blob/493b3c374c77587e96ef15527f6bfd954cf87368/Zend/zend_vm_def.h#L1997-L2017.

This patch can be backported to PHP 5.4 (you'll need to update the zend_execute_internal signature), and it fixes the segfault described here: http://bugs.xdebug.org/view.php?id=897.

Maybe @derickr or @sebastianbergmann can take a look at this if they get a minute :)

Contributor

Bump :)

Contributor
derickr commented Mar 9, 2013

Goes a bit over my head as well! Maybe @dstogov knows?

Contributor
nikic commented Mar 9, 2013

Patch looks good to me. At least it makes sense that if an exception is thrown during argument type verification the function should not be run.

Only thing you might want to do is shift the order of the code around so you only initialize the retval if there is no exception (so you don't have to alloc+dtor it when one is thrown):

        if (fbc->common.arg_info) {
            zend_uint i=0;
            zval **p = (zval**)EX(function_state).arguments;
            ulong arg_count = opline->extended_value;

            while (arg_count>0) {
                zend_verify_arg_type(fbc, ++i, *(p-arg_count), 0 TSRMLS_CC);
                arg_count--;
            }
        }

        if (EXPECTED(EG(exception) == NULL)) {
            temp_variable *ret = &EX_T(opline->result.var);

            MAKE_STD_ZVAL(ret->var.ptr);
            ZVAL_NULL(ret->var.ptr);
            ret->var.ptr_ptr = &ret->var.ptr;
            ret->var.fcall_returned_reference = (fbc->common.fn_flags & ZEND_ACC_RETURN_REFERENCE) != 0;

            if (!zend_execute_internal) {
                /* saves one function call if zend_execute_internal is not used */
                fbc->internal_function.handler(opline->extended_value, ret->var.ptr, (fbc->common.fn_flags & ZEND_ACC_RETURN_REFERENCE) ? &ret->var.ptr : NULL, EX(object), RETURN_VALUE_USED(opline) TSRMLS_CC);
            } else {
                zend_execute_internal(execute_data, NULL, RETURN_VALUE_USED(opline) TSRMLS_CC);
            }

            if (!RETURN_VALUE_USED(opline)) {
                zval_ptr_dtor(&ret->var.ptr);
            }
        }
@whatthejeff whatthejeff Fix #63914
Handle exceptions from the `zend_verify_arg_type` check in `zend_do_fcall_common_helper_SPEC` appropriately.
70921ae
Contributor

Good point, @nikic. I've updated the fix to reflect your suggestions.

Contributor

Looks like @dstogov added this at git.php.net. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment