Prevent segmentation fault in Reflection->getParameters()

[Caffe1neAdd1ct]

When a native php function has been disabled via disabled_functions (php.ini) calling getParameters() on the reflection object of the disabled function causes a segmentation fault on PHP 5.6.31.

Reproducible on both Arch Linux and CentOS 6 with the following script:

php.ini

disable_functions = "passthru"

console.php

<?php

require_once 'vendor/autoload.php';

use Raven_Client;

$connection = false;
$client = new Raven_Client($connection);
$client->environment = 'test';
$client->install();

passthru('clear');

Run straight through either php or php-cli php56 console.php gives a segmentation fault as follows:

[1]    22373 segmentation fault (core dumped)  php56 console.php

Testing through php 7 does not segfault and reflection seems to be able to cope with the disabled function:

PHP Warning:  passthru() has been disabled for security reasons in /home/kandrews/NetBeansProjects/compareandrecycle.co.uk/console.php on line 12
PHP Stack trace:
PHP   1. {main}() /home/kandrews/NetBeansProjects/compareandrecycle.co.uk/console.php:0
PHP   2. passthru() /home/kandrews/NetBeansProjects/compareandrecycle.co.uk/console.php:12

[Caffe1neAdd1ct]

A bit more history on this one, our staging environment started segmentation faulting only on cli scripts. Enabling the core dump files and running through gdb for a backtrace gave:

#0  reflection_parameter_factory (ht=<value optimized out>, return_value=0x7eff1ce342c0, return_value_ptr=<value optimized out>, this_ptr=<value optimized out>, 
    return_value_used=<value optimized out>) at /home/cpeasyapache/src/php-5.6.31/ext/reflection/php_reflection.c:1285
#1  zim_reflection_function_getParameters (ht=<value optimized out>, return_value=0x7eff1ce342c0, return_value_ptr=<value optimized out>, this_ptr=<value optimized out>, 
    return_value_used=<value optimized out>) at /home/cpeasyapache/src/php-5.6.31/ext/reflection/php_reflection.c:2111
#2  0x00000000008070f2 in zend_do_fcall_common_helper_SPEC (execute_data=<value optimized out>) at /home/cpeasyapache/src/php-5.6.31/Zend/zend_vm_execute.h:558
#3  0x00000000007f6980 in execute_ex (execute_data=0x7eff2849aa30) at /home/cpeasyapache/src/php-5.6.31/Zend/zend_vm_execute.h:363
#4  0x000000000077a19e in zend_call_function (fci=0x7ffdc345b850, fci_cache=<value optimized out>) at /home/cpeasyapache/src/php-5.6.31/Zend/zend_execute_API.c:831
#5  0x000000000077ae40 in call_user_function_ex (function_table=<value optimized out>, object_pp=<value optimized out>, function_name=<value optimized out>, 
    retval_ptr_ptr=<value optimized out>, param_count=<value optimized out>, params=<value optimized out>, no_separation=1, symbol_table=0x0)
    at /home/cpeasyapache/src/php-5.6.31/Zend/zend_execute_API.c:617
#6  0x0000000000788508 in zend_error (type=2, format=0xd77498 "%s() has been disabled for security reasons") at /home/cpeasyapache/src/php-5.6.31/Zend/zend.c:1223
#7  0x00000000008070f2 in zend_do_fcall_common_helper_SPEC (execute_data=<value optimized out>) at /home/cpeasyapache/src/php-5.6.31/Zend/zend_vm_execute.h:558
#8  0x00000000007f6980 in execute_ex (execute_data=0x7eff28497bb8) at /home/cpeasyapache/src/php-5.6.31/Zend/zend_vm_execute.h:363
#9  0x0000000000787c09 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /home/cpeasyapache/src/php-5.6.31/Zend/zend.c:1341
#10 0x0000000000726b1a in php_execute_script (primary_file=0x7ffdc345f1b0) at /home/cpeasyapache/src/php-5.6.31/main/main.c:2613
#11 0x000000000082ec70 in do_cli (argc=2, argv=0x1f49ae0) at /home/cpeasyapache/src/php-5.6.31/sapi/cli/php_cli.c:998
#12 0x000000000082f3f8 in main (argc=2, argv=0x1f49ae0) at /home/cpeasyapache/src/php-5.6.31/sapi/cli/php_cli.c:1382

Running through gdb to see what userland/method call/phpfile caused the error gave:

(gdb) print (char *)(executor_globals.function_state_ptr->function)->common.function_name
There is no member named function_state_ptr.
(gdb) print (char *)executor_globals.active_op_array->function_name
$1 = 0x7eff1cef7a50 "get_frame_context"
(gdb) print (char *)executor_globals.active_op_array->filename
$2 = 0x7eff1ce337d0 "/home/compandr/vendor/sentry/sentry/lib/Raven/Stacktrace.php"

Gradually debugging through Stacktrace eventually showed we had passthru disabled and the cli framework was using a call to passthru to run clear on the console.

[Jean85]

Thank you for finding and fixing such a nasty bug!

Can you please contribute a regression test too, so we will avoid having this bug pop up an other time in the future?

[Caffe1neAdd1ct]

@Jean85 The nature of a segmentation fault is a hard exit from PHP which would interesting to cover with a test.

The other obstacles would be limiting the test to only PHP 5.6 as >= 7 seems to be ok and being able to configure php.ini disable_functions for that test.

running a quick idea on the test script above:

var_dump(ini_get('disable_functions'));
ini_set('disable_functions', 'passthru');
var_dump(ini_get('disable_functions'));

results in

console.php:7:
string(72) "show_source, system, shell_exec, exec, popen, proc_open, allow_url_fopen"
console.php:9:
string(72) "show_source, system, shell_exec, exec, popen, proc_open, allow_url_fopen"

disable_functions can't be set inside the php process ...

[stayallive]

I don't think this is testable, a segfault is not very testable with PHPUnit. So I don't think we can test for this.

[ste93cry]

This should be fixed in the branch of the 2.0 version too. @Caffe1neAdd1ct Would you mind to open a new PR to address it?

[stayallive]

Why shouldn't this be fixes in 1.x @ste93cry?

[ste93cry]

I think you misunderstood what I've said. It should be fixed in both branches, not only in 1.x

[stayallive]

@ste93cry definitely did 👍 Sorry about that!

And ofcourse @Caffe1neAdd1ct, thanks for finding, reporting and fixing!

[Jean85]

@stayallive why it shouldn't be testable? A segfault is a test failure all the same. Calling the piece of code with a disabled function is enough to trigger the bug. So something like:

$this->assertNotEmpty(ini_get('disable_functions'));
$disabledFunction = ini_get('disable_functions')[0];
// make call to $disabledFunction to trigger code

...should be enough. The fact that PHP 7+ is fine on itself is good because it will mean that the test can remain, the fix will be removed once PHP 7+ will be required.

[Caffe1neAdd1ct]

My concern would be the testing process crashing due to the segfault, not try{} catchable, would need to run in a separate process and determine the result possibly?

Sure i'll try and have a look at raising a PR to 2.* shortly.

[Jean85]

Since you already committed the fix, making the tests fatal shouldn't be an issue. It would fatal only if someone removes your fix, so that test should warn him and teach him a good lesson 😄