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 dae8db5034..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); diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 0c37745690..6d5b67ce5c 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 && PHP_VERSION_ID < 80400 +#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,13 +41,113 @@ static ddtrace_profiling_context noop_get_profiling_context(void) { return (ddtrace_profiling_context){0, 0}; } -#if CFG_PRELOAD // defined by build.rs +#if PHP_VERSION_ID >= 80300 +unsigned int php_version_id(void); +#else +// 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); +} + +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); + } + + // 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 + +/** + * Returns the PHP_VERSION_ID of the engine at run-time, not the version the + * extension was built against at compile-time. + */ +uint32_t ddog_php_prof_php_version_id(void) { return php_version_id(); } + +#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 + * 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; +} + +// 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) { + + /* 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 + */ + 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 ((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); + } + + /* 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 + #if PHP_VERSION_ID < 80000 #define post_startup_cb_result int #else @@ -61,6 +168,11 @@ static post_startup_cb_result ddog_php_prof_post_startup_cb(void) { _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 @@ -76,8 +188,7 @@ 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 +#if CFG_RUN_TIME_CACHE // defined by build.rs _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; #endif @@ -97,8 +208,9 @@ void datadog_php_profiling_startup(zend_extension *extension) { } } -#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/tests/phpt/allocation_bind_static_01.phpt b/profiling/tests/phpt/allocation_bind_static_01.phpt new file mode 100644 index 0000000000..816a5c8df2 --- /dev/null +++ b/profiling/tests/phpt/allocation_bind_static_01.phpt @@ -0,0 +1,50 @@ +--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/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 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