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

@@ -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);
 
@@ -940,7 +903,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.

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
+#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

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