Fix #72409 - return previous handler #1985
4 participants
| @@ -1012,6 +1017,29 @@ PHP_FUNCTION(pcntl_signal) | ||
| } | ||
| /* }}} */ | ||
| +/* {{{ proto bool pcntl_signal_get_handler(int signo) | ||
| + Gets signal handler */ | ||
| +PHP_FUNCTION(pcntl_signal_get_handler) | ||
| +{ | ||
| + zval *prev_handle; | ||
| + zend_long signo; | ||
| + | ||
| + if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &signo) == FAILURE) { | ||
| + return; | ||
| + } | ||
| + | ||
| + if (signo < 1 || signo > 32) { |
|
I think it's better to use NSIG instead of hardcode 32 I was copy-pasting that via pcntl_signal(). The problem with using NSIG is that on my box(linux 4.4.0, libc 2.22-4) it's value is 64, however 32 is the largest defined signal. That said, NSIG on FreeBSD is defined to 32, even though it has SIGLIBRT defined at 33. So, I'm not sure what happens if you're to sigaction() a signal that's not defined in signal.h, or if in general we might look to be smarter about what is the highest signal we support per-platform. Regardless, this could be something addressed in a distinct PR trying to abstract and remove the hard-coded values.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| @@ -986,7 +991,7 @@ PHP_FUNCTION(pcntl_signal) | ||
| php_error_docref(NULL, E_WARNING, "Error assigning signal"); | ||
| RETURN_FALSE; | ||
| } | ||
| - zend_hash_index_del(&PCNTL_G(php_signal_table), signo); | ||
| + zend_hash_index_update(&PCNTL_G(php_signal_table), signo, handle); |
|
handle is not a calable but integer at this point. Could you please explain why this needs to be updated in this case? I guess a non callable should not be added to the signal mapping. Thanks. Since a user can set a signal to be handled by \SIG_ING we would need to take note of it, for the new function to acquire the handler. I guess we wouldn't need to save \SIG_DFL but, that is why I changed this from _del to _update Although, this does make me think about the handling side in pcntl_signal_dispatch. No reason to attempt to call the handler, if it is a long. Ah, i see. Need that so the new function can return it. Looks fine then, The recorded zval won't be found in the lookup table, as the signal won't be delivered into the queue. Seems fine. Thanks. Yes, that was my concern, it is tricky, After thinking a bit more, probably it should be ensured that pcntl_signal_dispatch() doesn't handle an integer as a function. Possible erroneous scenario By this state, it could be possible that to the time of dispatching signals, call_user_function gets an integer instead fo callable. Not sure how to phpt test this, would be yet more tricky :) Thanks. Well, zend_call_function() will properly make sure that we're dealing with a callable and fail gracefully. That said, it's simple (already added) a check to make sure the handle isn't a long.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
@trowski about the question from the other PR - if this PR gets accepted in 7.1, i don't mind a backport. Though please lets make the gap a bit longer, letting it to go through 7.1 QA first (still it would be beta). Then target 7.0.11 if everything is ok, so merging into 7.0 like after August 4th.
Thanks.
Comment on behalf of trowski at php.net:
Merged into master and will be included in 7.1.0-beta1. I'll merge the changes down into 7.0 later as suggested.
This patch addresses https://bugs.php.net/bug.php?id=72409
This patch is applied to master compared to PR #1952 which was
patching 5.6.x branch of PHP
This patch takes into account discussions on PR #1978
Addressing that rather than have pcntl_signal() return a value
to create a new function that can be used to get the current
value of the signal handler.