From d35ba1b3f16071b8fe9b36398ba15352dbf2a54d Mon Sep 17 00:00:00 2001
From: Vasil Dimov <vd@FreeBSD.org>
Date: Mon, 13 May 2024 16:57:03 +0200
Subject: util: avoid using thread_local variable that has a destructor

Store the thread name in a `thread_local` variable of type `char[]`
instead of `std::string`. This avoids calling the destructor when
the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

For type-safety, return `std::string` from
`util::ThreadGetInternalName()` instead of `char[]`.

As a side effect of this change, we no longer store a reference
to a `thread_local` variable in `CLockLocation`. This was
dangerous because if the thread quits while the reference still
exists (in the global variable `lock_data`, see inside `GetLockData()`)
then the reference will become dangling.
---
 configure.ac             |  5 -----
 src/sync.cpp             |  6 +++---
 src/util/threadnames.cpp | 31 +++++++++++++++++++++----------
 src/util/threadnames.h   |  6 +++---
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/configure.ac b/configure.ac
index dfb7b99756..bfc1e54921 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1049,11 +1049,6 @@ if test "$use_thread_local" = "yes" || test "$use_thread_local" = "auto"; then
           dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
           AC_MSG_RESULT([no])
           ;;
-        *freebsd*)
-          dnl FreeBSD's implementation of thread_local is also buggy (per
-          dnl https://groups.google.com/d/msg/bsdmailinglist/22ncTZAbDp4/Dii_pII5AwAJ)
-          AC_MSG_RESULT([no])
-          ;;
         *)
           AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define if thread_local is supported.])
           AC_MSG_RESULT([yes])
diff --git a/src/sync.cpp b/src/sync.cpp
index a8bdfc1dea..93c9194541 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -37,11 +37,11 @@ struct CLockLocation {
         const char* pszFile,
         int nLine,
         bool fTryIn,
-        const std::string& thread_name)
+        std::string&& thread_name)
         : fTry(fTryIn),
           mutexName(pszName),
           sourceFile(pszFile),
-          m_thread_name(thread_name),
+          m_thread_name(std::move(thread_name)),
           sourceLine(nLine) {}
 
     std::string ToString() const
@@ -60,7 +60,7 @@ private:
     bool fTry;
     std::string mutexName;
     std::string sourceFile;
-    const std::string& m_thread_name;
+    const std::string m_thread_name;
     int sourceLine;
 };
 
diff --git a/src/util/threadnames.cpp b/src/util/threadnames.cpp
index ea597dd05a..ba074e30e0 100644
--- a/src/util/threadnames.cpp
+++ b/src/util/threadnames.cpp
@@ -4,6 +4,7 @@
 
 #include <config/bitcoin-config.h> // IWYU pragma: keep
 
+#include <cstring>
 #include <string>
 #include <thread>
 #include <utility>
@@ -40,27 +41,37 @@ static void SetThreadName(const char* name)
 // global.
 #if defined(HAVE_THREAD_LOCAL)
 
-static thread_local std::string g_thread_name;
-const std::string& util::ThreadGetInternalName() { return g_thread_name; }
+/**
+ * The name of the thread. We use char array instead of std::string to avoid
+ * complications with running a destructor when the thread exits. Avoid adding
+ * other thread_local variables.
+ * @see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701
+ */
+static thread_local char g_thread_name[128]{'\0'};
+std::string util::ThreadGetInternalName() { return g_thread_name; }
 //! Set the in-memory internal name for this thread. Does not affect the process
 //! name.
-static void SetInternalName(std::string name) { g_thread_name = std::move(name); }
+static void SetInternalName(const std::string& name)
+{
+    const size_t copy_bytes{std::min(sizeof(g_thread_name) - 1, name.length())};
+    std::memcpy(g_thread_name, name.data(), copy_bytes);
+    g_thread_name[copy_bytes] = '\0';
+}
 
 // Without thread_local available, don't handle internal name at all.
 #else
 
-static const std::string empty_string;
-const std::string& util::ThreadGetInternalName() { return empty_string; }
-static void SetInternalName(std::string name) { }
+std::string util::ThreadGetInternalName() { return ""; }
+static void SetInternalName(const std::string& name) { }
 #endif
 
-void util::ThreadRename(std::string&& name)
+void util::ThreadRename(const std::string& name)
 {
     SetThreadName(("b-" + name).c_str());
-    SetInternalName(std::move(name));
+    SetInternalName(name);
 }
 
-void util::ThreadSetInternalName(std::string&& name)
+void util::ThreadSetInternalName(const std::string& name)
 {
-    SetInternalName(std::move(name));
+    SetInternalName(name);
 }
diff --git a/src/util/threadnames.h b/src/util/threadnames.h
index 64b2689cf1..adca8c3000 100644
--- a/src/util/threadnames.h
+++ b/src/util/threadnames.h
@@ -12,14 +12,14 @@ namespace util {
 //! as its system thread name.
 //! @note Do not call this for the main thread, as this will interfere with
 //! UNIX utilities such as top and killall. Use ThreadSetInternalName instead.
-void ThreadRename(std::string&&);
+void ThreadRename(const std::string&);
 
 //! Set the internal (in-memory) name of the current thread only.
-void ThreadSetInternalName(std::string&&);
+void ThreadSetInternalName(const std::string&);
 
 //! Get the thread's internal (in-memory) name; used e.g. for identification in
 //! logging.
-const std::string& ThreadGetInternalName();
+std::string ThreadGetInternalName();
 
 } // namespace util
 
-- 
cgit v1.2.3