From 5b8ee5b7c2dae3925b7a2b37f9ba94e2e8039012 Mon Sep 17 00:00:00 2001 From: Indragie Karunaratne Date: Wed, 24 May 2023 14:20:00 -0700 Subject: [PATCH 1/3] fix: Profiling memory leak in std::thread std::thread does not free the parameters that are passed to it correctly, fall back to pthread APIs. --- Sources/Sentry/SentrySamplingProfiler.cpp | 69 ++++++++++++------- .../Sentry/include/SentrySamplingProfiler.hpp | 4 +- 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/Sources/Sentry/SentrySamplingProfiler.cpp b/Sources/Sentry/SentrySamplingProfiler.cpp index 658de412001..c43da0cac0e 100644 --- a/Sources/Sentry/SentrySamplingProfiler.cpp +++ b/Sources/Sentry/SentrySamplingProfiler.cpp @@ -16,41 +16,55 @@ namespace sentry { namespace profiling { namespace { + struct SamplingThreadParams { + mach_port_t port; + clock_serv_t clock; + mach_timespec_t delaySpec; + std::shared_ptr cache; + std::function callback; + std::atomic_uint64_t &numSamples; + std::function onThreadStart; + }; + void - samplingThreadCleanup(void *buf) + freeReplyBuf(void *reply) { - free(buf); + free(reply); + } + + void deleteParams(void *params) { + delete reinterpret_cast(params); } void * - samplingThreadMain(mach_port_t port, clock_serv_t clock, mach_timespec_t delaySpec, - const std::shared_ptr &cache, - const std::function &callback, - std::atomic_uint64_t &numSamples, std::function onThreadStart) + samplingThreadMain(void *arg) { SENTRY_PROF_LOG_ERROR_RETURN(pthread_setname_np("io.sentry.SamplingProfiler")); - const int maxSize = 512; - const auto bufRequest = reinterpret_cast(malloc(maxSize)); - if (onThreadStart) { - onThreadStart(); + const auto params = reinterpret_cast(arg); + if (params->onThreadStart != nullptr) { + params->onThreadStart(); } - pthread_cleanup_push(samplingThreadCleanup, bufRequest); + const int maxSize = 512; + const auto replyBuf = reinterpret_cast(malloc(maxSize)); + pthread_cleanup_push(freeReplyBuf, replyBuf); + pthread_cleanup_push(deleteParams, params); while (true) { pthread_testcancel(); - if (SENTRY_PROF_LOG_MACH_MSG_RETURN(mach_msg(&bufRequest->Head, MACH_RCV_MSG, 0, - maxSize, port, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL)) + if (SENTRY_PROF_LOG_MACH_MSG_RETURN(mach_msg(&replyBuf->Head, MACH_RCV_MSG, 0, + maxSize, params->port, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL)) != MACH_MSG_SUCCESS) { break; } - if (SENTRY_PROF_LOG_KERN_RETURN(clock_alarm(clock, TIME_RELATIVE, delaySpec, port)) + if (SENTRY_PROF_LOG_KERN_RETURN(clock_alarm(params->clock, TIME_RELATIVE, params->delaySpec, params->port)) != KERN_SUCCESS) { break; } - numSamples.fetch_add(1, std::memory_order_relaxed); - enumerateBacktracesForAllThreads(callback, cache); + params->numSamples.fetch_add(1, std::memory_order_relaxed); + enumerateBacktracesForAllThreads(params->callback, params->cache); } pthread_cleanup_pop(1); + pthread_cleanup_pop(1); return nullptr; } @@ -107,20 +121,23 @@ namespace profiling { } isSampling_ = true; numSamples_ = 0; - thread_ = std::thread(samplingThreadMain, port_, clock_, delaySpec_, std::cref(cache_), - std::cref(callback_), std::ref(numSamples_), onThreadStart); - - int policy; + pthread_attr_t attr; + if (SENTRY_PROF_LOG_ERROR_RETURN(pthread_attr_init(&attr)) != 0) { + return; + } sched_param param; - const auto pthreadHandle = thread_.native_handle(); - if (SENTRY_PROF_LOG_ERROR_RETURN(pthread_getschedparam(pthreadHandle, &policy, ¶m)) - == 0) { + if (SENTRY_PROF_LOG_ERROR_RETURN(pthread_attr_getschedparam(&attr, ¶m)) == 0) { // A priority of 50 is higher than user input, according to: // https://chromium.googlesource.com/chromium/src/base/+/master/threading/platform_thread_mac.mm#302 // Run at a higher priority than the main thread so that we can capture main thread // backtraces even when it's busy. param.sched_priority = 50; - SENTRY_PROF_LOG_ERROR_RETURN(pthread_setschedparam(pthreadHandle, policy, ¶m)); + SENTRY_PROF_LOG_ERROR_RETURN(pthread_attr_setschedparam(&attr, ¶m)); + } + + const auto params = new SamplingThreadParams{port_, clock_, delaySpec_, cache_, callback_, std::ref(numSamples_), std::move(onThreadStart)}; + if (SENTRY_PROF_LOG_ERROR_RETURN(pthread_create(&thread_, &attr, samplingThreadMain, params)) != 0) { + return; } SENTRY_PROF_LOG_KERN_RETURN(clock_alarm(clock_, TIME_RELATIVE, delaySpec_, port_)); @@ -136,8 +153,8 @@ namespace profiling { if (!isSampling_) { return; } - SENTRY_PROF_LOG_KERN_RETURN(pthread_cancel(thread_.native_handle())); - thread_.join(); + SENTRY_PROF_LOG_ERROR_RETURN(pthread_cancel(thread_)); + SENTRY_PROF_LOG_ERROR_RETURN(pthread_join(thread_, NULL)); isSampling_ = false; } diff --git a/Sources/Sentry/include/SentrySamplingProfiler.hpp b/Sources/Sentry/include/SentrySamplingProfiler.hpp index 0499fffa6e1..4d56ded6b78 100644 --- a/Sources/Sentry/include/SentrySamplingProfiler.hpp +++ b/Sources/Sentry/include/SentrySamplingProfiler.hpp @@ -10,7 +10,7 @@ # include # include # include -# include +# include namespace sentry { namespace profiling { @@ -60,7 +60,7 @@ namespace profiling { bool isInitialized_; std::mutex isSamplingLock_; bool isSampling_; - std::thread thread_; + pthread_t thread_; clock_serv_t clock_; mach_port_t port_; std::atomic_uint64_t numSamples_; From 7cd51d81d4e1dbccbd539598d051821f3b81b6fd Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Wed, 24 May 2023 21:23:19 +0000 Subject: [PATCH 2/3] Format code --- Sources/Sentry/SentrySamplingProfiler.cpp | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Sources/Sentry/SentrySamplingProfiler.cpp b/Sources/Sentry/SentrySamplingProfiler.cpp index c43da0cac0e..a3a2441dcd3 100644 --- a/Sources/Sentry/SentrySamplingProfiler.cpp +++ b/Sources/Sentry/SentrySamplingProfiler.cpp @@ -25,14 +25,16 @@ namespace profiling { std::atomic_uint64_t &numSamples; std::function onThreadStart; }; - + void freeReplyBuf(void *reply) { free(reply); } - - void deleteParams(void *params) { + + void + deleteParams(void *params) + { delete reinterpret_cast(params); } @@ -51,11 +53,12 @@ namespace profiling { while (true) { pthread_testcancel(); if (SENTRY_PROF_LOG_MACH_MSG_RETURN(mach_msg(&replyBuf->Head, MACH_RCV_MSG, 0, - maxSize, params->port, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL)) + maxSize, params->port, MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL)) != MACH_MSG_SUCCESS) { break; } - if (SENTRY_PROF_LOG_KERN_RETURN(clock_alarm(params->clock, TIME_RELATIVE, params->delaySpec, params->port)) + if (SENTRY_PROF_LOG_KERN_RETURN( + clock_alarm(params->clock, TIME_RELATIVE, params->delaySpec, params->port)) != KERN_SUCCESS) { break; } @@ -134,9 +137,12 @@ namespace profiling { param.sched_priority = 50; SENTRY_PROF_LOG_ERROR_RETURN(pthread_attr_setschedparam(&attr, ¶m)); } - - const auto params = new SamplingThreadParams{port_, clock_, delaySpec_, cache_, callback_, std::ref(numSamples_), std::move(onThreadStart)}; - if (SENTRY_PROF_LOG_ERROR_RETURN(pthread_create(&thread_, &attr, samplingThreadMain, params)) != 0) { + + const auto params = new SamplingThreadParams { port_, clock_, delaySpec_, cache_, callback_, + std::ref(numSamples_), std::move(onThreadStart) }; + if (SENTRY_PROF_LOG_ERROR_RETURN( + pthread_create(&thread_, &attr, samplingThreadMain, params)) + != 0) { return; } From c4a410464a6564942ea72862ca0c2ee5afbc3879 Mon Sep 17 00:00:00 2001 From: Indragie Karunaratne Date: Wed, 24 May 2023 14:24:16 -0700 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d04b952da51..ed3ef2d0028 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixes - Convert one of the two remaining usages of `sprintf` to `snprintf` (#2866) -- Fix memory leaks in the profiler (#3055) +- Fix memory leaks in the profiler (#3055, #3061) ## 8.7.2