From c01ce9b4fab04dfce9a4940462b6eed230ca5655 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Wed, 14 Jan 2026 12:18:16 +0100 Subject: [PATCH 1/4] Problem: When compiling on Alpine Linux (musl libc) and running on glibc systems, pthread_once has incompatible internal state machines that cause crashes after fork(). This was observed as SIGSEGV in PHP binaries that use forking when returning from the pthread_once API. Root cause: - musl uses state value 1 during pthread_once initialization - glibc uses state value (__fork_generation | 1), typically 5 - After fork(), glibc sees musl's state=1, treats it as invalid for the current fork generation, and re-initializes - This causes double initialization of pthread keys Solution: Replaced pthread_once with atomic-based initialization in AllocationTracker::get_tl_state(): - Uses std::atomic with three states: kKeyNotInitialized, kKeyInitializing, kKeyInitialized - Hot path optimized with relaxed atomic load - Slow path uses compare-and-swap with proper memory ordering - Added safety timeout to prevent infinite loops - Returns nullptr on timeout --- include/lib/allocation_tracker.hpp | 2 +- src/lib/allocation_tracker.cc | 42 +++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/include/lib/allocation_tracker.hpp b/include/lib/allocation_tracker.hpp index ef7c9705..b17a56e2 100644 --- a/include/lib/allocation_tracker.hpp +++ b/include/lib/allocation_tracker.hpp @@ -160,7 +160,7 @@ class AllocationTracker { // These can not be tied to the internal state of the instance. // The creation of the instance depends on this - static pthread_once_t _key_once; // ensures we call key creation a single time + static std::atomic _key_init_state; static pthread_key_t _tl_state_key; static AllocationTracker *_instance; diff --git a/src/lib/allocation_tracker.cc b/src/lib/allocation_tracker.cc index e7a2981d..ff6361ab 100644 --- a/src/lib/allocation_tracker.cc +++ b/src/lib/allocation_tracker.cc @@ -3,7 +3,7 @@ // developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present // Datadog, Inc. -#include "allocation_tracker.hpp" +#include "lib/allocation_tracker.hpp" #include "allocation_event.hpp" #include "ddprof_perf_event.hpp" @@ -23,12 +23,20 @@ #include #include #include +#include #include namespace ddprof { +namespace { +// TLS key initialization states +constexpr int kKeyNotInitialized = 0; +constexpr int kKeyInitializing = 1; +constexpr int kKeyInitialized = 2; +} // namespace + // Static declarations -pthread_once_t AllocationTracker::_key_once = PTHREAD_ONCE_INIT; +std::atomic AllocationTracker::_key_init_state{kKeyNotInitialized}; pthread_key_t AllocationTracker::_tl_state_key; @@ -56,7 +64,35 @@ TrackerThreadLocalState *AllocationTracker::get_tl_state() { // In shared libraries, TLS access requires a call to tls_get_addr, // tls_get_addr can call into malloc, which can create a recursive loop // instead we call pthread APIs to control the creation of TLS objects - pthread_once(&_key_once, make_key); + + // Thread-safe initialization using atomics (avoids pthread_once ABI issues) + + // Fast path: relaxed load is sufficient since we only care if it's + // initialized Once initialized, this value never changes, so no + // synchronization needed + if (_key_init_state.load(std::memory_order_relaxed) != kKeyInitialized) { + // Slow path: need proper synchronization for initialization + int expected = kKeyNotInitialized; + if (_key_init_state.compare_exchange_strong(expected, kKeyInitializing, + std::memory_order_acq_rel)) { + // We won the race, do the initialization + make_key(); + _key_init_state.store(kKeyInitialized, std::memory_order_release); + } else { + // Another thread is initializing or already done, wait until complete + constexpr int k_max_init_wait_attempts = 100; + int attempts = 0; + while (_key_init_state.load(std::memory_order_acquire) != + kKeyInitialized) { + if (++attempts >= k_max_init_wait_attempts) { + return nullptr; + } + std::this_thread::yield(); + std::this_thread::sleep_for(std::chrono::microseconds(1)); + } + } + } + auto *tl_state = static_cast( pthread_getspecific(_tl_state_key)); return tl_state; From e8c8a830b1b3f4da8c8aa20df5d034860fde7b99 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Wed, 14 Jan 2026 12:55:06 +0100 Subject: [PATCH 2/4] Allocation tracker fork test - Cover TLS init with forking --- test/CMakeLists.txt | 32 +++++ test/allocation_tracker_fork_test.c | 143 +++++++++++++++++++ test/allocation_tracker_fork_test_wrapper.cc | 19 +++ 3 files changed, 194 insertions(+) create mode 100644 test/allocation_tracker_fork_test.c create mode 100644 test/allocation_tracker_fork_test_wrapper.cc diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 01d4e625..fb26e7b7 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -393,6 +393,38 @@ add_unit_test(address_bitset-ut address_bitset-ut.cc ../src/lib/address_bitset.c add_unit_test(lib_logger-ut ./lib_logger-ut.cc) +# Manual test for allocation_tracker fork behavior (no gtest due to TLS issues) +add_exe( + allocation_tracker_fork_test + allocation_tracker_fork_test.c + allocation_tracker_fork_test_wrapper.cc + ../src/lib/allocation_tracker.cc + ../src/lib/address_bitset.cc + ../src/logger.cc + ../src/lib/pthread_fixes.cc + ../src/lib/savecontext.cc + ../src/lib/saveregisters.cc + ../src/procutils.cc + ../src/ratelimiter.cc + ../src/ringbuffer_utils.cc + ../src/sys_utils.cc + ../src/tsc_clock.cc + ../src/perf_clock.cc + ../src/perf.cc + ../src/ddres_list.cc + ../src/perf_ringbuffer.cc + ../src/pevent_lib.cc + ../src/user_override.cc + LIBRARIES llvm-demangle) +target_include_directories( + allocation_tracker_fork_test PRIVATE ${CMAKE_SOURCE_DIR}/include ${CMAKE_SOURCE_DIR}/include/lib + ${CMAKE_SOURCE_DIR}/src) + +add_test( + NAME allocation_tracker_fork_test + COMMAND allocation_tracker_fork_test + WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) + add_unit_test( create_elf-ut create_elf-ut.cc diff --git a/test/allocation_tracker_fork_test.c b/test/allocation_tracker_fork_test.c new file mode 100644 index 00000000..b5b2b344 --- /dev/null +++ b/test/allocation_tracker_fork_test.c @@ -0,0 +1,143 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. This product includes software +// developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present +// Datadog, Inc. + +// Simple test to verify pthread key initialization works across fork() +// This tests the atomic-based pthread_once replacement + +#include +#include +#include +#include +#include +#include + +// Forward declare the C++ functions we need +#ifdef __cplusplus +extern "C" { +#endif + +void *AllocationTracker_get_tl_state(void); +void *AllocationTracker_init_tl_state(void); + +#ifdef __cplusplus +} +#endif + +// Thread function to test TLS in new thread +static void *thread_test_func(void *arg) { + int *result = (int *)arg; + + void *thread_state = AllocationTracker_get_tl_state(); + + if (thread_state != NULL) { + fprintf(stderr, "FAIL: New thread expected NULL TLS, got %p\n", + thread_state); + *result = 1; + return NULL; + } + + *result = 0; + return NULL; +} + +int main(void) { + // Initialize pthread key and create TLS data in parent + void *state_before = AllocationTracker_get_tl_state(); + + void *parent_state = AllocationTracker_init_tl_state(); + if (parent_state == NULL) { + fprintf(stderr, "FAIL: Parent init_tl_state() returned NULL\n"); + return 1; + } + + void *retrieved = AllocationTracker_get_tl_state(); + if (retrieved != parent_state) { + fprintf(stderr, "FAIL: Parent get_tl_state() mismatch\n"); + return 1; + } + + // Test fork - this is where the old pthread_once bug would crash + fflush(stdout); + fflush(stderr); + + pid_t pid = fork(); + if (pid == -1) { + perror("FAIL: fork"); + return 1; + } + + if (pid == 0) { + // Child process - test pthread key works after fork + void *child_inherited = AllocationTracker_get_tl_state(); + + void *child_state = AllocationTracker_init_tl_state(); + if (child_state == NULL) { + fprintf(stderr, "FAIL: Child init_tl_state() returned NULL\n"); + _exit(1); + } + + void *child_retrieved = AllocationTracker_get_tl_state(); + if (child_retrieved != child_state) { + fprintf(stderr, "FAIL: Child pthread key broken after fork\n"); + _exit(2); + } + + // Test that a new thread starts with NULL TLS + pthread_t thread; + int thread_result = -1; + if (pthread_create(&thread, NULL, thread_test_func, &thread_result) != 0) { + fprintf(stderr, "FAIL: pthread_create failed\n"); + _exit(3); + } + + pthread_join(thread, NULL); + if (thread_result != 0) { + _exit(4); + } + + _exit(0); + } + + // Parent waits for child + int status; + waitpid(pid, &status, 0); + + if (!WIFEXITED(status)) { + if (WIFSIGNALED(status)) { + fprintf(stderr, + "FAIL: Child crashed with signal %d (pthread_once ABI bug!)\n", + WTERMSIG(status)); + } else { + fprintf(stderr, "FAIL: Child did not exit normally\n"); + } + return 1; + } + + int exit_code = WEXITSTATUS(status); + if (exit_code != 0) { + const char *reason = "unknown"; + if (exit_code == 1) + reason = "init_tl_state failed"; + else if (exit_code == 2) + reason = "pthread key broken"; + else if (exit_code == 3) + reason = "pthread_create failed"; + else if (exit_code == 4) + reason = "thread test failed"; + fprintf(stderr, "FAIL: Child exited with code %d (%s)\n", exit_code, + reason); + return 1; + } + + // Verify parent still works after fork + void *parent_after = AllocationTracker_get_tl_state(); + if (parent_after != parent_state) { + fprintf(stderr, "FAIL: Parent TLS corrupted after fork\n"); + return 1; + } + + // All tests passed - silent success for CI + return 0; +} diff --git a/test/allocation_tracker_fork_test_wrapper.cc b/test/allocation_tracker_fork_test_wrapper.cc new file mode 100644 index 00000000..ece43d22 --- /dev/null +++ b/test/allocation_tracker_fork_test_wrapper.cc @@ -0,0 +1,19 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. This product includes software +// developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present +// Datadog, Inc. + +// C++ wrapper to expose AllocationTracker functions to C test + +#include "lib/allocation_tracker.hpp" + +extern "C" { + +void *AllocationTracker_get_tl_state(void) { + return ddprof::AllocationTracker::get_tl_state(); +} + +void *AllocationTracker_init_tl_state(void) { + return ddprof::AllocationTracker::init_tl_state(); +} +} From 57ba49e006d0e0474db38da27112f29404a1a4f6 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Thu, 29 Jan 2026 10:38:52 +0100 Subject: [PATCH 3/4] Allocation profiling crash - Fork safety - Use the key as synchronization - Avoid init during a get of the thread local state - Add a fork handler to ensure we have the key initialized --- include/lib/allocation_tracker.hpp | 11 ++- src/lib/allocation_tracker.cc | 93 ++++++++++---------- test/allocation_tracker_fork_test.c | 13 +-- test/allocation_tracker_fork_test_wrapper.cc | 4 + 4 files changed, 66 insertions(+), 55 deletions(-) diff --git a/include/lib/allocation_tracker.hpp b/include/lib/allocation_tracker.hpp index b17a56e2..17fc04a5 100644 --- a/include/lib/allocation_tracker.hpp +++ b/include/lib/allocation_tracker.hpp @@ -80,6 +80,11 @@ class AllocationTracker { // can return null (does not init) static TrackerThreadLocalState *get_tl_state(); + // Initialize pthread key for TLS (idempotent, fork-safe) + static void ensure_key_initialized(); + // Register pthread_atfork handler (called during init) + static void register_fork_handler(); + private: static constexpr unsigned k_ratio_max_elt_to_bitset_size = 16; @@ -120,8 +125,6 @@ class AllocationTracker { static void delete_tl_state(void *tl_state); - static void make_key(); - void track_allocation(uintptr_t addr, size_t size, TrackerThreadLocalState &tl_state, bool is_large_alloc); void track_deallocation(uintptr_t addr, TrackerThreadLocalState &tl_state, @@ -160,8 +163,8 @@ class AllocationTracker { // These can not be tied to the internal state of the instance. // The creation of the instance depends on this - static std::atomic _key_init_state; - static pthread_key_t _tl_state_key; + static constexpr pthread_key_t kInvalidKey = static_cast(-1); + static std::atomic _tl_state_key; static AllocationTracker *_instance; }; diff --git a/src/lib/allocation_tracker.cc b/src/lib/allocation_tracker.cc index ff6361ab..be611344 100644 --- a/src/lib/allocation_tracker.cc +++ b/src/lib/allocation_tracker.cc @@ -28,19 +28,9 @@ namespace ddprof { -namespace { -// TLS key initialization states -constexpr int kKeyNotInitialized = 0; -constexpr int kKeyInitializing = 1; -constexpr int kKeyInitialized = 2; -} // namespace - -// Static declarations -std::atomic AllocationTracker::_key_init_state{kKeyNotInitialized}; - -pthread_key_t AllocationTracker::_tl_state_key; - -AllocationTracker *AllocationTracker::_instance; +AllocationTracker *AllocationTracker::_instance = nullptr; +std::atomic AllocationTracker::_tl_state_key{ + AllocationTracker::kInvalidKey}; namespace { DDPROF_NOINLINE auto sleep_and_retry_reserve(MPSCRingBufferWriter &writer, @@ -65,36 +55,15 @@ TrackerThreadLocalState *AllocationTracker::get_tl_state() { // tls_get_addr can call into malloc, which can create a recursive loop // instead we call pthread APIs to control the creation of TLS objects - // Thread-safe initialization using atomics (avoids pthread_once ABI issues) - - // Fast path: relaxed load is sufficient since we only care if it's - // initialized Once initialized, this value never changes, so no - // synchronization needed - if (_key_init_state.load(std::memory_order_relaxed) != kKeyInitialized) { - // Slow path: need proper synchronization for initialization - int expected = kKeyNotInitialized; - if (_key_init_state.compare_exchange_strong(expected, kKeyInitializing, - std::memory_order_acq_rel)) { - // We won the race, do the initialization - make_key(); - _key_init_state.store(kKeyInitialized, std::memory_order_release); - } else { - // Another thread is initializing or already done, wait until complete - constexpr int k_max_init_wait_attempts = 100; - int attempts = 0; - while (_key_init_state.load(std::memory_order_acquire) != - kKeyInitialized) { - if (++attempts >= k_max_init_wait_attempts) { - return nullptr; - } - std::this_thread::yield(); - std::this_thread::sleep_for(std::chrono::microseconds(1)); - } - } - } + // The pthread key is initialized during allocation_tracking_init() and + // maintained by the fork handler, so we can directly use it here (hot path) + pthread_key_t key = _tl_state_key.load(std::memory_order_relaxed); + + // In debug builds, verify our assumption + assert(key != kInvalidKey && "pthread key should be initialized before use"); - auto *tl_state = static_cast( - pthread_getspecific(_tl_state_key)); + auto *tl_state = + static_cast(pthread_getspecific(key)); return tl_state; } @@ -127,15 +96,49 @@ void AllocationTracker::delete_tl_state(void *tl_state) { delete static_cast(tl_state); } -void AllocationTracker::make_key() { - // delete is called on all key objects - pthread_key_create(&_tl_state_key, delete_tl_state); +void AllocationTracker::ensure_key_initialized() { + // Ensure pthread key is initialized (idempotent) + pthread_key_t key = _tl_state_key.load(std::memory_order_acquire); + + if (key == kInvalidKey) { + pthread_key_t new_key; + if (pthread_key_create(&new_key, delete_tl_state) != 0) { + return; // Failed, will be retried later + } + + pthread_key_t expected = kInvalidKey; + if (!_tl_state_key.compare_exchange_strong(expected, new_key, + std::memory_order_release)) { + // Another thread beat us, clean up our key + pthread_key_delete(new_key); + } + } +} + +static void child_after_fork_handler() { + // After fork in child, verify the pthread key is still valid. + // The key itself survives fork, but we want to ensure it's properly set. + AllocationTracker::ensure_key_initialized(); +} + +void AllocationTracker::register_fork_handler() { + static bool registered = false; + if (!registered) { + pthread_atfork(nullptr, nullptr, child_after_fork_handler); + registered = true; + } } DDRes AllocationTracker::allocation_tracking_init( uint64_t allocation_profiling_rate, uint32_t flags, uint32_t stack_sample_size, const RingBufferInfo &ring_buffer, const IntervalTimerCheck &timer_check) { + // Register fork handler to ensure key is valid after fork + register_fork_handler(); + + // Ensure pthread key is initialized before we try to use it + ensure_key_initialized(); + TrackerThreadLocalState *tl_state = get_tl_state(); if (!tl_state) { // This is the time at which the init_tl_state should not fail diff --git a/test/allocation_tracker_fork_test.c b/test/allocation_tracker_fork_test.c index b5b2b344..dfaf5902 100644 --- a/test/allocation_tracker_fork_test.c +++ b/test/allocation_tracker_fork_test.c @@ -20,6 +20,7 @@ extern "C" { void *AllocationTracker_get_tl_state(void); void *AllocationTracker_init_tl_state(void); +void AllocationTracker_ensure_key_initialized(void); #ifdef __cplusplus } @@ -43,9 +44,10 @@ static void *thread_test_func(void *arg) { } int main(void) { - // Initialize pthread key and create TLS data in parent - void *state_before = AllocationTracker_get_tl_state(); + // Initialize pthread key before using it + AllocationTracker_ensure_key_initialized(); + // Initialize pthread key and create TLS data in parent void *parent_state = AllocationTracker_init_tl_state(); if (parent_state == NULL) { fprintf(stderr, "FAIL: Parent init_tl_state() returned NULL\n"); @@ -70,8 +72,6 @@ int main(void) { if (pid == 0) { // Child process - test pthread key works after fork - void *child_inherited = AllocationTracker_get_tl_state(); - void *child_state = AllocationTracker_init_tl_state(); if (child_state == NULL) { fprintf(stderr, "FAIL: Child init_tl_state() returned NULL\n"); @@ -94,6 +94,7 @@ int main(void) { pthread_join(thread, NULL); if (thread_result != 0) { + fprintf(stderr, "FAIL: Thread test failed - new thread TLS not NULL\n"); _exit(4); } @@ -121,11 +122,11 @@ int main(void) { if (exit_code == 1) reason = "init_tl_state failed"; else if (exit_code == 2) - reason = "pthread key broken"; + reason = "pthread key broken after fork"; else if (exit_code == 3) reason = "pthread_create failed"; else if (exit_code == 4) - reason = "thread test failed"; + reason = "new thread TLS not NULL"; fprintf(stderr, "FAIL: Child exited with code %d (%s)\n", exit_code, reason); return 1; diff --git a/test/allocation_tracker_fork_test_wrapper.cc b/test/allocation_tracker_fork_test_wrapper.cc index ece43d22..97d63019 100644 --- a/test/allocation_tracker_fork_test_wrapper.cc +++ b/test/allocation_tracker_fork_test_wrapper.cc @@ -13,6 +13,10 @@ void *AllocationTracker_get_tl_state(void) { return ddprof::AllocationTracker::get_tl_state(); } +void AllocationTracker_ensure_key_initialized(void) { + ddprof::AllocationTracker::ensure_key_initialized(); +} + void *AllocationTracker_init_tl_state(void) { return ddprof::AllocationTracker::init_tl_state(); } From d40c811083c61b79d25494fca97450fea5c25e09 Mon Sep 17 00:00:00 2001 From: Erwan Viollet Date: Thu, 29 Jan 2026 12:47:32 +0100 Subject: [PATCH 4/4] Minor linter fix --- src/lib/allocation_tracker.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lib/allocation_tracker.cc b/src/lib/allocation_tracker.cc index be611344..625496ac 100644 --- a/src/lib/allocation_tracker.cc +++ b/src/lib/allocation_tracker.cc @@ -48,6 +48,12 @@ DDPROF_NOINLINE auto sleep_and_retry_reserve(MPSCRingBufferWriter &writer, } return Buffer{}; } + +void child_after_fork_handler() { + // After fork in child, verify the pthread key is still valid. + // The key itself survives fork, but we want to ensure it's properly set. + AllocationTracker::ensure_key_initialized(); +} } // namespace TrackerThreadLocalState *AllocationTracker::get_tl_state() { @@ -57,7 +63,7 @@ TrackerThreadLocalState *AllocationTracker::get_tl_state() { // The pthread key is initialized during allocation_tracking_init() and // maintained by the fork handler, so we can directly use it here (hot path) - pthread_key_t key = _tl_state_key.load(std::memory_order_relaxed); + const pthread_key_t key = _tl_state_key.load(std::memory_order_relaxed); // In debug builds, verify our assumption assert(key != kInvalidKey && "pthread key should be initialized before use"); @@ -98,7 +104,7 @@ void AllocationTracker::delete_tl_state(void *tl_state) { void AllocationTracker::ensure_key_initialized() { // Ensure pthread key is initialized (idempotent) - pthread_key_t key = _tl_state_key.load(std::memory_order_acquire); + const pthread_key_t key = _tl_state_key.load(std::memory_order_acquire); if (key == kInvalidKey) { pthread_key_t new_key; @@ -115,12 +121,6 @@ void AllocationTracker::ensure_key_initialized() { } } -static void child_after_fork_handler() { - // After fork in child, verify the pthread key is still valid. - // The key itself survives fork, but we want to ensure it's properly set. - AllocationTracker::ensure_key_initialized(); -} - void AllocationTracker::register_fork_handler() { static bool registered = false; if (!registered) {