This is an automated email from the ASF dual-hosted git repository. guangmingchen pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push: new b6ae7bf9 Bugfix: Signal Trace mode may send SIGURG to wrong thread (#3039) b6ae7bf9 is described below commit b6ae7bf97314c1234883060062abd80a3af44be4 Author: chenyujie <78433231+codez...@users.noreply.github.com> AuthorDate: Tue Aug 26 14:16:52 2025 +0800 Bugfix: Signal Trace mode may send SIGURG to wrong thread (#3039) * Bugfix: Use `pthread_sigqueue` to send the signal to correct worker thread' * Change the type of `TaskGroup::_tid` to `pthread_t` --- src/bthread/task_control.cpp | 3 ++- src/bthread/task_group.cpp | 2 +- src/bthread/task_group.h | 4 ++-- src/bthread/task_meta.h | 2 +- src/bthread/task_tracer.cpp | 35 ++++++++++++++++++++--------------- src/bthread/task_tracer.h | 20 ++++++++++---------- 6 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/bthread/task_control.cpp b/src/bthread/task_control.cpp index 05ceec09..1bdcf0c0 100644 --- a/src/bthread/task_control.cpp +++ b/src/bthread/task_control.cpp @@ -19,6 +19,7 @@ // Date: Tue Jul 10 17:40:58 CST 2012 +#include <pthread.h> #include <sys/syscall.h> // SYS_gettid #include "butil/scoped_lock.h" // BAIDU_SCOPED_LOCK #include "butil/errno.h" // berror @@ -91,7 +92,7 @@ void* TaskControl::worker_thread(void* arg) { return NULL; } - g->_tid = syscall(SYS_gettid); + g->_tid = pthread_self(); if (FLAGS_task_group_set_worker_name) { std::string worker_thread_name = butil::string_printf( diff --git a/src/bthread/task_group.cpp b/src/bthread/task_group.cpp index d4cb81f6..773a442b 100644 --- a/src/bthread/task_group.cpp +++ b/src/bthread/task_group.cpp @@ -1107,7 +1107,7 @@ void print_task(std::ostream& os, bthread_t tid) { TaskStatistics stat = {0, 0, 0}; TaskStatus status = TASK_STATUS_UNKNOWN; bool traced = false; - pid_t worker_tid = 0; + pthread_t worker_tid{}; { BAIDU_SCOPED_LOCK(m->version_lock); if (given_ver == *m->version_butex) { diff --git a/src/bthread/task_group.h b/src/bthread/task_group.h index b79e69d8..43c91152 100644 --- a/src/bthread/task_group.h +++ b/src/bthread/task_group.h @@ -219,7 +219,7 @@ public: bthread_tag_t tag() const { return _tag; } - pid_t tid() const { return _tid; } + pthread_t tid() const { return _tid; } int64_t current_task_cpu_clock_ns() { if (_last_cpu_clock_ns == 0) { @@ -378,7 +378,7 @@ friend class TaskControl; bthread_tag_t _tag{BTHREAD_TAG_DEFAULT}; // Worker thread id. - pid_t _tid{-1}; + pthread_t _tid{}; }; } // namespace bthread diff --git a/src/bthread/task_meta.h b/src/bthread/task_meta.h index 34d86632..a4ed42bf 100644 --- a/src/bthread/task_meta.h +++ b/src/bthread/task_meta.h @@ -113,7 +113,7 @@ struct TaskMeta { // Whether bthread is traced? bool traced{false}; // Worker thread id. - pid_t worker_tid{-1}; + pthread_t worker_tid{}; public: // Only initialize [Not Reset] fields, other fields will be reset in diff --git a/src/bthread/task_tracer.cpp b/src/bthread/task_tracer.cpp index 32d103cd..e2483495 100644 --- a/src/bthread/task_tracer.cpp +++ b/src/bthread/task_tracer.cpp @@ -18,17 +18,18 @@ #ifdef BRPC_BTHREAD_TRACER #include "bthread/task_tracer.h" -#include <unistd.h> -#include <poll.h> -#include <gflags/gflags.h> -#include <absl/debugging/stacktrace.h> -#include <absl/debugging/symbolize.h> -#include "butil/debug/stack_trace.h" +#include "bthread/processor.h" +#include "bthread/task_group.h" +#include "butil/fd_utility.h" #include "butil/memory/scope_guard.h" #include "butil/reloadable_flags.h" -#include "butil/fd_utility.h" -#include "bthread/task_group.h" -#include "bthread/processor.h" +#include <absl/debugging/stacktrace.h> +#include <absl/debugging/symbolize.h> +#include <csignal> +#include <gflags/gflags.h> +#include <poll.h> +#include <pthread.h> +#include <unistd.h> namespace bthread { @@ -151,7 +152,7 @@ void TaskTracer::set_status(TaskStatus s, TaskMeta* m) { m->status = s; } if (TASK_STATUS_CREATED == s) { - m->worker_tid = -1; + m->worker_tid = pthread_t{}; } } @@ -161,7 +162,7 @@ void TaskTracer::set_status(TaskStatus s, TaskMeta* m) { } } -void TaskTracer::set_running_status(pid_t worker_tid, TaskMeta* m) { +void TaskTracer::set_running_status(pthread_t worker_tid, TaskMeta* m) { BAIDU_SCOPED_LOCK(m->version_lock); m->worker_tid = worker_tid; m->status = TASK_STATUS_RUNNING; @@ -230,7 +231,7 @@ TaskTracer::Result TaskTracer::TraceImpl(bthread_t tid) { BAIDU_SCOPED_LOCK(_mutex); TaskStatus status; - pid_t worker_tid; + pthread_t worker_tid; const uint32_t given_version = get_version(tid); { BAIDU_SCOPED_LOCK(m->version_lock); @@ -368,7 +369,7 @@ void TaskTracer::SignalHandler(int, siginfo_t* info, void* context) { butil::ignore_result(write(signal_sync->pipe_fds[1], "1", 1)); } -TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) { +TaskTracer::Result TaskTracer::SignalTrace(pthread_t worker_tid) { // CAUTION: // https://github.com/gperftools/gperftools/wiki/gperftools'-stacktrace-capturing-methods-and-their-issues#libunwind // Generally, libunwind promises async-signal-safety for capturing backtraces. @@ -403,6 +404,10 @@ TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) { // // Therefore, use async-signal-safe absl::DefaultStackUnwinder instead of libunwind. + if (pthread_equal(worker_tid, pthread_self())) { + return Result::MakeErrorResult("Forbid to trace self"); + } + // Remove unused SignalSyncs. auto iter = std::remove_if( _inuse_signal_syncs.begin(), _inuse_signal_syncs.end(), @@ -424,11 +429,11 @@ TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) { sigval value{}; value.sival_ptr = signal_sync.get(); size_t sigqueue_try = 0; - while (sigqueue(tid, SIGURG, value) != 0) { + while (pthread_sigqueue(worker_tid, SIGURG, value) != 0) { if (errno != EAGAIN || sigqueue_try++ >= 3) { // Remove reference for SignalHandler. signal_sync->RemoveRefManually(); - return Result::MakeErrorResult("Fail to sigqueue: %s, tid: %d", berror(), tid); + return Result::MakeErrorResult("Fail to pthread_sigqueue: %s", berror()); } } _inuse_signal_syncs.push_back(signal_sync); diff --git a/src/bthread/task_tracer.h b/src/bthread/task_tracer.h index 81940cfd..0888c658 100644 --- a/src/bthread/task_tracer.h +++ b/src/bthread/task_tracer.h @@ -20,15 +20,15 @@ #ifdef BRPC_BTHREAD_TRACER -#include <signal.h> -#include <vector> -#include <libunwind.h> -#include "butil/synchronization/condition_variable.h" +#include "bthread/mutex.h" +#include "bthread/task_meta.h" #include "butil/intrusive_ptr.hpp" -#include "butil/strings/safe_sprintf.h" #include "butil/shared_object.h" -#include "bthread/task_meta.h" -#include "bthread/mutex.h" +#include "butil/strings/safe_sprintf.h" +#include "butil/synchronization/condition_variable.h" +#include <libunwind.h> +#include <signal.h> +#include <vector> namespace bthread { @@ -39,10 +39,10 @@ public: bool Init(); // Set the status to `s'. void set_status(TaskStatus s, TaskMeta* meta); - static void set_running_status(pid_t worker_tid, TaskMeta* meta); + static void set_running_status(pthread_t worker_tid, TaskMeta* meta); static bool set_end_status_unsafe(TaskMeta* m); - // Trace the bthread of `tid'. + // Trace the bthread of `tid`. std::string Trace(bthread_t tid); void Trace(std::ostream& os, bthread_t tid); @@ -103,7 +103,7 @@ private: static bool RegisterSignalHandler(); static void SignalHandler(int sig, siginfo_t* info, void* context); - Result SignalTrace(pid_t worker_tid); + Result SignalTrace(pthread_t worker_tid); // Make sure only one bthread is traced at a time. Mutex _trace_request_mutex; --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org