fix(profiling): potential allocation profiling crashes with certain o…

…pcodes (#2352)

Co-authored-by: Florian Engelhardt <florian.engelhardt@datadoghq.com>

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");
     }
 }
 

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,

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);
 

profiling/src/php_ffi.c

@@ -1,14 +1,21 @@
 #include "php_ffi.h"
 
 #include <assert.h>
-#include <stdbool.h>
+#include <stdlib.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <string.h>
 
 #if CFG_STACK_WALKING_TESTS
 #include <dlfcn.h> // 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

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--
+<?php
+if (PHP_VERSION_ID < 70400)
+    echo "skip: test requires typed properties", PHP_EOL;
+if (!extension_loaded('datadog-profiling'))
+    echo "skip: test requires datadog-profiling", PHP_EOL;
+?>
+--FILE--
+<?php
+function &ref() {
+    static $a = 5;
+    return $a;
+}
+
+class Foo {
+    public static int $i;
+    public static string $s = "x";
+}
+
+var_dump(Foo::$i = "1");
+var_dump(Foo::$s, Foo::$i);
+
+// Crash was here.
+var_dump(ref());
+
+?>
+--EXPECTF--
+int(1)
+string(1) "x"
+int(1)
+int(5)
+