From 631d4fff25bc59bac73f75103694d51e61d85e8b Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Tue, 14 Jan 2025 14:44:15 +0100 Subject: [PATCH] Add missing "in-preloading" check to `RSHUTDOWN` `RSHUTDOWN` was missing the "in-preloading" check which would lead to a situation where some profilers would collect data even though in preloading. In allocation profiling this would lead to a situation where if a customer has a custom memory handler (or uses `USE_ZEND_ALLOC=0`) the allocation profiler would crash in `allocation::alloc_prof_rshutdown()` when trying to reset the heap. --- profiling/src/lib.rs | 40 ++++++++++++++++------------ profiling/tests/phpt/preload_01.phpt | 6 ++--- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index 3b3db288a02..b400ad70b94 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -491,6 +491,23 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { locals.system_settings = system_settings; }); + // Preloading happens before zend_post_startup_cb is called for the first + // time. When preloading is enabled and a non-root user is used for + // php-fpm, there is fork that happens. In the past, having the profiler + // enabled at this time would cause php-fpm eventually hang once the + // Profiler's channels were full; this has been fixed. See: + // https://github.com/DataDog/dd-trace-php/issues/1919 + // + // There are a few ways to handle this preloading scenario with the fork, + // but the simplest is to not enable the profiler until the engine's + // startup is complete. This means the preloading will not be profiled, + // but this should be okay. + #[cfg(php_preload)] + if !unsafe { bindings::ddog_php_prof_is_post_startup() } { + debug!("zend_post_startup_cb hasn't happened yet; not enabling profiler."); + return ZendResult::Success; + } + // SAFETY: still safe to access in rinit after first_rinit. let system_settings = unsafe { system_settings.as_mut() }; @@ -531,23 +548,6 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { exception::exception_profiling_first_rinit(); }); - // Preloading happens before zend_post_startup_cb is called for the first - // time. When preloading is enabled and a non-root user is used for - // php-fpm, there is fork that happens. In the past, having the profiler - // enabled at this time would cause php-fpm eventually hang once the - // Profiler's channels were full; this has been fixed. See: - // https://github.com/DataDog/dd-trace-php/issues/1919 - // - // There are a few ways to handle this preloading scenario with the fork, - // but the simplest is to not enable the profiler until the engine's - // startup is complete. This means the preloading will not be profiled, - // but this should be okay. - #[cfg(php_preload)] - if !unsafe { bindings::ddog_php_prof_is_post_startup() } { - debug!("zend_post_startup_cb hasn't happened yet; not enabling profiler."); - return ZendResult::Success; - } - Profiler::init(system_settings); if system_settings.profiling_enabled { @@ -631,6 +631,12 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult { #[cfg(debug_assertions)] trace!("RSHUTDOWN({_type}, {_module_number})"); + #[cfg(php_preload)] + if !unsafe { bindings::ddog_php_prof_is_post_startup() } { + debug!("zend_post_startup_cb hasn't happened yet; not disabling profiler."); + return ZendResult::Success; + } + profiling::stack_walking::rshutdown(); REQUEST_LOCALS.with(|cell| { diff --git a/profiling/tests/phpt/preload_01.phpt b/profiling/tests/phpt/preload_01.phpt index 17c4629f2ca..30710cf4087 100644 --- a/profiling/tests/phpt/preload_01.phpt +++ b/profiling/tests/phpt/preload_01.phpt @@ -12,18 +12,18 @@ See: https://github.com/DataDog/dd-trace-php/issues/1919 Due to limitations of PHPT, this test does not test PHP-FPM or processes run with different permissions, but it makes sure, that profiling is started after preloading is done. +--ENV-- +USE_ZEND_ALLOC=0 --SKIPIF-- = 7.4.0", PHP_EOL; if (!extension_loaded('datadog-profiling')) echo "skip: test requires datadog-profiling", PHP_EOL; ?> --INI-- datadog.profiling.enabled=yes datadog.profiling.log_level=debug -datadog.profiling.allocation_enabled=no -datadog.profiling.experimental_cpu_time_enabled=no zend_extension=opcache opcache.enable_cli=1 opcache.preload={PWD}/preload_01_preload.php