refactor(profiling): less unsafe code (#2739)

* refactor(profiling): auto-derive Sync for Profiler

* refactor(profiling): make PHP_VERSION_ID safe

profiling/src/allocation.rs

@@ -9,7 +9,8 @@ use log::{debug, error, trace, warn};
 use rand::rngs::ThreadRng;
 use rand_distr::{Distribution, Poisson};
 use std::cell::{RefCell, UnsafeCell};
-use std::sync::atomic::{AtomicU64, Ordering::SeqCst};
+use std::sync::atomic::AtomicU64;
+use std::sync::atomic::Ordering::{Relaxed, SeqCst};
 use std::{ffi, ptr};
 
 static mut GC_MEM_CACHES_HANDLER: zend::InternalFunctionHandler = None;
@@ -154,7 +155,7 @@ pub fn alloc_prof_minit() {
 
 pub fn first_rinit_should_disable_due_to_jit() -> bool {
     if NEEDS_RUN_TIME_CHECK_FOR_ENABLED_JIT
-        && alloc_prof_needs_disabled_for_jit(unsafe { crate::RUNTIME_PHP_VERSION_ID })
+        && alloc_prof_needs_disabled_for_jit(crate::RUNTIME_PHP_VERSION_ID.load(Relaxed))
         && *JIT_ENABLED
     {
         error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.1.21 or 8.2.8. See https://github.com/DataDog/dd-trace-php/pull/2088");

profiling/src/lib.rs

@@ -65,7 +65,7 @@ static PROFILER_VERSION: &[u8] = concat!(include_str!("../../VERSION"), "\0").as
 
 /// Version ID of PHP at run-time, not the version it was built against at
 /// compile-time. Its value is overwritten during minit.
-static mut RUNTIME_PHP_VERSION_ID: u32 = zend::PHP_VERSION_ID;
+static RUNTIME_PHP_VERSION_ID: AtomicU32 = AtomicU32::new(zend::PHP_VERSION_ID);
 
 /// Version str of PHP at run-time, not the version it was built against at
 /// compile-time. Its value is overwritten during minit, unless there are
@@ -214,8 +214,10 @@ extern "C" fn minit(_type: c_int, module_number: c_int) -> ZendResult {
 
     // Update the runtime PHP_VERSION and PHP_VERSION_ID.
     {
-        // SAFETY: These are safe to call and mutate in minit.
-        unsafe { RUNTIME_PHP_VERSION_ID = ddog_php_prof_php_version_id() };
+        // SAFETY: safe to call any time in a module because the engine is
+        // initialized before modules are ever loaded.
+        let php_version_id = unsafe { ddog_php_prof_php_version_id() };
+        RUNTIME_PHP_VERSION_ID.store(php_version_id, Ordering::Relaxed);
 
         // SAFETY: calling zero-arg fn that is safe to call in minit.
         let ptr = unsafe { ddog_php_prof_php_version() };
@@ -420,8 +422,8 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
 
     unsafe { bindings::zai_config_rinit() };
 
-    // Safety: We are after first rinit and before config mshutdown.
-    let system_settings = unsafe { SystemSettings::get() };
+    // SAFETY: We are after first rinit and before config mshutdown.
+    let mut system_settings = unsafe { SystemSettings::get() };
 
     // initialize the thread local storage and cache some items
     REQUEST_LOCALS.with(|cell| {
@@ -450,7 +452,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
     });
 
     // SAFETY: still safe to access in rinit after first_rinit.
-    let system_settings = unsafe { system_settings.as_ref() };
+    let system_settings = unsafe { system_settings.as_mut() };
 
     // SAFETY: the once control is not mutable during request.
     let once = unsafe { &*ptr::addr_of!(RINIT_ONCE) };

profiling/src/profiling/mod.rs

@@ -31,7 +31,7 @@ use std::collections::HashMap;
 use std::hash::Hash;
 use std::intrinsics::transmute;
 use std::num::NonZeroI64;
-use std::sync::atomic::{AtomicBool, AtomicU32, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU32, Ordering};
 use std::sync::{Arc, Barrier};
 use std::thread::JoinHandle;
 use std::time::{Duration, Instant, SystemTime};
@@ -207,17 +207,9 @@ pub struct Profiler {
     uploader_handle: JoinHandle<()>,
     should_join: AtomicBool,
     sample_types_filter: SampleTypeFilter,
-    system_settings: ptr::NonNull<SystemSettings>,
+    system_settings: AtomicPtr<SystemSettings>,
 }
 
-/// The [Profiler] is sync *except* for some edge cases system_settings, which
-/// may be mutated under fringe conditions like in the child of a fork.
-unsafe impl Sync for Profiler {}
-
-/// The [Profiler] is send *except* for some edge cases system_settings, which
-/// may be mutated under fringe conditions like in the child of a fork.
-unsafe impl Send for Profiler {}
-
 struct TimeCollector {
     fork_barrier: Arc<Barrier>,
     interrupt_manager: Arc<InterruptManager>,
@@ -519,7 +511,7 @@ pub enum UploadMessage {
 const COW_EVAL: Cow<str> = Cow::Borrowed("[eval]");
 
 impl Profiler {
-    pub fn new(system_settings: &SystemSettings) -> Self {
+    pub fn new(system_settings: &mut SystemSettings) -> Self {
         let fork_barrier = Arc::new(Barrier::new(3));
         let interrupt_manager = Arc::new(InterruptManager::new());
         let (message_sender, message_receiver) = crossbeam_channel::bounded(100);
@@ -558,7 +550,7 @@ impl Profiler {
             }),
             should_join: AtomicBool::new(true),
             sample_types_filter,
-            system_settings: ptr::NonNull::from(system_settings),
+            system_settings: AtomicPtr::new(system_settings),
         }
     }
 
@@ -693,12 +685,14 @@ impl Profiler {
 
                 let labels = Profiler::common_labels(0);
                 let mut timestamp = 0;
-                // SAFETY: collecting time is done during PHP requests only,
-                //         so the ptr is valid at this time.
                 #[cfg(feature = "timeline")]
-                if unsafe { self.system_settings.as_ref() }.profiling_timeline_enabled {
-                    if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) {
-                        timestamp = now.as_nanos() as i64;
+                {
+                    let system_settings = self.system_settings.load(Ordering::SeqCst);
+                    // SAFETY: system settings are stable during a request.
+                    if unsafe { *ptr::addr_of!((*system_settings).profiling_timeline_enabled) } {
+                        if let Ok(now) = SystemTime::now().duration_since(UNIX_EPOCH) {
+                            timestamp = now.as_nanos() as i64;
+                        }
                     }
                 }
 
@@ -1134,7 +1128,7 @@ mod tests {
         settings.profiling_experimental_cpu_time_enabled = true;
         settings.profiling_timeline_enabled = true;
 
-        let profiler = Profiler::new(&settings);
+        let profiler = Profiler::new(&mut settings);
 
         let message: SampleMessage = profiler.prepare_sample_message(frames, samples, labels, 900);