From de44fa849c32e9c361232f25182e156b5154b3ca Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 17 Nov 2023 16:43:36 -0700 Subject: [PATCH] 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