Skip to content

Respect script execution time limit when invoking shutdown functions #5543

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Respect script execution time limit when invoking shutdown functions #5543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9fc273
Honor script time limit when calling shutdown functions
alexdowad May 6, 2020
7f35610
Add pcntl signals test
nikic May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,17 @@ int zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache) /
/* We must re-initialize function again */
fci_cache->function_handler = NULL;
}

/* This flag is regularly checked while running user functions, but not internal
* So see whether interrupt flag was set while the function was running... */
if (EG(vm_interrupt)) {
EG(vm_interrupt) = 0;
if (EG(timed_out)) {
zend_timeout();
} else if (zend_interrupt_function) {
zend_interrupt_function(EG(current_execute_data));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

After thinking about this a bit, I think instead of checking timed_out specifically here, we should do a VM interrupt check as in

php-src/Zend/zend_vm_def.h

Line 9206 in 5d93eab

ZEND_VM_HELPER(zend_interrupt_helper, ANY, ANY)
. After all the core issue here applies not just to internal timeouts, but also pcntl signal handling, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I think I have figured out what you mean...

pcntl sets a signal handler which sets vm_interrupt to 1. That causes execution to bounce into pcntl's zend_interrupt_function next time the VM checks the interrupt flag. But that only happens when the VM is running PHP bytecode.

If you are running native shutdown functions, and pcntl is enabled, and a signal comes along, it will set the vm_interrupt flag but pcntl's zend_interrupt_function will never be called.

Do you think pcntl really should care about what is happening during shutdown, though? Maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think pcntl really should care about what is happening during shutdown, though? Maybe?

During shutdown probably isn't important, but there are other cases where we could run into this, at least in theory. For example, consider something like array_map('expensive_internal_function', $array). This would allow a signal handler to be executed in the middle of the array_map, rather than at the end.

Maybe it's possible to test this with a combination of InfiniteIterator and iterator_apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha. Wow, I never thought of that. OK.

Copy link
Member

Choose a reason for hiding this comment

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

I've added an extra test in 7f35610. I was hoping to show that this leaks the time_nanosleep return value (which should be an array in this case), but due to the way array_map is implemented this doesn't happen...

I think we should be repeating the EG(exception) check from a few lines above if the VM interrupt is executed, to make sure that code not freeing the retval on exception doesn't leak...

}

zend_vm_stack_free_call_frame(call);
Expand Down
29 changes: 29 additions & 0 deletions ext/pcntl/tests/async_signals_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
--TEST--
Async signals in zend_call_function
--SKIPIF--
<?php
if (!extension_loaded("pcntl")) print "skip";
if (getenv("SKIP_SLOW_TESTS")) print "skip slow test";
?>
--FILE--
<?php

pcntl_async_signals(1);
pcntl_signal(SIGALRM, function($signo) {
throw new Exception("Alarm!");
});

pcntl_alarm(1);
try {
array_map(
'time_nanosleep',
array_fill(0, 360, 1),
array_fill(0, 360, 0)
);
} catch (Exception $e) {
echo $e->getMessage(), "\n";
}

?>
--EXPECT--
Alarm!
3 changes: 1 addition & 2 deletions tests/basic/timeout_variation_10.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ Timeout within shutdown function, variation
if (getenv("SKIP_SLOW_TESTS")) die("skip slow test");
if (PHP_OS_FAMILY !== "Windows") die("skip Windows only test");
?>
--XFAIL--
Missing timeout check in call_user_function
--FILE--
<?php

Expand All @@ -19,4 +17,5 @@ register_shutdown_function("sleep", 1);
shutdown happens after here
--EXPECTF--
shutdown happens after here

Fatal error: Maximum execution time of 1 second exceeded in %s on line %d
2 changes: 0 additions & 2 deletions tests/basic/timeout_variation_9.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ Timeout within shutdown function
if (getenv("SKIP_SLOW_TESTS")) die("skip slow test");
if (PHP_OS_FAMILY !== "Windows") die("skip Windows only test");
?>
--XFAIL--
Missing timeout check in call_user_function
--FILE--
<?php

Expand Down