Skip to content

Commit

Permalink
Skip dummy frames allocated on CPU stack of zend_call_function().
Browse files Browse the repository at this point in the history 
(Usage of "current_observed_frame" varible looks unsafe to me).
  • Loading branch information
@dstogov
dstogov committed Jan 26, 2021
1 parent a2dcd44 commit 094e1a8
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 2 deletions.
8 changes: 6 additions & 2 deletions Zend/zend_observer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,19 @@ ZEND_API void ZEND_FASTCALL zend_observer_fcall_end(
first_observed_frame = NULL;
current_observed_frame = NULL;
} else {
current_observed_frame = execute_data->prev_execute_data;
zend_execute_data *ex = execute_data->prev_execute_data;
while (ex && !ex->func) {
ex = ex->prev_execute_data;
}
current_observed_frame = ex;
}
}

ZEND_API void zend_observer_fcall_end_all(void)
{
zend_execute_data *ex = current_observed_frame;
while (ex != NULL) {
if (ex->func->type != ZEND_INTERNAL_FUNCTION) {
if (ex->func && ex->func->type != ZEND_INTERNAL_FUNCTION) {
zend_observer_fcall_end(ex, NULL);
}
ex = ex->prev_execute_data;
Expand Down
35 changes: 35 additions & 0 deletions ext/zend_test/tests/observer_error_05.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
Observer: End handlers fire after a userland fatal error
--SKIPIF--
<?php if (!extension_loaded('zend-test')) die('skip: zend-test extension required'); ?>

This comment has been minimized.

Sign in to view

Copy link
@SammyK

SammyK Jan 28, 2021 •
edited

Contributor

@dstogov This test is being skipped now that the extension was renamed to zend_test.

This comment has been minimized.

Sign in to view

Copy link
@dstogov

dstogov Jan 28, 2021

Author Member

thanks. It seems Nikita already fixed this,

This comment has been minimized.

Sign in to view

Copy link
@SammyK

SammyK Jan 28, 2021

Contributor

Ah excellent. Sorry I missed that. Nikita is so fast! :)
--INI--
zend_test.observer.enabled=1
zend_test.observer.observe_all=1
zend_test.observer.show_return_value=1
--FILE--
<?php
set_error_handler(function ($errno, $errstr, $errfile, $errline) {
trigger_error('Foo error', E_USER_ERROR);
});

function foo()
{
return $x; // warning
}

foo();

echo 'You should not see this.';
?>
--EXPECTF--
<!-- init '%s%eobserver_error_%d.php' -->
<file '%s%eobserver_error_%d.php'>
<!-- init foo() -->
<foo>
<!-- init {closure}() -->
<{closure}>

Fatal error: Foo error in %s on line %d
</{closure}:NULL>
</foo:NULL>
</file '%s%eobserver_error_%d.php'>

3 comments on commit 094e1a8

@TysonAndre
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test appears to be causing a failure on the azure mac tests - https://dev.azure.com/phpazuredevops/eb1f6f74-bc61-47a6-a414-0e3ff41c9bb4/_apis/build/builds/14422/logs/45
(noticed in #6655)

2021-01-31T19:18:42.3820320Z 
2021-01-31T19:18:42.3869400Z ========DIFF========
2021-01-31T19:18:42.3880160Z --
2021-01-31T19:18:42.3897100Z      
2021-01-31T19:18:42.3913250Z      Fatal error: Foo error in %s on line %d
2021-01-31T19:18:42.3920790Z          </{closure}:NULL>
2021-01-31T19:18:42.3935260Z �[1;32m010+ �[0m
2021-01-31T19:18:42.3941880Z �[1;32m011+ Termsig=11�[0m
2021-01-31T19:18:42.3944120Z �[1;31m010-   </foo:NULL>�[0m
2021-01-31T19:18:42.3950700Z �[1;31m011- </file '%s%eobserver_error_%d.php'>�[0m
2021-01-31T19:18:42.3963560Z ========DONE========
2021-01-31T19:18:42.3983230Z �[1;31mFAIL�[0m Observer: End handlers fire after a userland fatal error [ext/zend_test/tests/observer_error_05.phpt]

@nikic
Copy link
Member

@nikic nikic commented on 094e1a8 Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately the "end_all" handling is unsafe due to stack-allocated frames. We should remove the bailout handling entirely and let the extension deal with it.

@patrickallaert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately the "end_all" handling is unsafe due to stack-allocated frames. We should remove the bailout handling entirely and let the extension deal with it.

Would that be the reason for a crash like this on ARM64?

#0  0x0000aaaabb1136f4 in zend_observer_fcall_end_all ()
    at /home/iamluc/php-src/Zend/zend_observer.c:235
#1  0x0000aaaabaf613b4 in php_request_shutdown (dummy=0x0) at /home/iamluc/php-src/main/main.c:1745
#2  0x0000aaaabb11f4c0 in php_cli_server_request_shutdown (server=0xaaaabbaec8d8 <server>, 
    client=0xaaaad809d830) at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2122
#3  0x0000aaaabb11f81c in php_cli_server_dispatch (server=0xaaaabbaec8d8 <server>, 
    client=0xaaaad809d830) at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2200
#4  0x0000aaaabb120334 in php_cli_server_recv_event_read_request (server=0xaaaabbaec8d8 <server>, 
    client=0xaaaad809d830) at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2513
#5  0x0000aaaabb1206c0 in php_cli_server_do_event_for_each_fd_callback (_params=0xffffdbbba4e8, 
    fd=4, event=1) at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2599
#6  0x0000aaaabb11c3f4 in php_cli_server_poller_iter_on_active (poller=0xaaaabbaec8e0 <server+8>, 
    opaque=0xffffdbbba4e8, callback=0xaaaabb12049c <php_cli_server_do_event_for_each_fd_callback>)
    at /home/iamluc/php-src/sapi/cli/php_cli_server.c:869
#7  0x0000aaaabb120740 in php_cli_server_do_event_for_each_fd (server=0xaaaabbaec8d8 <server>, 
    rhandler=0xaaaabb1201d8 <php_cli_server_recv_event_read_request>, 
    whandler=0xaaaabb120364 <php_cli_server_send_event>)
    at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2617
#8  0x0000aaaabb1207ac in php_cli_server_do_event_loop (server=0xaaaabbaec8d8 <server>)
    at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2627
#9  0x0000aaaabb120b9c in do_cli_server (argc=11, argv=0xaaaad7eb00c0)
    at /home/iamluc/php-src/sapi/cli/php_cli_server.c:2757
#10 0x0000aaaabb116398 in main (argc=11, argv=0xaaaad7eb00c0)
    at /home/iamluc/php-src/sapi/cli/php_cli.c:1339

The crash happens at this very line: if (ex->func && ex->func->type != ZEND_INTERNAL_FUNCTION) {.

Please sign in to comment.