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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
