Skip to content

Commit

Permalink
Add option to allow stack extension (#131)
Browse files Browse the repository at this point in the history
  • Loading branch information
agoallikmaa authored Feb 5, 2024
1 parent 7010de9 commit 85d4d59
Show file tree
Hide file tree
Showing 12 changed files with 276 additions and 9 deletions.
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions ext/opentelemetry.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
100 changes: 94 additions & 6 deletions ext/otel_observer.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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) {
Expand All @@ -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(&params[2]),
" - %s, class=%s function=%s",
idx, failure_reason,
zval_get_chars(&params[2]),
zval_get_chars(&params[3]));
invalid_arg_warned = true;
continue;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions ext/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
<file name="disable_hook_function_validation.phpt" role="test"/>
<file name="disabled_with_conflicting_extension.phpt" role="test"/>
<file name="expand_params.phpt" role="test"/>
<file name="expand_params_extend.phpt" role="test"/>
<file name="expand_params_extend_internal.phpt" role="test"/>
<file name="expand_params_extend_internal_long.phpt" role="test"/>
<file name="expand_params_extend_limit.phpt" role="test"/>
<file name="expand_params_extra.phpt" role="test"/>
<file name="expand_params_internal.phpt" role="test"/>
<file name="function_closure.phpt" role="test"/>
Expand Down
1 change: 1 addition & 0 deletions ext/php_opentelemetry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
41 changes: 41 additions & 0 deletions ext/tests/expand_params_extend.phpt
Original file line number Diff line number Diff line change
@@ -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--
<?php
OpenTelemetry\Instrumentation\hook(
null,
'helloWorld',
pre: function($instance, array $params) {
return [$params[0], 'b', 'c', 'd', 'e', 'f', 'g', 'h'];
},
post: fn() => 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"
}
26 changes: 26 additions & 0 deletions ext/tests/expand_params_extend_internal.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--TEST--
Check if pre hook can expand params of internal function when that requires extending the stack
--SKIPIF--
<?php if (PHP_VERSION_ID < 80200) die('skip requires PHP >= 8.2'); ?>
--EXTENSIONS--
opentelemetry
--INI--
opentelemetry.allow_stack_extension=On
--FILE--
<?php
OpenTelemetry\Instrumentation\hook(
null,
'array_slice',
pre: function(null $instance, array $params) {
return [$params[0], $params[1], 1, true];
},
post: fn() => null
);

var_dump(array_slice([1,2,3], 1));
?>
--EXPECTF--
array(1) {
[1]=>
int(2)
}
31 changes: 31 additions & 0 deletions ext/tests/expand_params_extend_internal_long.phpt
Original file line number Diff line number Diff line change
@@ -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--
<?php if (PHP_VERSION_ID < 80200) die('skip requires PHP >= 8.2'); ?>
--EXTENSIONS--
opentelemetry
--INI--
opentelemetry.allow_stack_extension=On
--FILE--
<?php
OpenTelemetry\Instrumentation\hook(
null,
'array_slice',
pre: function(null $instance, array $params) {
return [$params[0], $params[1], 1, true, true, true, true, true, true, true, true, true, true];
},
post: fn() => 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
62 changes: 62 additions & 0 deletions ext/tests/expand_params_extend_limit.phpt
Original file line number Diff line number Diff line change
@@ -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--
<?php
OpenTelemetry\Instrumentation\hook(
null,
'helloWorld',
pre: function($instance, array $params) {
return [$params[0], 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u'];
},
post: fn() => 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"
}
2 changes: 1 addition & 1 deletion ext/tests/expand_params_extra.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion ext/tests/expand_params_internal.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 85d4d59

Please sign in to comment.