Bugfix: Signal Trace mode may send SIGURG to wrong thread (#3038)

* Bugfix: Use `pthread_sigqueue` to send the signal to correct worker thread'

* Change the type of `TaskGroup::_tid` to `pthread_t`

Author: CodeZHXS

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
@@ -90,7 +91,7 @@ void* TaskControl::worker_thread(void* arg) {
         return NULL;
     }
 
-    g->_tid = syscall(SYS_gettid);
+    g->_tid = pthread_self();
 
     std::string worker_thread_name = butil::string_printf(
         "brpc_wkr:%d-%d", g->tag(),

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) {

src/bthread/task_group.h

@@ -219,7 +219,7 @@ class TaskGroup {
 
     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

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

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);

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 @@ class TaskTracer {
     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 @@ class TaskTracer {
 
     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;