Skip to content
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.

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

ext/standard: Refactor tick and shutdown functions #17295

Merged
merged 2 commits into from
Dec 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 17 additions & 21 deletions ext/session/session.c
Original file line number Diff line number Diff line change
Expand Up @@ -2175,19 +2175,17 @@ PHP_FUNCTION(session_set_save_handler)

if (register_shutdown) {
/* create shutdown function */
php_shutdown_function_entry shutdown_function_entry;
zval callable;
zend_result result;

ZVAL_STRING(&callable, "session_register_shutdown");
result = zend_fcall_info_init(&callable, 0, &shutdown_function_entry.fci,
&shutdown_function_entry.fci_cache, NULL, NULL);

ZEND_ASSERT(result == SUCCESS);
php_shutdown_function_entry shutdown_function_entry = {
.fci_cache = empty_fcall_info_cache,
.params = NULL,
.param_count = 0,
};
zend_function *fn_entry = zend_hash_str_find_ptr(CG(function_table), ZEND_STRL("session_register_shutdown"));
ZEND_ASSERT(fn_entry != NULL);
shutdown_function_entry.fci_cache.function_handler = fn_entry;

/* add shutdown function, removing the old one if it exists */
if (!register_user_shutdown_function("session_shutdown", strlen("session_shutdown"), &shutdown_function_entry)) {
zval_ptr_dtor(&callable);
if (!register_user_shutdown_function(ZEND_STRL("session_shutdown"), &shutdown_function_entry)) {
php_error_docref(NULL, E_WARNING, "Unable to register session shutdown function");
RETURN_FALSE;
}
Expand Down Expand Up @@ -2826,9 +2824,11 @@ PHP_FUNCTION(session_status)
/* {{{ Registers session_write_close() as a shutdown function */
PHP_FUNCTION(session_register_shutdown)
{
php_shutdown_function_entry shutdown_function_entry;
zval callable;
zend_result result;
php_shutdown_function_entry shutdown_function_entry = {
.fci_cache = empty_fcall_info_cache,
.params = NULL,
.param_count = 0,
};

ZEND_PARSE_PARAMETERS_NONE();

Expand All @@ -2838,15 +2838,11 @@ PHP_FUNCTION(session_register_shutdown)
* function after calling session_set_save_handler(), which expects
* the session still to be available.
*/
ZVAL_STRING(&callable, "session_write_close");
result = zend_fcall_info_init(&callable, 0, &shutdown_function_entry.fci,
&shutdown_function_entry.fci_cache, NULL, NULL);

ZEND_ASSERT(result == SUCCESS);
zend_function *fn_entry = zend_hash_str_find_ptr(CG(function_table), ZEND_STRL("session_write_close"));
ZEND_ASSERT(fn_entry != NULL);
shutdown_function_entry.fci_cache.function_handler = fn_entry;

if (!append_user_shutdown_function(&shutdown_function_entry)) {
zval_ptr_dtor(&callable);

/* Unable to register shutdown function, presumably because of lack
* of memory, so flush the session now. It would be done in rshutdown
* anyway but the handler will have had it's dtor called by then.
Expand Down
133 changes: 62 additions & 71 deletions ext/standard/basic_functions.c
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ PHPAPI php_basic_globals basic_globals;
#endif

typedef struct _user_tick_function_entry {
zend_fcall_info fci;
zend_fcall_info_cache fci_cache;
zval *params;
uint32_t param_count;
bool calling;
} user_tick_function_entry;

Expand Down Expand Up @@ -1577,51 +1578,34 @@ PHP_FUNCTION(forward_static_call_array)
}
/* }}} */

static void fci_addref(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache)
{
Z_TRY_ADDREF(fci->function_name);
if (fci_cache->object) {
GC_ADDREF(fci_cache->object);
}
}

static void fci_release(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache)
{
zval_ptr_dtor(&fci->function_name);
if (fci_cache->object) {
zend_object_release(fci_cache->object);
}
}

void user_shutdown_function_dtor(zval *zv) /* {{{ */
{
php_shutdown_function_entry *shutdown_function_entry = Z_PTR_P(zv);

zend_fcall_info_args_clear(&shutdown_function_entry->fci, true);
fci_release(&shutdown_function_entry->fci, &shutdown_function_entry->fci_cache);
for (uint32_t i = 0; i < shutdown_function_entry->param_count; i++) {
zval_ptr_dtor(&shutdown_function_entry->params[i]);
}
efree(shutdown_function_entry->params);
zend_fcc_dtor(&shutdown_function_entry->fci_cache);
efree(shutdown_function_entry);
}
/* }}} */

void user_tick_function_dtor(user_tick_function_entry *tick_function_entry) /* {{{ */
{
zend_fcall_info_args_clear(&tick_function_entry->fci, true);
fci_release(&tick_function_entry->fci, &tick_function_entry->fci_cache);
for (uint32_t i = 0; i < tick_function_entry->param_count; i++) {
zval_ptr_dtor(&tick_function_entry->params[i]);
}
efree(tick_function_entry->params);
zend_fcc_dtor(&tick_function_entry->fci_cache);
}
/* }}} */

static int user_shutdown_function_call(zval *zv) /* {{{ */
{
php_shutdown_function_entry *shutdown_function_entry = Z_PTR_P(zv);
zval retval;
zend_result call_status;

/* set retval zval for FCI struct */
shutdown_function_entry->fci.retval = &retval;
call_status = zend_call_function(&shutdown_function_entry->fci, &shutdown_function_entry->fci_cache);
ZEND_ASSERT(call_status == SUCCESS);
zval_ptr_dtor(&retval);
php_shutdown_function_entry *entry = Z_PTR_P(zv);

zend_call_known_fcc(&entry->fci_cache, NULL, entry->param_count, entry->params, NULL);
return 0;
}
/* }}} */
Expand All @@ -1630,16 +1614,8 @@ static void user_tick_function_call(user_tick_function_entry *tick_fe) /* {{{ */
{
/* Prevent re-entrant calls to the same user ticks function */
if (!tick_fe->calling) {
zval tmp;

/* set tmp zval */
tick_fe->fci.retval = &tmp;

tick_fe->calling = true;
zend_call_function(&tick_fe->fci, &tick_fe->fci_cache);

/* Destroy return value */
zval_ptr_dtor(&tmp);
zend_call_known_fcc(&tick_fe->fci_cache, NULL, tick_fe->param_count, tick_fe->params, NULL);
tick_fe->calling = false;
}
}
Expand All @@ -1653,25 +1629,13 @@ static void run_user_tick_functions(int tick_count, void *arg) /* {{{ */

static int user_tick_function_compare(user_tick_function_entry * tick_fe1, user_tick_function_entry * tick_fe2) /* {{{ */
{
zval *func1 = &tick_fe1->fci.function_name;
zval *func2 = &tick_fe2->fci.function_name;
int ret;

if (Z_TYPE_P(func1) == IS_STRING && Z_TYPE_P(func2) == IS_STRING) {
ret = zend_binary_zval_strcmp(func1, func2) == 0;
} else if (Z_TYPE_P(func1) == IS_ARRAY && Z_TYPE_P(func2) == IS_ARRAY) {
ret = zend_compare_arrays(func1, func2) == 0;
} else if (Z_TYPE_P(func1) == IS_OBJECT && Z_TYPE_P(func2) == IS_OBJECT) {
ret = zend_compare_objects(func1, func2) == 0;
} else {
ret = 0;
}
bool is_equal = zend_fcc_equals(&tick_fe1->fci_cache, &tick_fe2->fci_cache);

if (ret && tick_fe1->calling) {
if (is_equal && tick_fe1->calling) {
zend_throw_error(NULL, "Registered tick function cannot be unregistered while it is being executed");
return 0;
return false;
}
return ret;
return is_equal;
}
/* }}} */

Expand Down Expand Up @@ -1703,17 +1667,27 @@ PHPAPI void php_free_shutdown_functions(void) /* {{{ */
/* {{{ Register a user-level function to be called on request termination */
PHP_FUNCTION(register_shutdown_function)
{
php_shutdown_function_entry entry;
zend_fcall_info fci;
php_shutdown_function_entry entry = {
.fci_cache = empty_fcall_info_cache,
.params = NULL,
.param_count = 0,
};
zval *params = NULL;
uint32_t param_count = 0;
bool status;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &entry.fci, &entry.fci_cache, &params, &param_count) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "F*", &fci, &entry.fci_cache, &params, &entry.param_count) == FAILURE) {
RETURN_THROWS();
}

fci_addref(&entry.fci, &entry.fci_cache);
zend_fcall_info_argp(&entry.fci, param_count, params);
zend_fcc_addref(&entry.fci_cache);
if (entry.param_count) {
ZEND_ASSERT(params != NULL);
entry.params = (zval *) safe_emalloc(entry.param_count, sizeof(zval), 0);
for (uint32_t i = 0; i < entry.param_count; i++) {
ZVAL_COPY(&entry.params[i], &params[i]);
}
}

status = append_user_shutdown_function(&entry);
ZEND_ASSERT(status);
Expand Down Expand Up @@ -2285,17 +2259,27 @@ PHP_FUNCTION(getprotobynumber)
/* {{{ Registers a tick callback function */
PHP_FUNCTION(register_tick_function)
{
user_tick_function_entry tick_fe;
user_tick_function_entry tick_fe = {
.fci_cache = empty_fcall_info_cache,
.params = NULL,
.param_count = 0,
.calling = false,
};
zend_fcall_info fci;
zval *params = NULL;
uint32_t param_count = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &tick_fe.fci, &tick_fe.fci_cache, &params, &param_count) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "F*", &fci, &tick_fe.fci_cache, &params, &tick_fe.param_count) == FAILURE) {
Copy link
Member

Choose a reason for hiding this comment

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

The change from f to F means that the function never worked for trampolines, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It did because the callable was stored in the FCI, and it was fetched at each call from the function_name zval of the FCI.
I did think this as well at the beginning and so wrote the tests first.

RETURN_THROWS();
}

tick_fe.calling = false;
fci_addref(&tick_fe.fci, &tick_fe.fci_cache);
zend_fcall_info_argp(&tick_fe.fci, param_count, params);
zend_fcc_addref(&tick_fe.fci_cache);
if (tick_fe.param_count) {
ZEND_ASSERT(params != NULL);
tick_fe.params = (zval *) safe_emalloc(tick_fe.param_count, sizeof(zval), 0);
for (uint32_t i = 0; i < tick_fe.param_count; i++) {
ZVAL_COPY(&tick_fe.params[i], &params[i]);
}
}

if (!BG(user_tick_functions)) {
BG(user_tick_functions) = (zend_llist *) emalloc(sizeof(zend_llist));
Expand All @@ -2314,17 +2298,24 @@ PHP_FUNCTION(register_tick_function)
/* {{{ Unregisters a tick callback function */
PHP_FUNCTION(unregister_tick_function)
{
user_tick_function_entry tick_fe;
user_tick_function_entry tick_fe = {
.fci_cache = empty_fcall_info_cache,
.params = NULL,
.param_count = 0,
.calling = false,
};
zend_fcall_info fci;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_FUNC(tick_fe.fci, tick_fe.fci_cache)
Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(fci, tick_fe.fci_cache)
ZEND_PARSE_PARAMETERS_END();

if (!BG(user_tick_functions)) {
return;
if (BG(user_tick_functions)) {
zend_llist_del_element(BG(user_tick_functions), &tick_fe, (int (*)(void *, void *)) user_tick_function_compare);
}

zend_llist_del_element(BG(user_tick_functions), &tick_fe, (int (*)(void *, void *)) user_tick_function_compare);
/* Free potential trampoline */
zend_release_fcall_info_cache(&tick_fe.fci_cache);
}
/* }}} */

Expand Down
3 changes: 2 additions & 1 deletion ext/standard/basic_functions.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,9 @@ PHPAPI double php_get_nan(void);
PHPAPI double php_get_inf(void);

typedef struct _php_shutdown_function_entry {
zend_fcall_info fci;
zend_fcall_info_cache fci_cache;
zval *params;
uint32_t param_count;
} php_shutdown_function_entry;

PHPAPI extern bool register_user_shutdown_function(const char *function_name, size_t function_len, php_shutdown_function_entry *shutdown_function_entry);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
--TEST--
Use a trampoline as a shutdown function
--FILE--
<?php

class TrampolineTest {
public function __call(string $name, array $arguments) {
echo 'Trampoline for ', $name, PHP_EOL;
var_dump($arguments);
}
}
$o = new TrampolineTest();
$callback = [$o, 'shutdownTrampoline'];

echo "Before registering\n";

register_shutdown_function($callback, "arg1", "arg2");

echo "After registering\n";
?>
--EXPECT--
Before registering
After registering
Trampoline for shutdownTrampoline
array(2) {
[0]=>
string(4) "arg1"
[1]=>
string(4) "arg2"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
Unregistering a trampoline as a tick function
--FILE--
<?php
declare(ticks=1);

class TrampolineTest {
public function __call(string $name, array $arguments) {
echo 'Trampoline for ', $name, PHP_EOL;
var_dump($arguments);
}
}
$o = new TrampolineTest();
$callback = [$o, 'trampoline'];

unregister_tick_function($callback);

?>
OK
--EXPECT--
OK
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
Use a trampoline as a tick function
--FILE--
<?php
declare(ticks=1);

class TrampolineTest {
public function __call(string $name, array $arguments) {
echo 'Trampoline for ', $name, PHP_EOL;
var_dump($arguments);
}
}
$o = new TrampolineTest();
$callback = [$o, 'trampoline'];

register_tick_function($callback, "arg1", "arg2");

echo "Tick function should run\n";

unregister_tick_function($callback);

echo "Tick function should be removed and not run\n";
$o->notTickTrampoline("not in tick");

?>
--EXPECT--
Trampoline for trampoline
array(2) {
[0]=>
string(4) "arg1"
[1]=>
string(4) "arg2"
}
Tick function should run
Trampoline for trampoline
array(2) {
[0]=>
string(4) "arg1"
[1]=>
string(4) "arg2"
}
Tick function should be removed and not run
Trampoline for notTickTrampoline
array(1) {
[0]=>
string(11) "not in tick"
}
Loading