From 5175b3f25d740115f195be2b1914d83aed283a8a Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 9 Jul 2020 15:02:05 +0200 Subject: [PATCH 1/6] Refactor register_tick_function mechanism --- ext/standard/basic_functions.c | 100 +++++------------- .../register_tick_function_error.phpt | 2 +- .../unregister_tick_function_error.phpt | 14 +++ 3 files changed, 42 insertions(+), 74 deletions(-) create mode 100644 ext/standard/tests/general_functions/unregister_tick_function_error.phpt diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 04bc0a848de54..ba960225274e5 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -110,9 +110,9 @@ PHPAPI php_basic_globals basic_globals; #include "basic_functions_arginfo.h" typedef struct _user_tick_function_entry { - zval *arguments; - int arg_count; - int calling; + zend_fcall_info fci; + zend_fcall_info_cache fci_cache; + bool calling; } user_tick_function_entry; /* some prototypes for local functions */ @@ -1678,12 +1678,9 @@ void user_shutdown_function_dtor(zval *zv) /* {{{ */ void user_tick_function_dtor(user_tick_function_entry *tick_function_entry) /* {{{ */ { - int i; + zval_ptr_dtor(&tick_function_entry->fci.function_name); - for (i = 0; i < tick_function_entry->arg_count; i++) { - zval_ptr_dtor(&tick_function_entry->arguments[i]); - } - efree(tick_function_entry->arguments); + efree(tick_function_entry); } /* }}} */ @@ -1713,27 +1710,12 @@ static int user_shutdown_function_call(zval *zv) /* {{{ */ static void user_tick_function_call(user_tick_function_entry *tick_fe) /* {{{ */ { - zval retval; - zval *function = &tick_fe->arguments[0]; - - /* Prevent reentrant calls to the same user ticks function */ - if (! tick_fe->calling) { - tick_fe->calling = 1; - - if (call_user_function(NULL, NULL, - function, - &retval, - tick_fe->arg_count - 1, - tick_fe->arguments + 1 - ) == SUCCESS) { - zval_ptr_dtor(&retval); - } else { - zend_string *function_name = zend_get_callable_name(function); - zend_throw_error(NULL, "Registered tick function %s() cannot be called, function does not exist", ZSTR_VAL(function_name)); - zend_string_release(function_name); - } - - tick_fe->calling = 0; + /* Prevent re-entrant calls to the same user ticks function */ + if (!tick_fe->calling) { + tick_fe->calling = true; + zend_fcall_info_call(&tick_fe->fci, &tick_fe->fci_cache, tick_fe->fci.retval, NULL); + zend_release_fcall_info_cache(&tick_fe->fci_cache); + tick_fe->calling = false; } } /* }}} */ @@ -1746,8 +1728,8 @@ 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->arguments[0]; - zval *func2 = &tick_fe2->arguments[0]; + 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) { @@ -2386,36 +2368,15 @@ PHP_FUNCTION(getprotobynumber) /* {{{ Registers a tick callback function */ PHP_FUNCTION(register_tick_function) { - user_tick_function_entry tick_fe; - int i; - zend_string *function_name = NULL; - - tick_fe.calling = 0; - tick_fe.arg_count = ZEND_NUM_ARGS(); - - if (tick_fe.arg_count < 1) { - WRONG_PARAM_COUNT; - } - - tick_fe.arguments = (zval *) safe_emalloc(sizeof(zval), tick_fe.arg_count, 0); + user_tick_function_entry *tick_fe = emalloc(sizeof(user_tick_function_entry)); - if (zend_get_parameters_array(ZEND_NUM_ARGS(), tick_fe.arg_count, tick_fe.arguments) == FAILURE) { - efree(tick_fe.arguments); - RETURN_FALSE; - } - - if (!zend_is_callable(&tick_fe.arguments[0], 0, &function_name)) { - efree(tick_fe.arguments); - zend_argument_type_error(1, "must be a valid tick callback, \"%s\" given", ZSTR_VAL(function_name)); - zend_string_release_ex(function_name, 0); + if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &tick_fe->fci, &tick_fe->fci_cache, + &tick_fe->fci.params, &tick_fe->fci.param_count) == FAILURE) { + efree(tick_fe); RETURN_THROWS(); - } else if (function_name) { - zend_string_release_ex(function_name, 0); } - if (Z_TYPE(tick_fe.arguments[0]) != IS_ARRAY && Z_TYPE(tick_fe.arguments[0]) != IS_OBJECT) { - convert_to_string(&tick_fe.arguments[0]); - } + Z_TRY_ADDREF(tick_fe->fci.function_name); if (!BG(user_tick_functions)) { BG(user_tick_functions) = (zend_llist *) emalloc(sizeof(zend_llist)); @@ -2425,11 +2386,8 @@ PHP_FUNCTION(register_tick_function) php_add_tick_function(run_user_tick_functions, NULL); } - for (i = 0; i < tick_fe.arg_count; i++) { - Z_TRY_ADDREF(tick_fe.arguments[i]); - } - - zend_llist_add_element(BG(user_tick_functions), &tick_fe); + zend_llist_add_element(BG(user_tick_functions), tick_fe); + efree(tick_fe); RETURN_TRUE; } @@ -2438,23 +2396,19 @@ PHP_FUNCTION(register_tick_function) /* {{{ Unregisters a tick callback function */ PHP_FUNCTION(unregister_tick_function) { - user_tick_function_entry tick_fe; - zend_fcall_info fci; - zend_fcall_info_cache fci_cache; + user_tick_function_entry *tick_fe = emalloc(sizeof(user_tick_function_entry)); - ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_FUNC(fci, fci_cache) - ZEND_PARSE_PARAMETERS_END(); + if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &tick_fe->fci, &tick_fe->fci_cache) == FAILURE) { + efree(tick_fe); + RETURN_THROWS(); + } if (!BG(user_tick_functions)) { return; } - tick_fe.arguments = (zval *) emalloc(sizeof(zval)); - ZVAL_COPY_VALUE(&tick_fe.arguments[0], &fci.function_name); - tick_fe.arg_count = 1; - zend_llist_del_element(BG(user_tick_functions), &tick_fe, (int (*)(void *, void *)) user_tick_function_compare); - efree(tick_fe.arguments); + zend_llist_del_element(BG(user_tick_functions), tick_fe, (int (*)(void *, void *)) user_tick_function_compare); + efree(tick_fe); } /* }}} */ diff --git a/ext/standard/tests/general_functions/register_tick_function_error.phpt b/ext/standard/tests/general_functions/register_tick_function_error.phpt index adcc20215ed91..9eae12c5b6925 100644 --- a/ext/standard/tests/general_functions/register_tick_function_error.phpt +++ b/ext/standard/tests/general_functions/register_tick_function_error.phpt @@ -11,4 +11,4 @@ try { } ?> --EXPECT-- -register_tick_function(): Argument #1 ($callback) must be a valid tick callback, "a" given +register_tick_function(): Argument #1 ($callback) must be a valid callback, function "a" not found or invalid function name diff --git a/ext/standard/tests/general_functions/unregister_tick_function_error.phpt b/ext/standard/tests/general_functions/unregister_tick_function_error.phpt new file mode 100644 index 0000000000000..fd96a4658faa7 --- /dev/null +++ b/ext/standard/tests/general_functions/unregister_tick_function_error.phpt @@ -0,0 +1,14 @@ +--TEST-- +unregister_tick_function only accepts a valid callback as parameter +--FILE-- +getMessage() . "\n"; +} +?> +--EXPECT-- +unregister_tick_function(): Argument #1 ($callback) must be a valid callback, function "a" not found or invalid function name From a63386d55b60c94729e2965e9206f66db08eb2ee Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 7 May 2021 13:12:08 +0100 Subject: [PATCH 2/6] Add ref for callable params --- ext/standard/basic_functions.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index ba960225274e5..00f857cfd83d4 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1679,6 +1679,9 @@ void user_shutdown_function_dtor(zval *zv) /* {{{ */ void user_tick_function_dtor(user_tick_function_entry *tick_function_entry) /* {{{ */ { zval_ptr_dtor(&tick_function_entry->fci.function_name); + for (size_t i = 0; i < tick_function_entry->fci.param_count; i++) { + zval_ptr_dtor(&tick_function_entry->fci.params[i]); + } efree(tick_function_entry); } @@ -2376,8 +2379,11 @@ PHP_FUNCTION(register_tick_function) RETURN_THROWS(); } + tick_fe->calling = false; Z_TRY_ADDREF(tick_fe->fci.function_name); - + for (size_t i = 0; i < tick_fe->fci.param_count; i++) { + Z_TRY_ADDREF(tick_fe->fci.params[i]); + } if (!BG(user_tick_functions)) { BG(user_tick_functions) = (zend_llist *) emalloc(sizeof(zend_llist)); zend_llist_init(BG(user_tick_functions), From 2433381d9b0cf0757d3cb008d357dde5827c748f Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 7 May 2021 14:01:22 +0100 Subject: [PATCH 3/6] Drop heap allocation --- ext/standard/basic_functions.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 00f857cfd83d4..fd74548237dfe 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -2371,18 +2371,17 @@ PHP_FUNCTION(getprotobynumber) /* {{{ Registers a tick callback function */ PHP_FUNCTION(register_tick_function) { - user_tick_function_entry *tick_fe = emalloc(sizeof(user_tick_function_entry)); + user_tick_function_entry tick_fe; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &tick_fe->fci, &tick_fe->fci_cache, - &tick_fe->fci.params, &tick_fe->fci.param_count) == FAILURE) { - efree(tick_fe); + if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &tick_fe.fci, &tick_fe.fci_cache, + &tick_fe.fci.params, &tick_fe.fci.param_count) == FAILURE) { RETURN_THROWS(); } - tick_fe->calling = false; - Z_TRY_ADDREF(tick_fe->fci.function_name); - for (size_t i = 0; i < tick_fe->fci.param_count; i++) { - Z_TRY_ADDREF(tick_fe->fci.params[i]); + tick_fe.calling = false; + Z_TRY_ADDREF(tick_fe.fci.function_name); + for (size_t i = 0; i < tick_fe.fci.param_count; i++) { + Z_TRY_ADDREF(tick_fe.fci.params[i]); } if (!BG(user_tick_functions)) { BG(user_tick_functions) = (zend_llist *) emalloc(sizeof(zend_llist)); @@ -2392,8 +2391,7 @@ PHP_FUNCTION(register_tick_function) php_add_tick_function(run_user_tick_functions, NULL); } - zend_llist_add_element(BG(user_tick_functions), tick_fe); - efree(tick_fe); + zend_llist_add_element(BG(user_tick_functions), &tick_fe); RETURN_TRUE; } @@ -2402,19 +2400,17 @@ PHP_FUNCTION(register_tick_function) /* {{{ Unregisters a tick callback function */ PHP_FUNCTION(unregister_tick_function) { - user_tick_function_entry *tick_fe = emalloc(sizeof(user_tick_function_entry)); + user_tick_function_entry tick_fe; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &tick_fe->fci, &tick_fe->fci_cache) == FAILURE) { - efree(tick_fe); - RETURN_THROWS(); - } + ZEND_PARSE_PARAMETERS_START(1, 1) + Z_PARAM_FUNC(tick_fe.fci, tick_fe.fci_cache) + ZEND_PARSE_PARAMETERS_END(); if (!BG(user_tick_functions)) { return; } - zend_llist_del_element(BG(user_tick_functions), tick_fe, (int (*)(void *, void *)) user_tick_function_compare); - efree(tick_fe); + zend_llist_del_element(BG(user_tick_functions), &tick_fe, (int (*)(void *, void *)) user_tick_function_compare); } /* }}} */ From ded8c16f2fa7f10ea5750dd2934640e76aaef846 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 8 May 2021 09:42:11 +0100 Subject: [PATCH 4/6] Use zend_call_function() API --- ext/standard/basic_functions.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index fd74548237dfe..c16d5b30fc52b 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1715,8 +1715,13 @@ 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_fcall_info_call(&tick_fe->fci, &tick_fe->fci_cache, tick_fe->fci.retval, NULL); + zend_call_function(&tick_fe->fci, &tick_fe->fci_cache); zend_release_fcall_info_cache(&tick_fe->fci_cache); tick_fe->calling = false; } From 1452d54db8a18ce25e380fd28a01aa8b424a0787 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 8 May 2021 09:47:12 +0100 Subject: [PATCH 5/6] Drop explicit call to zend_release_fcall_info_cache() --- ext/standard/basic_functions.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index c16d5b30fc52b..be827dc47a7f2 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1722,7 +1722,6 @@ static void user_tick_function_call(user_tick_function_entry *tick_fe) /* {{{ */ tick_fe->calling = true; zend_call_function(&tick_fe->fci, &tick_fe->fci_cache); - zend_release_fcall_info_cache(&tick_fe->fci_cache); tick_fe->calling = false; } } From cda8c1ef427b7be987bf0609d3f94519d9621125 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 8 May 2021 09:48:07 +0100 Subject: [PATCH 6/6] Destroy return value --- ext/standard/basic_functions.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index be827dc47a7f2..8bd69cde48f58 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1722,6 +1722,9 @@ static void user_tick_function_call(user_tick_function_entry *tick_fe) /* {{{ */ tick_fe->calling = true; zend_call_function(&tick_fe->fci, &tick_fe->fci_cache); + + /* Destroy return value */ + zval_ptr_dtor(&tmp); tick_fe->calling = false; } }