From 7010de909f8fa65ae83ffe3ffa815c82c71842c6 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Mon, 5 Feb 2024 11:13:13 +1100 Subject: [PATCH 1/2] 1.0.1 release prep (#132) --- ext/package.xml | 32 +++++++++++++++++++++++--------- ext/php_opentelemetry.h | 2 +- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/ext/package.xml b/ext/package.xml index 9549f05..01dd743 100644 --- a/ext/package.xml +++ b/ext/package.xml @@ -16,22 +16,18 @@ pdelewski@example.com yes - 2024-01-27 - + 2024-02-04 + - 1.0.1beta2 + 1.0.1 1.0 - beta + stable stable Apache 2.0 - opentelemetry 1.0.1beta2 - * Fix exception raised by `php_error_docref` that hangs the process in hook (#127) - * clang format (#126) - * Fix build warnings (#125) - + See https://github.com/open-telemetry/opentelemetry-php-instrumentation/releases/tag/1.0.1 @@ -328,5 +324,23 @@ * add pear to build (#108) + + 2024-01-27 + + + 1.0.1beta2 + 1.0 + + + beta + stable + + Apache 2.0 + opentelemetry 1.0.1beta2 + * Fix exception raised by `php_error_docref` that hangs the process in hook (#127) + * clang format (#126) + * Fix build warnings (#125) + + \ No newline at end of file diff --git a/ext/php_opentelemetry.h b/ext/php_opentelemetry.h index f364a5e..41a0f9a 100644 --- a/ext/php_opentelemetry.h +++ b/ext/php_opentelemetry.h @@ -18,7 +18,7 @@ ZEND_EXTERN_MODULE_GLOBALS(opentelemetry) #define OTEL_G(v) ZEND_MODULE_GLOBALS_ACCESSOR(opentelemetry, v) -#define PHP_OPENTELEMETRY_VERSION "1.0.1beta2" +#define PHP_OPENTELEMETRY_VERSION "1.0.1" #if defined(ZTS) && defined(COMPILE_DL_OPENTELEMETRY) ZEND_TSRMLS_CACHE_EXTERN() From 85d4d59a859ecef7502edbaad3199c671995d408 Mon Sep 17 00:00:00 2001 From: Ago Allikmaa Date: Mon, 5 Feb 2024 02:45:51 +0200 Subject: [PATCH 2/2] Add option to allow stack extension (#131) --- README.md | 10 ++ ext/opentelemetry.c | 4 + ext/otel_observer.c | 100 ++++++++++++++++-- ext/package.xml | 4 + ext/php_opentelemetry.h | 1 + ext/tests/expand_params_extend.phpt | 41 +++++++ ext/tests/expand_params_extend_internal.phpt | 26 +++++ .../expand_params_extend_internal_long.phpt | 31 ++++++ ext/tests/expand_params_extend_limit.phpt | 62 +++++++++++ ext/tests/expand_params_extra.phpt | 2 +- ext/tests/expand_params_internal.phpt | 2 +- .../return_expanded_params_internal.phpt | 2 +- 12 files changed, 276 insertions(+), 9 deletions(-) create mode 100644 ext/tests/expand_params_extend.phpt create mode 100644 ext/tests/expand_params_extend_internal.phpt create mode 100644 ext/tests/expand_params_extend_internal_long.phpt create mode 100644 ext/tests/expand_params_extend_limit.phpt diff --git a/README.md b/README.md index 408e796..ee4ffa3 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,16 @@ hook functions to ensure they are compatible. If not, the hook will not be execu This feature can be disabled by setting the `opentelemetry.validate_hook_functions` ini value to `Off`; +### Increasing function argument count + +By default, increasing the number of arguments provided to a function in the pre hook is allowed only if that does +not require the stack frame of the function call to be extended in size. For internal functions, adding arguments +not provided at the callsite always requires stack extension. For PHP functions, it is required only if the +argument is not included in the function definition. + +Extending stack frame automatically can be enabled by setting `opentelemetry.allow_stack_extension` ini value to `On`. +This enables extending the stack frame by up to another 16 arguments. + ## Usage *Warning* Be aware that trivial functions are candidates for optimizations. diff --git a/ext/opentelemetry.c b/ext/opentelemetry.c index fa53554..0db31ae 100644 --- a/ext/opentelemetry.c +++ b/ext/opentelemetry.c @@ -82,6 +82,10 @@ STD_PHP_INI_ENTRY_EX("opentelemetry.validate_hook_functions", "On", PHP_INI_ALL, OnUpdateBool, validate_hook_functions, zend_opentelemetry_globals, opentelemetry_globals, zend_ini_boolean_displayer_cb) +STD_PHP_INI_ENTRY_EX("opentelemetry.allow_stack_extension", "Off", PHP_INI_ALL, + OnUpdateBool, allow_stack_extension, + zend_opentelemetry_globals, opentelemetry_globals, + zend_ini_boolean_displayer_cb) PHP_INI_END() PHP_FUNCTION(hook) { diff --git a/ext/otel_observer.c b/ext/otel_observer.c index ad006ea..2fdc8e0 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -22,6 +22,8 @@ typedef struct otel_exception_state { const zend_op *opline; } otel_exception_state; +#define STACK_EXTENSION_LIMIT 16 + typedef struct otel_arg_locator { zend_execute_data *execute_data; // Number of argument slots reserved in execute_data, any arguments beyond @@ -32,6 +34,10 @@ typedef struct otel_arg_locator { uint32_t provided; // Number of slots between reserved and "extra" argument slots uint32_t auxiliary_slots; + uint32_t extended_start; + uint32_t extended_max; + uint32_t extended_used; + zval extended_slots[STACK_EXTENSION_LIMIT]; } otel_arg_locator; static inline void @@ -332,19 +338,94 @@ static void arg_locator_initialize(otel_arg_locator *arg_locator, arg_locator->provided = ZEND_CALL_NUM_ARGS(execute_data); arg_locator->auxiliary_slots = execute_data->func->op_array.T; + + arg_locator->extended_used = 0; + arg_locator->extended_start = arg_locator->provided > arg_locator->reserved + ? arg_locator->provided + : arg_locator->reserved; + + if (OTEL_G(allow_stack_extension)) { + arg_locator->extended_max = STACK_EXTENSION_LIMIT; + + size_t slots_left_in_stack = EG(vm_stack_end) - EG(vm_stack_top); + if (slots_left_in_stack < arg_locator->extended_max) { + arg_locator->extended_max = slots_left_in_stack; + } + } else { + arg_locator->extended_max = 0; + } } -static zval *arg_locator_get_slot(otel_arg_locator *arg_locator, - uint32_t index) { +static zval *arg_locator_get_slot(otel_arg_locator *arg_locator, uint32_t index, + const char **failure_reason) { + if (index < arg_locator->reserved) { return ZEND_CALL_ARG(arg_locator->execute_data, index + 1); } else if (index < arg_locator->provided) { return ZEND_CALL_ARG(arg_locator->execute_data, index + arg_locator->auxiliary_slots + 1); } + + uint32_t extended_index = index - arg_locator->extended_start; + + if (extended_index < arg_locator->extended_max) { + uint32_t extended_index = index - arg_locator->extended_start; + if (extended_index >= arg_locator->extended_used) { + arg_locator->extended_used = extended_index + 1; + } + + return &arg_locator->extended_slots[extended_index]; + } + + if (failure_reason != NULL) { + // Having a hardcoded upper limit allows performing stack + // extension as one step in the end, rather than moving slots around in + // the stack each time a new argument is discovered + if (extended_index >= STACK_EXTENSION_LIMIT) { + *failure_reason = "exceeds built-in stack extension limit"; + } else if (!OTEL_G(allow_stack_extension)) { + *failure_reason = "stack extension must be enabled with " + "opentelemetry.allow_stack_extension option"; + } else { + *failure_reason = "not enough room left in stack page"; + } + } + return NULL; } +static void arg_locator_store_extended(otel_arg_locator *arg_locator) { + if (arg_locator->extended_used == 0) { + return; + } + + // This is safe because extended_max is adjusted to not exceed current stack + // page end + EG(vm_stack_top) += arg_locator->extended_used; + + if (arg_locator->execute_data->func->type == ZEND_INTERNAL_FUNCTION) { + // For internal functions, the additional arguments need to go before + // the auxiliary slots, therefore the auxiliary slots need to be moved + // ahead + zval *target = + ZEND_CALL_ARG(arg_locator->execute_data, arg_locator->provided + 1); + zval *aux_target = target + arg_locator->extended_used; + + memmove(aux_target, target, + sizeof(*aux_target) * arg_locator->auxiliary_slots); + memcpy(target, arg_locator->extended_slots, + sizeof(*target) * arg_locator->extended_used); + } else { + // For PHP functions, the additional arguments go to the end of the + // frame, so nothing else needs to be moved around + zval *target = ZEND_CALL_ARG(arg_locator->execute_data, + arg_locator->extended_start + + arg_locator->auxiliary_slots + 1); + memcpy(target, arg_locator->extended_slots, + sizeof(*target) * arg_locator->extended_used); + } +} + static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { if (!zend_llist_count(hooks)) { return; @@ -402,6 +483,8 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { uint32_t args_initialized = arg_locator.provided; ZEND_HASH_FOREACH_KEY_VAL(Z_ARR(ret), idx, str_idx, val) { + const char *failure_reason = ""; + if (str_idx != NULL) { idx = func_get_arg_index_by_name(execute_data, str_idx); @@ -416,7 +499,8 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { } } - zval *target = arg_locator_get_slot(&arg_locator, idx); + zval *target = arg_locator_get_slot(&arg_locator, idx, + &failure_reason); if (target == NULL) { if (invalid_arg_warned) { @@ -426,8 +510,9 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { php_error_docref(NULL, E_CORE_WARNING, "OpenTelemetry: pre hook invalid " "argument index " ZEND_ULONG_FMT - ", class=%s function=%s", - idx, zval_get_chars(¶ms[2]), + " - %s, class=%s function=%s", + idx, failure_reason, + zval_get_chars(¶ms[2]), zval_get_chars(¶ms[3])); invalid_arg_warned = true; continue; @@ -438,7 +523,8 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { // all slots between current and the last initialized // one for (uint32_t i = args_initialized; i < idx; i++) { - ZVAL_UNDEF(arg_locator_get_slot(&arg_locator, i)); + ZVAL_UNDEF( + arg_locator_get_slot(&arg_locator, i, NULL)); ZEND_ADD_CALL_FLAG(execute_data, ZEND_CALL_MAY_HAVE_UNDEF); } @@ -472,6 +558,8 @@ static void observer_begin(zend_execute_data *execute_data, zend_llist *hooks) { } ZEND_HASH_FOREACH_END(); + arg_locator_store_extended(&arg_locator); + // Update provided argument count if begin hook added arguments // that were not provided originally if (args_initialized > arg_locator.provided) { diff --git a/ext/package.xml b/ext/package.xml index 01dd743..c721816 100644 --- a/ext/package.xml +++ b/ext/package.xml @@ -56,6 +56,10 @@ + + + + diff --git a/ext/php_opentelemetry.h b/ext/php_opentelemetry.h index 41a0f9a..b3ec053 100644 --- a/ext/php_opentelemetry.h +++ b/ext/php_opentelemetry.h @@ -12,6 +12,7 @@ ZEND_BEGIN_MODULE_GLOBALS(opentelemetry) int validate_hook_functions; char *conflicts; int disabled; // module disabled? (eg due to conflicting extension loaded) + int allow_stack_extension; ZEND_END_MODULE_GLOBALS(opentelemetry) ZEND_EXTERN_MODULE_GLOBALS(opentelemetry) diff --git a/ext/tests/expand_params_extend.phpt b/ext/tests/expand_params_extend.phpt new file mode 100644 index 0000000..d349f04 --- /dev/null +++ b/ext/tests/expand_params_extend.phpt @@ -0,0 +1,41 @@ +--TEST-- +Check if pre hook can expand params of function when that requires extending the stack +--EXTENSIONS-- +opentelemetry +--INI-- +opentelemetry.allow_stack_extension=On +--FILE-- + null +); + +function helloWorld($a, $b) { + var_dump(func_get_args()); +} +helloWorld('a'); +?> +--EXPECTF-- +array(8) { + [0]=> + string(1) "a" + [1]=> + string(1) "b" + [2]=> + string(1) "c" + [3]=> + string(1) "d" + [4]=> + string(1) "e" + [5]=> + string(1) "f" + [6]=> + string(1) "g" + [7]=> + string(1) "h" +} diff --git a/ext/tests/expand_params_extend_internal.phpt b/ext/tests/expand_params_extend_internal.phpt new file mode 100644 index 0000000..c982a6f --- /dev/null +++ b/ext/tests/expand_params_extend_internal.phpt @@ -0,0 +1,26 @@ +--TEST-- +Check if pre hook can expand params of internal function when that requires extending the stack +--SKIPIF-- += 8.2'); ?> +--EXTENSIONS-- +opentelemetry +--INI-- +opentelemetry.allow_stack_extension=On +--FILE-- + null +); + +var_dump(array_slice([1,2,3], 1)); +?> +--EXPECTF-- +array(1) { + [1]=> + int(2) +} diff --git a/ext/tests/expand_params_extend_internal_long.phpt b/ext/tests/expand_params_extend_internal_long.phpt new file mode 100644 index 0000000..aa1062c --- /dev/null +++ b/ext/tests/expand_params_extend_internal_long.phpt @@ -0,0 +1,31 @@ +--TEST-- +Check if pre hook can expand params of internal function when that requires extending the stack (many params) +--DESCRIPTION-- +This will add MANY extra arguments to an internal function, making it fail with an error. +However, the purpose of this test is to just make sure it does not somehow corrupt the stack +and cause a crash with a large number of extra parameters. +--SKIPIF-- += 8.2'); ?> +--EXTENSIONS-- +opentelemetry +--INI-- +opentelemetry.allow_stack_extension=On +--FILE-- + null +); + +var_dump(array_slice([1,2,3], 1)); +?> +--EXPECTF-- +Fatal error: Uncaught ArgumentCountError: array_slice() expects at most 4 arguments, 13 given in %s +Stack trace: +#0 %s: array_slice(Array, 1, 1, true, true, true, true, true, true, true, true, true, true) +#1 {main} + thrown in %s on line %d \ No newline at end of file diff --git a/ext/tests/expand_params_extend_limit.phpt b/ext/tests/expand_params_extend_limit.phpt new file mode 100644 index 0000000..0f9ed83 --- /dev/null +++ b/ext/tests/expand_params_extend_limit.phpt @@ -0,0 +1,62 @@ +--TEST-- +Check if pre hook can expand params of function when that requires extending the stack only until hardcoded limit +--EXTENSIONS-- +opentelemetry +--INI-- +opentelemetry.allow_stack_extension=On +--FILE-- + null +); + +function helloWorld($a, $b) { + var_dump(func_get_args()); +} +helloWorld('a'); +?> +--EXPECTF-- +Warning: helloWorld(): OpenTelemetry: pre hook invalid argument index 18 - exceeds built-in stack extension limit, class=null function=helloWorld in %s +array(18) { + [0]=> + string(1) "a" + [1]=> + string(1) "b" + [2]=> + string(1) "c" + [3]=> + string(1) "d" + [4]=> + string(1) "e" + [5]=> + string(1) "f" + [6]=> + string(1) "g" + [7]=> + string(1) "h" + [8]=> + string(1) "i" + [9]=> + string(1) "j" + [10]=> + string(1) "k" + [11]=> + string(1) "l" + [12]=> + string(1) "m" + [13]=> + string(1) "n" + [14]=> + string(1) "o" + [15]=> + string(1) "p" + [16]=> + string(1) "q" + [17]=> + string(1) "r" +} diff --git a/ext/tests/expand_params_extra.phpt b/ext/tests/expand_params_extra.phpt index 98562d1..5ce133e 100644 --- a/ext/tests/expand_params_extra.phpt +++ b/ext/tests/expand_params_extra.phpt @@ -23,7 +23,7 @@ function helloWorld($a, $b) { helloWorld('a'); ?> --EXPECTF-- -Warning: helloWorld(): OpenTelemetry: pre hook invalid argument index 2, class=null function=helloWorld in %s +Warning: helloWorld(): OpenTelemetry: pre hook invalid argument index 2 - stack extension must be enabled with opentelemetry.allow_stack_extension option, class=null function=helloWorld in %s array(2) { [0]=> string(1) "a" diff --git a/ext/tests/expand_params_internal.phpt b/ext/tests/expand_params_internal.phpt index 2edec06..ad15e69 100644 --- a/ext/tests/expand_params_internal.phpt +++ b/ext/tests/expand_params_internal.phpt @@ -23,7 +23,7 @@ OpenTelemetry\Instrumentation\hook( var_dump(array_slice([1,2,3], 1)); ?> --EXPECTF-- -Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s +Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2 - stack extension must be enabled with opentelemetry.allow_stack_extension option, class=null function=array_slice in %s array(2) { [0]=> int(2) diff --git a/ext/tests/return_expanded_params_internal.phpt b/ext/tests/return_expanded_params_internal.phpt index 4918e65..3cf0f98 100644 --- a/ext/tests/return_expanded_params_internal.phpt +++ b/ext/tests/return_expanded_params_internal.phpt @@ -21,7 +21,7 @@ opentelemetry var_dump(array_slice(['a', 'b', 'c'], 1)); ?> --EXPECTF-- -Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2, class=null function=array_slice in %s +Warning: array_slice(): OpenTelemetry: pre hook invalid argument index 2 - stack extension must be enabled with opentelemetry.allow_stack_extension option, class=null function=array_slice in %s array(2) { [0]=> string(1) "b"