diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 1c859351ea2..dda46d4970b 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -111,3 +111,9 @@ if (LIB_INSTALL_DIR) RUNTIME DESTINATION ${LIB_INSTALL_DIR} ) endif() + +if(BUILD_TESTING) + enable_testing() + add_subdirectory(test) +endif() + diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/thread_span_links.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/thread_span_links.hpp index 061cb123c03..02dbf064406 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/thread_span_links.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/thread_span_links.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -36,8 +37,8 @@ class ThreadSpanLinks ThreadSpanLinks& operator=(ThreadSpanLinks const&) = delete; void link_span(uint64_t thread_id, uint64_t span_id, uint64_t local_root_span_id, std::string span_type); - - const Span* get_active_span_from_thread_id(uint64_t thread_id); + const std::optional get_active_span_from_thread_id(uint64_t thread_id); + void reset(); static void postfork_child(); diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp index dc1ac1239b9..6243033e75b 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -55,8 +55,8 @@ StackRenderer::render_thread_begin(PyThreadState* tstate, ddup_push_threadinfo(sample, static_cast(thread_id), static_cast(native_id), name); ddup_push_walltime(sample, thread_state.wall_time_ns, 1); - const Span* active_span = ThreadSpanLinks::get_instance().get_active_span_from_thread_id(thread_id); - if (active_span != nullptr) { + const std::optional active_span = ThreadSpanLinks::get_instance().get_active_span_from_thread_id(thread_id); + if (active_span) { ddup_push_span_id(sample, active_span->span_id); ddup_push_local_root_span_id(sample, active_span->local_root_span_id); ddup_push_trace_type(sample, std::string_view(active_span->span_type)); diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/thread_span_links.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/thread_span_links.cpp index 602aad9fdb3..80c5ef06bc1 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/thread_span_links.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/thread_span_links.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -19,15 +20,17 @@ ThreadSpanLinks::link_span(uint64_t thread_id, uint64_t span_id, uint64_t local_ thread_id_to_span[thread_id]->span_type = span_type; } -const Span* +const std::optional ThreadSpanLinks::get_active_span_from_thread_id(uint64_t thread_id) { std::lock_guard lock(mtx); - if (thread_id_to_span.find(thread_id) == thread_id_to_span.end()) { - return nullptr; + std::optional span; + auto it = thread_id_to_span.find(thread_id); + if (it != thread_id_to_span.end()) { + span = *(it->second); } - return thread_id_to_span[thread_id].get(); + return span; } void diff --git a/ddtrace/internal/datadog/profiling/stack_v2/test/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/test/CMakeLists.txt new file mode 100644 index 00000000000..23fcda3eedb --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/test/CMakeLists.txt @@ -0,0 +1,25 @@ +FetchContent_Declare( + googletest + GIT_REPOSITORY https://github.com/google/googletest.git + GIT_TAG release-1.11.0) +set(gtest_force_shared_crt + ON + CACHE BOOL "" FORCE) +set(INSTALL_GTEST + OFF + CACHE BOOL "" FORCE) +FetchContent_MakeAvailable(googletest) +include(GoogleTest) +include(AnalysisFunc) + +function(dd_wrapper_add_test name) + add_executable(${name} ${ARGN}) + target_include_directories(${name} PRIVATE ../include) + target_link_libraries(${name} PRIVATE gmock gtest_main _stack_v2) + add_ddup_config(${name}) + + gtest_discover_tests(${name}) +endfunction() + +# Add the tests +dd_wrapper_add_test(thread_span_links thread_span_links.cpp) diff --git a/ddtrace/internal/datadog/profiling/stack_v2/test/thread_span_links.cpp b/ddtrace/internal/datadog/profiling/stack_v2/test/thread_span_links.cpp new file mode 100644 index 00000000000..b668c3b3d80 --- /dev/null +++ b/ddtrace/internal/datadog/profiling/stack_v2/test/thread_span_links.cpp @@ -0,0 +1,48 @@ +#include + +#include +#include + +#include "thread_span_links.hpp" + +static void +get() +{ + for (int i = 0; i < 100; i++) { + std::string span_type; + for (int j = 0; j < i; j++) { + span_type.append("a"); + } + Datadog::ThreadSpanLinks::get_instance().link_span(42, 1, 2, span_type); + } +} + +static std::string +set() +{ + std::string s; + for (int i = 0; i < 100; i++) { + auto thing = Datadog::ThreadSpanLinks::get_instance().get_active_span_from_thread_id(42); + if (!thing) { + continue; + } + s = thing->span_type; + } + return s; +} + +TEST(ThreadSpanLinksConcurrency, GetSetRace) +{ + std::thread t1(get); + std::thread t2(set); + t1.join(); + t2.join(); +} + +int +main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + (void)(::testing::GTEST_FLAG(death_test_style) = "threadsafe"); + return RUN_ALL_TESTS(); +} diff --git a/releasenotes/notes/profiling-fix-data-race-segfault-b14f59c06c6bf9ed.yaml b/releasenotes/notes/profiling-fix-data-race-segfault-b14f59c06c6bf9ed.yaml new file mode 100644 index 00000000000..0dcae82aea8 --- /dev/null +++ b/releasenotes/notes/profiling-fix-data-race-segfault-b14f59c06c6bf9ed.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + profiling: fix a data race where span information associated with a thread + was read and updated concurrently, leading to segfaults