From 31039a946839d86221ae97db25283c3c6b3aba76 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Thu, 9 Nov 2023 21:29:09 +0100 Subject: [PATCH 01/15] add failing test --- .../tests/phpt/allocation_generator_01.phpt | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 profiling/tests/phpt/allocation_generator_01.phpt diff --git a/profiling/tests/phpt/allocation_generator_01.phpt b/profiling/tests/phpt/allocation_generator_01.phpt new file mode 100644 index 0000000000..72f9640c0b --- /dev/null +++ b/profiling/tests/phpt/allocation_generator_01.phpt @@ -0,0 +1,30 @@ +--TEST-- +[profiling] profiling should not crash during a `ZEND_GENERATOR_CREATE` +--DESCRIPTION-- +We found a segfault when allocation profiling triggers during a +`ZEND_GENERATOR_CREATE`. This is due to a missing `SAVE_OPLINE()` in +https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h#1790. This missing +`SAVE_OPLINE()` was added in PHP 8.0.26 with the commit +https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a +where also a test was added, that we "borrowed" from that commit. + +Note: this does not trigger with PHP 7 when the tracer is enabled, as the tracer +restores the opline in a opcode handler! +--SKIPIF-- + +--INI-- +memory_limit=16m +--FILE-- + +--EXPECTF-- +Fatal error: Allowed memory size of %d bytes exhausted %s From e60500a12321671cd2a48e07b6212bcacb86075d Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 9 Nov 2023 20:12:18 -0700 Subject: [PATCH 02/15] style(profiling): clang-format --- profiling/src/php_ffi.c | 90 +++++++++++++++++++---------------------- profiling/src/php_ffi.h | 24 +++++------ 2 files changed, 52 insertions(+), 62 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 0c37745690..ad27d73c85 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -6,7 +6,7 @@ #include #if CFG_STACK_WALKING_TESTS -#include // for dlsym +#include // for dlsym #endif const char *datadog_extension_build_id(void) { return ZEND_EXTENSION_BUILD_ID; } @@ -34,12 +34,10 @@ static ddtrace_profiling_context noop_get_profiling_context(void) { return (ddtrace_profiling_context){0, 0}; } -#if CFG_PRELOAD // defined by build.rs +#if CFG_PRELOAD // defined by build.rs static bool _is_post_startup = false; -bool ddog_php_prof_is_post_startup(void) { - return _is_post_startup; -} +bool ddog_php_prof_is_post_startup(void) { return _is_post_startup; } #if PHP_VERSION_ID < 80000 #define post_startup_cb_result int @@ -65,8 +63,7 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { } #endif - -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs /** * Currently used to ignore run_time_cache on CLI SAPI as a precaution against * unbounded memory growth. Unbounded growth is more likely there since it's @@ -75,9 +72,21 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { static bool _ignore_run_time_cache = false; #endif -void datadog_php_profiling_startup(zend_extension *extension) { +#if PHP_VERSION_ID >= 70400 PHP_VERSION_ID < 80100 +/* The purpose here is to set the opline, because these versions are missing a + * SAVE_OPLINE() and it causes a crash for the allocation profiler, see: + * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a + * The handler doesn't actually need to do anything, because just by having a + * handler will save the opline before calling the user handler: + * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 + */ +zend_result ddog_php_prof_generator_create(zend_execute_data *execute_data) { + return ZEND_USER_OPCODE_DISPATCH; +} +#endif -#if CFG_RUN_TIME_CACHE // defined by build.rs +void datadog_php_profiling_startup(zend_extension *extension) { +#if CFG_RUN_TIME_CACHE // defined by build.rs _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; #endif @@ -97,7 +106,7 @@ void datadog_php_profiling_startup(zend_extension *extension) { } } -#if CFG_PRELOAD // defined by build.rs +#if CFG_PRELOAD // defined by build.rs _is_post_startup = false; orig_post_startup_cb = zend_post_startup_cb; zend_post_startup_cb = ddog_php_prof_post_startup_cb; @@ -123,8 +132,7 @@ void datadog_php_profiling_install_internal_function_handler( } } -void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, - bool persistent) { +void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, bool persistent) { ZEND_ASSERT(dest); if (view.len == 0) { @@ -145,10 +153,9 @@ void ddog_php_prof_copy_long_into_zval(zval *dest, long num) { return; } -void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, - void* (*_malloc)(size_t), - void (*_free)(void*), - void* (*_realloc)(void*, size_t)) { +void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, void *(*_malloc)(size_t), + void (*_free)(void *), + void *(*_realloc)(void *, size_t)) { zend_mm_set_custom_handlers(heap, _malloc, _free, _realloc); #if PHP_VERSION_ID < 70300 if (!_malloc && !_free && !_realloc) { @@ -157,39 +164,29 @@ void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, #endif } -zend_execute_data* ddog_php_prof_get_current_execute_data() { - return EG(current_execute_data); -} +zend_execute_data *ddog_php_prof_get_current_execute_data() { return EG(current_execute_data); } -#if CFG_FIBERS // defined by build.rs -zend_fiber* ddog_php_prof_get_active_fiber() -{ - return EG(active_fiber); -} +#if CFG_FIBERS // defined by build.rs +zend_fiber *ddog_php_prof_get_active_fiber() { return EG(active_fiber); } -zend_fiber* ddog_php_prof_get_active_fiber_test() -{ - return NULL; -} +zend_fiber *ddog_php_prof_get_active_fiber_test() { return NULL; } #endif -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs static int ddog_php_prof_run_time_cache_handle = -1; #endif void ddog_php_prof_function_run_time_cache_init(const char *module_name) { -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs // Grab 1 slot for the full module|class::method name. // Grab 1 slot for caching filename, as it turns out the utf-8 validity // check is worth caching. #if PHP_VERSION_ID < 80200 - ddog_php_prof_run_time_cache_handle = - zend_get_op_array_extension_handle(module_name); + ddog_php_prof_run_time_cache_handle = zend_get_op_array_extension_handle(module_name); int second = zend_get_op_array_extension_handle(module_name); ZEND_ASSERT(ddog_php_prof_run_time_cache_handle + 1 == second); #else - ddog_php_prof_run_time_cache_handle = - zend_get_op_array_extension_handles(module_name, 2); + ddog_php_prof_run_time_cache_handle = zend_get_op_array_extension_handles(module_name, 2); #endif #else (void)module_name; @@ -202,14 +199,14 @@ void ddog_php_prof_function_run_time_cache_init(const char *module_name) { */ } -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs static bool has_invalid_run_time_cache(zend_function const *func) { if (UNEXPECTED(_ignore_run_time_cache) || UNEXPECTED(ddog_php_prof_run_time_cache_handle < 0)) return true; - // during an `include()`/`require()` with enabled OPcache, OPcache is - // persisting the compiled file and puts a fake frame on the stack where the - // runtime cache is not yet initialized. + // during an `include()`/`require()` with enabled OPcache, OPcache is + // persisting the compiled file and puts a fake frame on the stack where the + // runtime cache is not yet initialized. #if PHP_VERSION_ID < 80200 bool is_file_compile = ZEND_MAP_PTR(func->op_array.run_time_cache) == NULL; #else @@ -402,7 +399,7 @@ bool ddog_php_jit_enabled() { zval *jit_stats = zend_hash_str_find(Z_ARR(jit_stats_arr), ZEND_STRL("jit")); zval *jit_buffer = zend_hash_str_find(Z_ARR_P(jit_stats), ZEND_STRL("buffer_size")); - jit = Z_LVAL_P(jit_buffer) > 0; // JIT is active! + jit = Z_LVAL_P(jit_buffer) > 0; // JIT is active! zval_ptr_dtor(&jit_stats_arr); } @@ -410,7 +407,6 @@ bool ddog_php_jit_enabled() { return jit; } - #if PHP_VERSION_ID < 70200 #define zend_parse_parameters_none_throw() \ (EXPECTED(ZEND_NUM_ARGS() == 0) ? SUCCESS : zend_parse_parameters_throw(ZEND_NUM_ARGS(), "")) @@ -432,13 +428,9 @@ ZEND_END_ARG_INFO() static const zend_function_entry functions[] = { #if CFG_TRIGGER_TIME_SAMPLE - ZEND_NS_NAMED_FE( - "Datadog\\Profiling", - trigger_time_sample, - ZEND_FN(Datadog_Profiling_trigger_time_sample), - arginfo_Datadog_Profiling_trigger_time_sample - ) + ZEND_NS_NAMED_FE("Datadog\\Profiling", trigger_time_sample, + ZEND_FN(Datadog_Profiling_trigger_time_sample), + arginfo_Datadog_Profiling_trigger_time_sample) #endif - ZEND_FE_END -}; -const zend_function_entry* ddog_php_prof_functions = functions; + ZEND_FE_END}; +const zend_function_entry *ddog_php_prof_functions = functions; diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index 3eea8468da..ebd69c628d 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -1,13 +1,13 @@ #include -#include #include +#include #include -#if CFG_FIBERS // defined by build.rs +#if CFG_FIBERS // defined by build.rs #include #endif +#include #include #include -#include #include
#include #include @@ -96,8 +96,7 @@ void datadog_php_profiling_install_internal_function_handler( * `dest` is expected to be uninitialized. Any existing content will not be * dtor'. */ -void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, - bool persistent); +void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, bool persistent); /** * Copies the number in `num` into a zval, which is stored in `dest` @@ -118,23 +117,22 @@ void ddog_php_prof_copy_long_into_zval(zval *dest, long num); * `use_custom_heap` flag back to normal when null pointers are being passed * in on those PHP versions. */ -void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, - void* (*_malloc)(size_t), - void (*_free)(void*), - void* (*_realloc)(void*, size_t)); +void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, void *(*_malloc)(size_t), + void (*_free)(void *), + void *(*_realloc)(void *, size_t)); -zend_execute_data* ddog_php_prof_get_current_execute_data(); +zend_execute_data *ddog_php_prof_get_current_execute_data(); #if CFG_FIBERS -zend_fiber* ddog_php_prof_get_active_fiber(); -zend_fiber* ddog_php_prof_get_active_fiber_test(); +zend_fiber *ddog_php_prof_get_active_fiber(); +zend_fiber *ddog_php_prof_get_active_fiber_test(); #endif /** * The following two functions exist for the sole purpose of creating fake stack * frames that can be used in testing/benchmarking scenarios */ -zend_execute_data* ddog_php_test_create_fake_zend_execute_data(int depth); +zend_execute_data *ddog_php_test_create_fake_zend_execute_data(int depth); void ddog_php_test_free_fake_zend_execute_data(zend_execute_data *execute_data); void ddog_php_opcache_init_handle(); From 16dad4c84d8b2f5bfeca2b7d945b92b098b435b2 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 9 Nov 2023 20:20:24 -0700 Subject: [PATCH 03/15] fix(profiling): save EX(opline) for ZEND_GENERATOR_CREATE --- profiling/src/lib.rs | 2 +- profiling/src/php_ffi.c | 22 ++++++++++++++++++---- profiling/src/php_ffi.h | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index dae8db5034..a8d3d90a54 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -974,7 +974,7 @@ extern "C" fn startup(extension: *mut ZendExtension) -> ZendResult { trace!("startup({:p})", extension); // Safety: called during startup hook with correct params. - unsafe { zend::datadog_php_profiling_startup(extension) }; + unsafe { zend::datadog_php_profiling_startup(extension, PHP_VERSION_ID) }; #[cfg(php_run_time_cache)] // Safety: calling this in startup/minit as required. diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index ad27d73c85..f026815087 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -72,7 +72,7 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { static bool _ignore_run_time_cache = false; #endif -#if PHP_VERSION_ID >= 70400 PHP_VERSION_ID < 80100 +#if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80100 /* The purpose here is to set the opline, because these versions are missing a * SAVE_OPLINE() and it causes a crash for the allocation profiler, see: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a @@ -80,12 +80,25 @@ static bool _ignore_run_time_cache = false; * handler will save the opline before calling the user handler: * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 */ -zend_result ddog_php_prof_generator_create(zend_execute_data *execute_data) { +static zend_result ddog_php_prof_noop_opcode(zend_execute_data *execute_data) { return ZEND_USER_OPCODE_DISPATCH; } #endif -void datadog_php_profiling_startup(zend_extension *extension) { +void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id) { + (void)php_version_id; +#if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80100 +#if PHP_VERSION_ID < 80000 + // Only need to install a handler if there isn't one, see `ddog_php_prof_noop_opcode`. + if (zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { +#else + // Issue is fixed in 8.0.26. + if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { +#endif + user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_noop_opcode; + zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, handler); + } +#endif #if CFG_RUN_TIME_CACHE // defined by build.rs _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; #endif @@ -432,5 +445,6 @@ static const zend_function_entry functions[] = { ZEND_FN(Datadog_Profiling_trigger_time_sample), arginfo_Datadog_Profiling_trigger_time_sample) #endif - ZEND_FE_END}; + ZEND_FE_END +}; const zend_function_entry *ddog_php_prof_functions = functions; diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index ebd69c628d..81698c929c 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -71,7 +71,7 @@ extern ddtrace_profiling_context (*datadog_php_profiling_get_profiling_context)( * burdensome in Rust, like locating the ddtrace extension in the module * registry and finding the ddtrace_get_profiling_context function. */ -void datadog_php_profiling_startup(zend_extension *extension); +void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id); /** * Used to hold information for overwriting the internal function handler From 57f9952cc33c3c217091f63971e3b301a26f9c00 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 9 Nov 2023 21:36:00 -0700 Subject: [PATCH 04/15] Revert "style(profiling): clang-format" This reverts commit e4ee56cb72f061c5ccccd7a869acc7306430b45c. --- profiling/src/php_ffi.c | 73 ++++++++++++++++++++++++++--------------- profiling/src/php_ffi.h | 24 +++++++------- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index f026815087..0f888252e1 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -6,7 +6,7 @@ #include #if CFG_STACK_WALKING_TESTS -#include // for dlsym +#include // for dlsym #endif const char *datadog_extension_build_id(void) { return ZEND_EXTENSION_BUILD_ID; } @@ -34,10 +34,12 @@ static ddtrace_profiling_context noop_get_profiling_context(void) { return (ddtrace_profiling_context){0, 0}; } -#if CFG_PRELOAD // defined by build.rs +#if CFG_PRELOAD // defined by build.rs static bool _is_post_startup = false; -bool ddog_php_prof_is_post_startup(void) { return _is_post_startup; } +bool ddog_php_prof_is_post_startup(void) { + return _is_post_startup; +} #if PHP_VERSION_ID < 80000 #define post_startup_cb_result int @@ -63,7 +65,8 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { } #endif -#if CFG_RUN_TIME_CACHE // defined by build.rs + +#if CFG_RUN_TIME_CACHE // defined by build.rs /** * Currently used to ignore run_time_cache on CLI SAPI as a precaution against * unbounded memory growth. Unbounded growth is more likely there since it's @@ -119,7 +122,7 @@ void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_versi } } -#if CFG_PRELOAD // defined by build.rs +#if CFG_PRELOAD // defined by build.rs _is_post_startup = false; orig_post_startup_cb = zend_post_startup_cb; zend_post_startup_cb = ddog_php_prof_post_startup_cb; @@ -145,7 +148,8 @@ void datadog_php_profiling_install_internal_function_handler( } } -void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, bool persistent) { +void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, + bool persistent) { ZEND_ASSERT(dest); if (view.len == 0) { @@ -166,9 +170,10 @@ void ddog_php_prof_copy_long_into_zval(zval *dest, long num) { return; } -void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, void *(*_malloc)(size_t), - void (*_free)(void *), - void *(*_realloc)(void *, size_t)) { +void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, + void* (*_malloc)(size_t), + void (*_free)(void*), + void* (*_realloc)(void*, size_t)) { zend_mm_set_custom_handlers(heap, _malloc, _free, _realloc); #if PHP_VERSION_ID < 70300 if (!_malloc && !_free && !_realloc) { @@ -177,29 +182,39 @@ void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, void *(*_mall #endif } -zend_execute_data *ddog_php_prof_get_current_execute_data() { return EG(current_execute_data); } +zend_execute_data* ddog_php_prof_get_current_execute_data() { + return EG(current_execute_data); +} -#if CFG_FIBERS // defined by build.rs -zend_fiber *ddog_php_prof_get_active_fiber() { return EG(active_fiber); } +#if CFG_FIBERS // defined by build.rs +zend_fiber* ddog_php_prof_get_active_fiber() +{ + return EG(active_fiber); +} -zend_fiber *ddog_php_prof_get_active_fiber_test() { return NULL; } +zend_fiber* ddog_php_prof_get_active_fiber_test() +{ + return NULL; +} #endif -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs static int ddog_php_prof_run_time_cache_handle = -1; #endif void ddog_php_prof_function_run_time_cache_init(const char *module_name) { -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs // Grab 1 slot for the full module|class::method name. // Grab 1 slot for caching filename, as it turns out the utf-8 validity // check is worth caching. #if PHP_VERSION_ID < 80200 - ddog_php_prof_run_time_cache_handle = zend_get_op_array_extension_handle(module_name); + ddog_php_prof_run_time_cache_handle = + zend_get_op_array_extension_handle(module_name); int second = zend_get_op_array_extension_handle(module_name); ZEND_ASSERT(ddog_php_prof_run_time_cache_handle + 1 == second); #else - ddog_php_prof_run_time_cache_handle = zend_get_op_array_extension_handles(module_name, 2); + ddog_php_prof_run_time_cache_handle = + zend_get_op_array_extension_handles(module_name, 2); #endif #else (void)module_name; @@ -212,14 +227,14 @@ void ddog_php_prof_function_run_time_cache_init(const char *module_name) { */ } -#if CFG_RUN_TIME_CACHE // defined by build.rs +#if CFG_RUN_TIME_CACHE // defined by build.rs static bool has_invalid_run_time_cache(zend_function const *func) { if (UNEXPECTED(_ignore_run_time_cache) || UNEXPECTED(ddog_php_prof_run_time_cache_handle < 0)) return true; - // during an `include()`/`require()` with enabled OPcache, OPcache is - // persisting the compiled file and puts a fake frame on the stack where the - // runtime cache is not yet initialized. + // during an `include()`/`require()` with enabled OPcache, OPcache is + // persisting the compiled file and puts a fake frame on the stack where the + // runtime cache is not yet initialized. #if PHP_VERSION_ID < 80200 bool is_file_compile = ZEND_MAP_PTR(func->op_array.run_time_cache) == NULL; #else @@ -412,7 +427,7 @@ bool ddog_php_jit_enabled() { zval *jit_stats = zend_hash_str_find(Z_ARR(jit_stats_arr), ZEND_STRL("jit")); zval *jit_buffer = zend_hash_str_find(Z_ARR_P(jit_stats), ZEND_STRL("buffer_size")); - jit = Z_LVAL_P(jit_buffer) > 0; // JIT is active! + jit = Z_LVAL_P(jit_buffer) > 0; // JIT is active! zval_ptr_dtor(&jit_stats_arr); } @@ -420,6 +435,7 @@ bool ddog_php_jit_enabled() { return jit; } + #if PHP_VERSION_ID < 70200 #define zend_parse_parameters_none_throw() \ (EXPECTED(ZEND_NUM_ARGS() == 0) ? SUCCESS : zend_parse_parameters_throw(ZEND_NUM_ARGS(), "")) @@ -441,10 +457,13 @@ ZEND_END_ARG_INFO() static const zend_function_entry functions[] = { #if CFG_TRIGGER_TIME_SAMPLE - ZEND_NS_NAMED_FE("Datadog\\Profiling", trigger_time_sample, - ZEND_FN(Datadog_Profiling_trigger_time_sample), - arginfo_Datadog_Profiling_trigger_time_sample) + ZEND_NS_NAMED_FE( + "Datadog\\Profiling", + trigger_time_sample, + ZEND_FN(Datadog_Profiling_trigger_time_sample), + arginfo_Datadog_Profiling_trigger_time_sample + ) #endif - ZEND_FE_END + ZEND_FE_END }; -const zend_function_entry *ddog_php_prof_functions = functions; +const zend_function_entry* ddog_php_prof_functions = functions; diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index 81698c929c..84e00e62cd 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -1,13 +1,13 @@ #include -#include #include +#include #include -#if CFG_FIBERS // defined by build.rs +#if CFG_FIBERS // defined by build.rs #include #endif -#include #include #include +#include #include
#include #include @@ -96,7 +96,8 @@ void datadog_php_profiling_install_internal_function_handler( * `dest` is expected to be uninitialized. Any existing content will not be * dtor'. */ -void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, bool persistent); +void datadog_php_profiling_copy_string_view_into_zval(zval *dest, zai_str view, + bool persistent); /** * Copies the number in `num` into a zval, which is stored in `dest` @@ -117,22 +118,23 @@ void ddog_php_prof_copy_long_into_zval(zval *dest, long num); * `use_custom_heap` flag back to normal when null pointers are being passed * in on those PHP versions. */ -void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, void *(*_malloc)(size_t), - void (*_free)(void *), - void *(*_realloc)(void *, size_t)); +void ddog_php_prof_zend_mm_set_custom_handlers(zend_mm_heap *heap, + void* (*_malloc)(size_t), + void (*_free)(void*), + void* (*_realloc)(void*, size_t)); -zend_execute_data *ddog_php_prof_get_current_execute_data(); +zend_execute_data* ddog_php_prof_get_current_execute_data(); #if CFG_FIBERS -zend_fiber *ddog_php_prof_get_active_fiber(); -zend_fiber *ddog_php_prof_get_active_fiber_test(); +zend_fiber* ddog_php_prof_get_active_fiber(); +zend_fiber* ddog_php_prof_get_active_fiber_test(); #endif /** * The following two functions exist for the sole purpose of creating fake stack * frames that can be used in testing/benchmarking scenarios */ -zend_execute_data *ddog_php_test_create_fake_zend_execute_data(int depth); +zend_execute_data* ddog_php_test_create_fake_zend_execute_data(int depth); void ddog_php_test_free_fake_zend_execute_data(zend_execute_data *execute_data); void ddog_php_opcache_init_handle(); From 68a6ff038db8c297b85c769f17d985bc3364ad79 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 9 Nov 2023 21:43:14 -0700 Subject: [PATCH 05/15] docs: improve wording --- profiling/src/php_ffi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 0f888252e1..3b1892dc20 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -80,7 +80,7 @@ static bool _ignore_run_time_cache = false; * SAVE_OPLINE() and it causes a crash for the allocation profiler, see: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a * The handler doesn't actually need to do anything, because just by having a - * handler will save the opline before calling the user handler: + * handler the engine will save the opline before calling the user handler: * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 */ static zend_result ddog_php_prof_noop_opcode(zend_execute_data *execute_data) { From 54622134d9f56b8c4eeba6f397974791fc16ca1b Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 10 Nov 2023 05:47:01 -0700 Subject: [PATCH 06/15] refactor: rename noop_opcode to opcode_dispatch --- profiling/src/php_ffi.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 3b1892dc20..bb5d8cbbef 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -83,7 +83,7 @@ static bool _ignore_run_time_cache = false; * handler the engine will save the opline before calling the user handler: * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 */ -static zend_result ddog_php_prof_noop_opcode(zend_execute_data *execute_data) { +static zend_result ddog_php_prof_opcode_dispatch(zend_execute_data *execute_data) { return ZEND_USER_OPCODE_DISPATCH; } #endif @@ -92,13 +92,16 @@ void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_versi (void)php_version_id; #if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80100 #if PHP_VERSION_ID < 80000 - // Only need to install a handler if there isn't one, see `ddog_php_prof_noop_opcode`. + /* Only need to install a handler if there isn't one, see + * `ddog_php_prof_opcode_dispatch`. Note that the tracer installs one for + * PHP 7. + */ if (zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { #else // Issue is fixed in 8.0.26. if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { #endif - user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_noop_opcode; + user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, handler); } #endif From f53cbfa5d2f593bbe4cfaadf7d76f146bf60748f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 10 Nov 2023 08:30:00 -0700 Subject: [PATCH 07/15] work on ZEND_BIND_STATIC fix too --- profiling/src/php_ffi.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index bb5d8cbbef..0782f68d23 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -88,23 +88,39 @@ static zend_result ddog_php_prof_opcode_dispatch(zend_execute_data *execute_data } #endif -void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id) { +static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { +#if PHP_VERSION_ID < 70400 || PHP_VERSION_ID >= 80100 (void)php_version_id; -#if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80100 -#if PHP_VERSION_ID < 80000 - /* Only need to install a handler if there isn't one, see - * `ddog_php_prof_opcode_dispatch`. Note that the tracer installs one for - * PHP 7. - */ - if (zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { #else - // Issue is fixed in 8.0.26. + + /* Only need to install handlers if there isn't one, see docs on + * `ddog_php_prof_opcode_dispatch`. + */ + + /* Issue is fixed in 8.0.26: + * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a + * But the tracer installed a handler for ZEND_GENERATOR_CREATE on PHP 7, + * so nearly no users will triggered this one. + */ if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { -#endif user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, handler); } + + /* Issue is fixed in 8.0.12: + * https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a + * The tracer didn't save us here. Fortunately, throwing destru + */ + if (php_version_id < 80012 && zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { + user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; + zend_set_user_opcode_handler(ZEND_BIND_STATIC, handler); + } #endif +} + +void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id) { + ddog_php_prof_install_opcode_handlers(php_version_id); + #if CFG_RUN_TIME_CACHE // defined by build.rs _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; #endif From feeeeae89a85f95d456cbaca00fb6c278be48514 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Wed, 15 Nov 2023 16:04:03 +0100 Subject: [PATCH 08/15] ZEND_BIND_STATIC is still crashing on latest PHP --- profiling/src/php_ffi.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 0782f68d23..597085f4c0 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -75,7 +75,7 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { static bool _ignore_run_time_cache = false; #endif -#if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80100 +#if PHP_VERSION_ID >= 70400 /* The purpose here is to set the opline, because these versions are missing a * SAVE_OPLINE() and it causes a crash for the allocation profiler, see: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a @@ -89,7 +89,7 @@ static zend_result ddog_php_prof_opcode_dispatch(zend_execute_data *execute_data #endif static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { -#if PHP_VERSION_ID < 70400 || PHP_VERSION_ID >= 80100 +#if PHP_VERSION_ID < 70400 (void)php_version_id; #else @@ -97,6 +97,7 @@ static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { * `ddog_php_prof_opcode_dispatch`. */ +#if PHP_VERSION_ID < 80100 /* Issue is fixed in 8.0.26: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a * But the tracer installed a handler for ZEND_GENERATOR_CREATE on PHP 7, @@ -106,12 +107,15 @@ static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, handler); } +#endif - /* Issue is fixed in 8.0.12: + /* Part of the issue was fixed in 8.0.12: * https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a - * The tracer didn't save us here. Fortunately, throwing destru + * The tracer didn't save us here. Also the fix introduced in 8.0.12 is not + * complete, as there is a `zend_array_dup()` call before the + * `SAVE_OPLINE()` which crashes */ - if (php_version_id < 80012 && zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { + if (zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; zend_set_user_opcode_handler(ZEND_BIND_STATIC, handler); } From 795e3001aeeee35cffd9c2331f94266e3c822953 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 15 Nov 2023 14:13:57 -0700 Subject: [PATCH 09/15] test: add ZEND_BIND_STATIC test Also clean up some warnings and XFAIL a test until upstream is fixed. --- profiling/src/php_ffi.c | 18 +++---- .../tests/phpt/allocation_bind_static_01.phpt | 48 +++++++++++++++++++ profiling/tests/phpt/phpinfo_04.phpt | 5 ++ 3 files changed, 63 insertions(+), 8 deletions(-) create mode 100644 profiling/tests/phpt/allocation_bind_static_01.phpt diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 597085f4c0..83bd39bcf9 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -84,18 +84,18 @@ static bool _ignore_run_time_cache = false; * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 */ static zend_result ddog_php_prof_opcode_dispatch(zend_execute_data *execute_data) { + (void)execute_data; return ZEND_USER_OPCODE_DISPATCH; } #endif static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { -#if PHP_VERSION_ID < 70400 - (void)php_version_id; -#else +#if PHP_VERSION_ID >= 70400 /* Only need to install handlers if there isn't one, see docs on * `ddog_php_prof_opcode_dispatch`. */ + user_opcode_handler_t dispatch_handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; #if PHP_VERSION_ID < 80100 /* Issue is fixed in 8.0.26: @@ -104,21 +104,23 @@ static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { * so nearly no users will triggered this one. */ if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { - user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; - zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, handler); + zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, dispatch_handler); } +#else + (void)php_version_id; #endif /* Part of the issue was fixed in 8.0.12: * https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a * The tracer didn't save us here. Also the fix introduced in 8.0.12 is not * complete, as there is a `zend_array_dup()` call before the - * `SAVE_OPLINE()` which crashes + * `SAVE_OPLINE()` which crashes. */ if (zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { - user_opcode_handler_t handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; - zend_set_user_opcode_handler(ZEND_BIND_STATIC, handler); + zend_set_user_opcode_handler(ZEND_BIND_STATIC, dispatch_handler); } +#else + (void)php_version_id; #endif } diff --git a/profiling/tests/phpt/allocation_bind_static_01.phpt b/profiling/tests/phpt/allocation_bind_static_01.phpt new file mode 100644 index 0000000000..66a6790258 --- /dev/null +++ b/profiling/tests/phpt/allocation_bind_static_01.phpt @@ -0,0 +1,48 @@ +--TEST-- +[profiling] sampling shouldn't crash on `ZEND_BIND_STATIC` opcode +--DESCRIPTION-- +Beginning with PHP 7.4 and still on master at the time of this writing (post +PHP 8.3), the ZEND_BIND_STATIC opcode doesn't save its opline. If it occurs on +a new frame before some other opcode has saved the opline, and then the +allocation profiler (or any other thing which examines oplines) triggers, then +the invalid opline will be accessed, possibly leading to a crash. + +There was a partial fix in PHP 8.0.12 with this commit: +https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a + +But there's an `zend_array_dup` operation which can occur before this the call +to `SAVE_OPLINE()`, so if the allocation profiler triggers there, it will +access the invalid opline (non-null and invalid). + +TODO: note that this probably will not fail, because we have to take a sample +at exactly the right time, and currently we don't have a way to force this. +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +int(1) +string(1) "x" +int(1) +int(5) + diff --git a/profiling/tests/phpt/phpinfo_04.phpt b/profiling/tests/phpt/phpinfo_04.phpt index 12f9c42978..d4e80a4b3b 100644 --- a/profiling/tests/phpt/phpinfo_04.phpt +++ b/profiling/tests/phpt/phpinfo_04.phpt @@ -59,5 +59,10 @@ foreach ($sections as [$key, $expected]) { echo "Done."; ?> +--XFAIL-- +To work around a potential crash, user opcode handlers are installed. This +can cause JIT to emit a warning like: +> JIT is incompatible with third party extensions that setup user opcode handlers. +TODO: when this is fixed upstream, move this to a skipif instead. --EXPECT-- Done. From b67cd68f9824308f63ced57161034f21b1dd852e Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 15 Nov 2023 15:47:15 -0700 Subject: [PATCH 10/15] test: guard for typed properties --- profiling/tests/phpt/allocation_bind_static_01.phpt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/profiling/tests/phpt/allocation_bind_static_01.phpt b/profiling/tests/phpt/allocation_bind_static_01.phpt index 66a6790258..816a5c8df2 100644 --- a/profiling/tests/phpt/allocation_bind_static_01.phpt +++ b/profiling/tests/phpt/allocation_bind_static_01.phpt @@ -18,6 +18,8 @@ TODO: note that this probably will not fail, because we have to take a sample at exactly the right time, and currently we don't have a way to force this. --SKIPIF-- From de44fa849c32e9c361232f25182e156b5154b3ca Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 17 Nov 2023 16:43:36 -0700 Subject: [PATCH 11/15] move opcode handlers to post-startup Preloading and opcode handlers both tied to having the post-startup callback. Previously the startup callback was tied to the preload feature. This separates them. --- profiling/build.rs | 15 +++- profiling/src/bindings/mod.rs | 6 +- profiling/src/lib.rs | 45 +--------- profiling/src/php_ffi.c | 151 ++++++++++++++++++++++------------ profiling/src/php_ffi.h | 2 +- 5 files changed, 121 insertions(+), 98 deletions(-) diff --git a/profiling/build.rs b/profiling/build.rs index 6e1a7e17b8..cad218fac1 100644 --- a/profiling/build.rs +++ b/profiling/build.rs @@ -25,6 +25,7 @@ fn main() { .expect("`php-config`'s stdout to be valid utf8"); let vernum = php_config_vernum(); + let post_startup_cb = cfg_post_startup_cb(vernum); let preload = cfg_preload(vernum); let fibers = cfg_fibers(vernum); let run_time_cache = cfg_run_time_cache(vernum); @@ -33,6 +34,7 @@ fn main() { generate_bindings(php_config_includes, fibers); build_zend_php_ffis( php_config_includes, + post_startup_cb, preload, run_time_cache, fibers, @@ -81,6 +83,7 @@ const ZAI_H_FILES: &[&str] = &[ fn build_zend_php_ffis( php_config_includes: &str, + post_startup_cb: bool, preload: bool, run_time_cache: bool, fibers: bool, @@ -115,6 +118,7 @@ fn build_zend_php_ffis( println!("cargo:rustc-link-search=native={}/lib", prefix.trim()); let files = ["src/php_ffi.c", "../ext/handlers_api.c"]; + let post_startup_cb = if post_startup_cb { "1" } else { "0" }; let preload = if preload { "1" } else { "0" }; let fibers = if fibers { "1" } else { "0" }; let run_time_cache = if run_time_cache { "1" } else { "0" }; @@ -128,6 +132,7 @@ fn build_zend_php_ffis( cc::Build::new() .files(files.into_iter().chain(zai_c_files.into_iter())) + .define("CFG_POST_STARTUP_CB", post_startup_cb) .define("CFG_PRELOAD", preload) .define("CFG_FIBERS", fibers) .define("CFG_RUN_TIME_CACHE", run_time_cache) @@ -246,6 +251,15 @@ fn generate_bindings(php_config_includes: &str, fibers: bool) { .expect("bindings to be written successfully"); } +fn cfg_post_startup_cb(vernum: u64) -> bool { + if vernum >= 70300 { + println!("cargo:rustc-cfg=php_post_startup_cb"); + true + } else { + false + } +} + fn cfg_preload(vernum: u64) -> bool { if vernum >= 70400 { println!("cargo:rustc-cfg=php_preload"); @@ -300,7 +314,6 @@ fn cfg_php_feature_flags(vernum: u64) { } if vernum >= 80300 { println!("cargo:rustc-cfg=php_gc_status_extended"); - println!("cargo:rustc-cfg=php_has_php_version_id_fn"); } } diff --git a/profiling/src/bindings/mod.rs b/profiling/src/bindings/mod.rs index 3e0937f053..045ce1e046 100644 --- a/profiling/src/bindings/mod.rs +++ b/profiling/src/bindings/mod.rs @@ -318,9 +318,13 @@ extern "C" { pub fn ddog_test_php_prof_function_run_time_cache( func: &zend_function, ) -> Option<&mut [usize; 2]>; + + /// Returns the PHP_VERSION_ID of the engine at run-time, not the version + /// the extension was built against at compile-time. + pub fn ddog_php_prof_php_version_id() -> u32; } -#[cfg(php_preload)] +#[cfg(php_post_startup_cb)] extern "C" { /// Returns true after zend_post_startup_cb has been called for the current /// startup/shutdown cycle. This is useful to know. For example, diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index a8d3d90a54..b1eb3cce87 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -17,11 +17,8 @@ mod exception; #[cfg(feature = "timeline")] mod timeline; -#[cfg(not(php_has_php_version_id_fn))] -use bindings::zend_long; - use bindings as zend; -use bindings::{sapi_globals, ZendExtension, ZendResult}; +use bindings::{ddog_php_prof_php_version_id, sapi_globals, ZendExtension, ZendResult}; use clocks::*; use config::AgentEndpoint; use datadog_profiling::exporter::{Tag, Uri}; @@ -184,38 +181,6 @@ static mut PREV_EXECUTE_INTERNAL: MaybeUninit< unsafe extern "C" fn(execute_data: *mut zend::zend_execute_data, return_value: *mut zend::zval), > = MaybeUninit::uninit(); -/// # Safety -/// Only call during minit/startup phase. -unsafe fn set_run_time_php_version_id() -> anyhow::Result<()> { - cfg_if::cfg_if! { - if #[cfg(php_has_php_version_id_fn)] { - PHP_VERSION_ID = zend::php_version_id(); - Ok(()) - } else { - let vernum = b"PHP_VERSION_ID"; - let vernum_zvp = zend::zend_get_constant_str( - vernum.as_ptr() as *const c_char, - vernum.len() - ); - // SAFETY: if the pointer returned by zend_get_constant_str is not - // null, then it points to a valid zval. - let Some(vernum_zv) = vernum_zvp.as_mut() else { - anyhow::bail!("zend_get_constant_str returned null") - }; - let vernum_long = match zend_long::try_from(vernum_zv) { - Ok(x) => x, - Err(err) => { - anyhow::bail!("zend_get_constant_str(\"PHP_VERSION_ID\", ...) failed: unexpected zval type {err}"); - } - }; - let vernum_u32 = u32::try_from(vernum_long)?; - // SAFETY: during minit, there shouldn't be any threads. - PHP_VERSION_ID = vernum_u32; - Ok(()) - } - } -} - /* Important note on the PHP lifecycle: * Based on how some SAPIs work and the documentation, one might expect that * MINIT is called once per process, but this is only sort-of true. Some SAPIs @@ -257,10 +222,8 @@ extern "C" fn minit(_type: c_int, module_number: c_int) -> ZendResult { */ } - // SAFETY: calling during minit as required. - if let Err(err) = unsafe { set_run_time_php_version_id() } { - panic!("error getting PHP_VERSION_ID at run-time: {err:#}"); - } + // SAFETY: setting global mutable value in MINIT. + unsafe { PHP_VERSION_ID = ddog_php_prof_php_version_id() }; config::minit(module_number); @@ -974,7 +937,7 @@ extern "C" fn startup(extension: *mut ZendExtension) -> ZendResult { trace!("startup({:p})", extension); // Safety: called during startup hook with correct params. - unsafe { zend::datadog_php_profiling_startup(extension, PHP_VERSION_ID) }; + unsafe { zend::datadog_php_profiling_startup(extension) }; #[cfg(php_run_time_cache)] // Safety: calling this in startup/minit as required. diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 83bd39bcf9..c7647221f0 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -1,14 +1,21 @@ #include "php_ffi.h" #include -#include +#include #include +#include #include #if CFG_STACK_WALKING_TESTS #include // for dlsym #endif +#if PHP_VERSION_ID >= 70400 +#define CFG_NEED_OPCODE_HANDLERS 1 +#else +#define CFG_NEED_OPCODE_HANDLERS 0 +#endif + const char *datadog_extension_build_id(void) { return ZEND_EXTENSION_BUILD_ID; } const char *datadog_module_build_id(void) { return ZEND_MODULE_BUILD_ID; } @@ -34,99 +41,134 @@ static ddtrace_profiling_context noop_get_profiling_context(void) { return (ddtrace_profiling_context){0, 0}; } -#if CFG_PRELOAD // defined by build.rs -static bool _is_post_startup = false; - -bool ddog_php_prof_is_post_startup(void) { - return _is_post_startup; -} - -#if PHP_VERSION_ID < 80000 -#define post_startup_cb_result int +#if PHP_VERSION_ID >= 80300 +unsigned int php_version_id(void); #else -#define post_startup_cb_result zend_result -#endif - -static post_startup_cb_result (*orig_post_startup_cb)(void) = NULL; - -static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { - if (orig_post_startup_cb) { - post_startup_cb_result (*cb)(void) = orig_post_startup_cb; +// Forward declare zend_get_constant_str which will be used to polyfill the +// php_version_id function. +zval *zend_get_constant_str(const char *name, size_t name_len); + +// Error helper in rare case of error for `php_version_id` shim. +ZEND_COLD zend_never_inline ZEND_NORETURN static void exit_php_version_id(zval *constant_str) { + const char *message = constant_str != NULL + ? "error looking up PHP_VERSION_ID: expected lval return type" + : "error looking up PHP_VERSION_ID: constant not found"; + fprintf(stderr, "%s", message); + exit(EXIT_FAILURE); +} - orig_post_startup_cb = NULL; - if (cb() != SUCCESS) { - return FAILURE; - } +static unsigned int php_version_id(void) { + zval *constant_str = zend_get_constant_str(ZEND_STRL("PHP_VERSION_ID")); + if (EXPECTED(constant_str && Z_TYPE_P(constant_str))) { + return Z_LVAL_P(constant_str); } - _is_post_startup = true; - - return SUCCESS; + // This branch should be dead code, just being defensive. The constant + // PHP_VERSION_ID is registered before modules are ever registered: + // https://heap.space/xref/PHP-7.1/main/main.c?r=ccd4716e#2180 + exit_php_version_id(constant_str); } #endif - -#if CFG_RUN_TIME_CACHE // defined by build.rs /** - * Currently used to ignore run_time_cache on CLI SAPI as a precaution against - * unbounded memory growth. Unbounded growth is more likely there since it's - * always one PHP request, and we only reset it on each new request. + * Returns the PHP_VERSION_ID of the engine at run-time, not the version the + * extension was built against at compile-time. */ -static bool _ignore_run_time_cache = false; -#endif +uint32_t ddog_php_prof_php_version_id(void) { return php_version_id(); } -#if PHP_VERSION_ID >= 70400 -/* The purpose here is to set the opline, because these versions are missing a - * SAVE_OPLINE() and it causes a crash for the allocation profiler, see: - * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a +#if CFG_POST_STARTUP_CB // defined by build.rs +static bool _is_post_startup = false; + +bool ddog_php_prof_is_post_startup(void) { + return _is_post_startup; +} + +#if CFG_NEED_OPCODE_HANDLERS +/* The purpose here is to set the opline, because the built-in opcode handlers + * for some versions are missing a SAVE_OPLINE() and it causes a crash when + * trying to access the line number. The allocation profiler is vulnerable in + * particular because it can access the opline on any allocation. + * * The handler doesn't actually need to do anything, because just by having a - * handler the engine will save the opline before calling the user handler: + * user opcode handler the engine will save the opline before calling the user + * handler: * https://heap.space/xref/PHP-7.4/Zend/zend_vm_execute.h?r=0b7dffb4#2650 */ static zend_result ddog_php_prof_opcode_dispatch(zend_execute_data *execute_data) { (void)execute_data; return ZEND_USER_OPCODE_DISPATCH; } -#endif +// Argument `php_version_id` should be the version of PHP at runtime, not the +// version this was compiled against. static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { -#if PHP_VERSION_ID >= 70400 - /* Only need to install handlers if there isn't one, see docs on - * `ddog_php_prof_opcode_dispatch`. + /* Only need to install user opcode handlers if there isn't one already + * for the given opcode, see the docs on `ddog_php_prof_opcode_dispatch`. */ user_opcode_handler_t dispatch_handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; -#if PHP_VERSION_ID < 80100 /* Issue is fixed in 8.0.26: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a - * But the tracer installed a handler for ZEND_GENERATOR_CREATE on PHP 7, - * so nearly no users will triggered this one. */ if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, dispatch_handler); } -#else - (void)php_version_id; -#endif /* Part of the issue was fixed in 8.0.12: * https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a - * The tracer didn't save us here. Also the fix introduced in 8.0.12 is not - * complete, as there is a `zend_array_dup()` call before the - * `SAVE_OPLINE()` which crashes. + * However, the fix is not complete as it's possible for the opcode to + * call `zend_array_dup()` before the `SAVE_OPLINE()`. */ if (zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { zend_set_user_opcode_handler(ZEND_BIND_STATIC, dispatch_handler); } -#else - (void)php_version_id; +#if PHP_VERSION_ID >= 80400 +#error Check if ZEND_BIND_STATIC needs an opcode handler still. #endif } +#endif + +#if PHP_VERSION_ID < 80000 +#define post_startup_cb_result int +#else +#define post_startup_cb_result zend_result +#endif + +static post_startup_cb_result (*orig_post_startup_cb)(void) = NULL; + +static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { + if (orig_post_startup_cb) { + post_startup_cb_result (*cb)(void) = orig_post_startup_cb; + + orig_post_startup_cb = NULL; + if (cb() != SUCCESS) { + return FAILURE; + } + } -void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id) { + _is_post_startup = true; + +#if CFG_NEED_OPCODE_HANDLERS + uint32_t php_version_id = ddog_php_prof_php_version_id(); ddog_php_prof_install_opcode_handlers(php_version_id); +#endif + + return SUCCESS; +} +#endif + +#if CFG_RUN_TIME_CACHE // defined by build.rs +/** + * Currently used to ignore run_time_cache on CLI SAPI as a precaution against + * unbounded memory growth. Unbounded growth is more likely there since it's + * always one PHP request, and we only reset it on each new request. + */ +static bool _ignore_run_time_cache = false; +#endif + +void datadog_php_profiling_startup(zend_extension *extension) { #if CFG_RUN_TIME_CACHE // defined by build.rs _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; #endif @@ -147,8 +189,9 @@ void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_versi } } -#if CFG_PRELOAD // defined by build.rs +#if CFG_POST_STARTUP_CB // defined by build.rs _is_post_startup = false; + orig_post_startup_cb = zend_post_startup_cb; zend_post_startup_cb = ddog_php_prof_post_startup_cb; #endif diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index 84e00e62cd..3eea8468da 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -71,7 +71,7 @@ extern ddtrace_profiling_context (*datadog_php_profiling_get_profiling_context)( * burdensome in Rust, like locating the ddtrace extension in the module * registry and finding the ddtrace_get_profiling_context function. */ -void datadog_php_profiling_startup(zend_extension *extension, uint32_t php_version_id); +void datadog_php_profiling_startup(zend_extension *extension); /** * Used to hold information for overwriting the internal function handler From 54ab67928174a3dd953771386e56fe759efa92aa Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 17 Nov 2023 16:56:33 -0700 Subject: [PATCH 12/15] test: remove temporary xfail --- profiling/tests/phpt/phpinfo_04.phpt | 5 ----- 1 file changed, 5 deletions(-) diff --git a/profiling/tests/phpt/phpinfo_04.phpt b/profiling/tests/phpt/phpinfo_04.phpt index d4e80a4b3b..12f9c42978 100644 --- a/profiling/tests/phpt/phpinfo_04.phpt +++ b/profiling/tests/phpt/phpinfo_04.phpt @@ -59,10 +59,5 @@ foreach ($sections as [$key, $expected]) { echo "Done."; ?> ---XFAIL-- -To work around a potential crash, user opcode handlers are installed. This -can cause JIT to emit a warning like: -> JIT is incompatible with third party extensions that setup user opcode handlers. -TODO: when this is fixed upstream, move this to a skipif instead. --EXPECT-- Done. From 1b6cbc31c175a4713c0fcce3cf213f7e3967c98f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 20 Nov 2023 11:10:03 -0700 Subject: [PATCH 13/15] [ci skip] elaborate on PHP 8.4 todo --- profiling/src/php_ffi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index c7647221f0..450c1425b0 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -124,7 +124,7 @@ static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { zend_set_user_opcode_handler(ZEND_BIND_STATIC, dispatch_handler); } #if PHP_VERSION_ID >= 80400 -#error Check if ZEND_BIND_STATIC needs an opcode handler still. +#error Check if ZEND_BIND_STATIC needs an opcode handler still. Possibly update things like CFG_NEED_OPCODE_HANDLERS as well. #endif } #endif From 198663d26d8c19b086f6d74ab6c7896d2eed0b7f Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Fri, 22 Dec 2023 12:14:39 +0100 Subject: [PATCH 14/15] update php versions --- profiling/src/php_ffi.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 450c1425b0..6d5b67ce5c 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -10,7 +10,7 @@ #include // for dlsym #endif -#if PHP_VERSION_ID >= 70400 +#if PHP_VERSION_ID >= 70400 && PHP_VERSION_ID < 80400 #define CFG_NEED_OPCODE_HANDLERS 1 #else #define CFG_NEED_OPCODE_HANDLERS 0 @@ -108,23 +108,42 @@ static void ddog_php_prof_install_opcode_handlers(uint32_t php_version_id) { */ user_opcode_handler_t dispatch_handler = (user_opcode_handler_t)ddog_php_prof_opcode_dispatch; +#if PHP_VERSION_ID < 80100 /* Issue is fixed in 8.0.26: * https://github.com/php/php-src/commit/26c7c82d32dad841dd151ebc6a31b8ea6f93f94a */ if (php_version_id < 80026 && zend_get_user_opcode_handler(ZEND_GENERATOR_CREATE) == NULL) { zend_set_user_opcode_handler(ZEND_GENERATOR_CREATE, dispatch_handler); } +#endif +#if PHP_VERSION_ID < 80400 /* Part of the issue was fixed in 8.0.12: * https://github.com/php/php-src/commit/ec54ffad1e3b15fedfd07f7d29d97ec3e8d1c45a * However, the fix is not complete as it's possible for the opcode to * call `zend_array_dup()` before the `SAVE_OPLINE()`. + * + * This was finally fixed with https://github.com/php/php-src/pull/12758 in + * PHP 8.1.27, 8.2.14 and 8.3.1 */ - if (zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) { + if ((php_version_id < 80127 || + (php_version_id >= 80200 && php_version_id < 80214) || + php_version_id == 80300) && + zend_get_user_opcode_handler(ZEND_BIND_STATIC) == NULL) + { zend_set_user_opcode_handler(ZEND_BIND_STATIC, dispatch_handler); } -#if PHP_VERSION_ID >= 80400 -#error Check if ZEND_BIND_STATIC needs an opcode handler still. Possibly update things like CFG_NEED_OPCODE_HANDLERS as well. + + /* This was fixed with https://github.com/php/php-src/pull/12768 in + * PHP 8.1.27, 8.2.14 and 8.3.1 + */ + if ((php_version_id < 80127 || + (php_version_id >= 80200 && php_version_id < 80214) || + php_version_id == 80300) && + zend_get_user_opcode_handler(ZEND_FUNC_GET_ARGS) == NULL) + { + zend_set_user_opcode_handler(ZEND_FUNC_GET_ARGS, dispatch_handler); + } #endif } #endif From c00421391f7f82a46adeaf043605053647518a47 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 28 Dec 2023 10:43:15 -0700 Subject: [PATCH 15/15] test: add phpt for ZEND_FUNC_GET_ARGS opline issue --- .../tests/phpt/allocation_func_get_args.phpt | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 profiling/tests/phpt/allocation_func_get_args.phpt diff --git a/profiling/tests/phpt/allocation_func_get_args.phpt b/profiling/tests/phpt/allocation_func_get_args.phpt new file mode 100644 index 0000000000..ce14cbc29c --- /dev/null +++ b/profiling/tests/phpt/allocation_func_get_args.phpt @@ -0,0 +1,53 @@ +--TEST-- +[profiling] sampling shouldn't crash on `ZEND_FUNC_GET_ARGS` opcode +--DESCRIPTION-- +Beginning with PHP 7.4, the ZEND_FUNC_GET_ARGS opcode doesn't save its opline. +If it occurs on a new frame before some other opcode has saved the opline, and +then the allocation profiler triggers (or any other thing which examines +oplines like the error message when hitting the memory limit), then the +invalid opline will be accessed, possibly leading to a crash. + +Fixed in PHP 8.1.27, 8.2.14 and 8.3.1: +https://github.com/php/php-src/pull/12768 + +This test shouldn't crash even on affected versions, because the profiler +should mitigate the issue with a user opcode handler. However, it's difficult +to trigger at exactly the right (wrong?) time anyway, so it's unlikely to +crash anyway. +TODO: run this in some mode which will look at the opline on every allocation. +--SKIPIF-- + +--FILE-- + +--EXPECT-- +int(1) +string(1) "x" +int(1) +array(2) { + [0]=> + string(6) "string" + [1]=> + int(0) +} +Done. \ No newline at end of file