Skip to content

Fix #72409 - return previous handler #1985

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 4 commits into from
Closed

Fix #72409 - return previous handler #1985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4a2190
Fix #72409 - return previous handler
bp1222 Jul 2, 2016
3e80b15
Fix to not attempt to call if handler is long. Idea for this came
bp1222 Jul 6, 2016
3182f07
change long to zend_long, for uniformity
bp1222 Jul 6, 2016
5da0187
oops, name test right
bp1222 Jul 6, 2016
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
48 changes: 39 additions & 9 deletions ext/pcntl/pcntl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_pcntl_signal, 0, 0, 2)
ZEND_ARG_INFO(0, restart_syscalls)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_pcntl_signal_get_handler, 0, 0, 1)
ZEND_ARG_INFO(0, signo)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_pcntl_sigprocmask, 0, 0, 2)
ZEND_ARG_INFO(0, how)
ZEND_ARG_INFO(0, set)
Expand Down Expand Up @@ -155,6 +159,7 @@ const zend_function_entry pcntl_functions[] = {
PHP_FE(pcntl_waitpid, arginfo_pcntl_waitpid)
PHP_FE(pcntl_wait, arginfo_pcntl_wait)
PHP_FE(pcntl_signal, arginfo_pcntl_signal)
PHP_FE(pcntl_signal_get_handler, arginfo_pcntl_signal_get_handler)
PHP_FE(pcntl_signal_dispatch, arginfo_pcntl_void)
PHP_FE(pcntl_wifexited, arginfo_pcntl_wifexited)
PHP_FE(pcntl_wifstopped, arginfo_pcntl_wifstopped)
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

pcntl_signal(SIGINT, "some_handler"); /* recorded into the lookup table */
/* do something else */
/* get SIGINT from outside, it has landed in the queue, but not yet handled */
pcntl_signal(SIGINT, SIG_IGN); /* the handler is an int */

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bp1222 looks fine for what i'd care. Thanks.

RETURN_TRUE;
}

Expand All @@ -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) {
Copy link
Member

@laruence laruence Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's better to use NSIG instead of hardcode 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

php_error_docref(NULL, E_WARNING, "Invalid signal");
RETURN_FALSE;
}

if ((prev_handle = zend_hash_index_find(&PCNTL_G(php_signal_table), signo)) != NULL) {
RETURN_ZVAL(prev_handle, 1, 0);
} else {
RETURN_LONG((zend_long)SIG_DFL);
}
}

/* {{{ proto bool pcntl_signal_dispatch()
Dispatch signals to signal handlers */
PHP_FUNCTION(pcntl_signal_dispatch)
Expand Down Expand Up @@ -1350,14 +1378,16 @@ void pcntl_signal_dispatch()

while (queue) {
if ((handle = zend_hash_index_find(&PCNTL_G(php_signal_table), queue->signo)) != NULL) {
ZVAL_NULL(&retval);
ZVAL_LONG(&param, queue->signo);

/* Call php signal handler - Note that we do not report errors, and we ignore the return value */
/* FIXME: this is probably broken when multiple signals are handled in this while loop (retval) */
call_user_function(EG(function_table), NULL, handle, &retval, 1, &param);
zval_ptr_dtor(&param);
zval_ptr_dtor(&retval);
if (Z_TYPE_P(handle) != IS_LONG) {
ZVAL_NULL(&retval);
ZVAL_LONG(&param, queue->signo);

/* Call php signal handler - Note that we do not report errors, and we ignore the return value */
/* FIXME: this is probably broken when multiple signals are handled in this while loop (retval) */
call_user_function(EG(function_table), NULL, handle, &retval, 1, &param);
zval_ptr_dtor(&param);
zval_ptr_dtor(&retval);
}
}

next = queue->next;
Expand Down
1 change: 1 addition & 0 deletions ext/pcntl/php_pcntl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ PHP_FUNCTION(pcntl_wexitstatus);
PHP_FUNCTION(pcntl_wtermsig);
PHP_FUNCTION(pcntl_wstopsig);
PHP_FUNCTION(pcntl_signal);
PHP_FUNCTION(pcntl_signal_get_handler);
PHP_FUNCTION(pcntl_signal_dispatch);
PHP_FUNCTION(pcntl_get_last_error);
PHP_FUNCTION(pcntl_strerror);
Expand Down
32 changes: 32 additions & 0 deletions ext/pcntl/tests/pcntl_signal_get_handler.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
pcntl_signal_get_handler()
--SKIPIF--
<?php if (!extension_loaded("pcntl")) print "skip"; ?>
<?php if (!extension_loaded("posix")) die("skip posix extension not available"); ?>
--FILE--
<?php
var_dump(pcntl_signal_get_handler(SIGUSR1));

function pcntl_test($signo) {}
pcntl_signal(SIGUSR1, 'pcntl_test');
var_dump(pcntl_signal_get_handler(SIGUSR1));

pcntl_signal(SIGUSR1, SIG_IGN);
var_dump(pcntl_signal_get_handler(SIGUSR1));

pcntl_signal(SIGUSR1, SIG_DFL);
var_dump(pcntl_signal_get_handler(SIGUSR1));

posix_kill(posix_getpid(), SIGUSR1);
pcntl_signal_dispatch();

echo "ok\n";
?>
--EXPECTF--
int(0)
string(10) "pcntl_test"
int(1)
int(0)
User defined signal 1

Termsig=10