From 34338e5798aa6756d66c1f9b64ba8e94a4aefbee Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 12 Aug 2021 22:25:38 -0400 Subject: [PATCH] Avoid undefined behavior in pthread TLS (#26999) See code commentary for an explanation. Add an additional constructor to allow `log_linux.cc` to compile with GPR_PTHREAD_TLS. Without it: ``` ../../third_party/grpc/src/core/lib/gpr/log_linux.cc:78:33: error: no viable conversion from 'int' to 'grpc_core::PthreadTlsImpl' static GPR_THREAD_LOCAL(long) tid = 0; ^ ~ ../../third_party/grpc/src/core/lib/gpr/tls.h:64:3: note: candidate constructor not viable: no known conversion from 'int' to 'const grpc_core::PthreadTlsImpl &' for 1st argument PthreadTlsImpl(const PthreadTlsImpl&) = delete; ^ 1 error generated. ``` --- src/core/lib/gpr/tls.h | 76 +++++++++++------------- src/core/lib/iomgr/ev_epoll1_linux.cc | 4 -- src/core/lib/iomgr/ev_epollex_linux.cc | 4 -- src/core/lib/iomgr/ev_poll_posix.cc | 11 +--- src/core/lib/iomgr/exec_ctx.cc | 2 - src/core/lib/iomgr/exec_ctx.h | 6 +- src/core/lib/iomgr/executor.cc | 2 +- src/core/lib/iomgr/timer_generic.cc | 4 +- src/core/lib/surface/completion_queue.cc | 5 +- test/core/gpr/tls_test.cc | 4 -- test/cpp/end2end/nonblocking_test.cc | 3 +- 11 files changed, 46 insertions(+), 75 deletions(-) diff --git a/src/core/lib/gpr/tls.h b/src/core/lib/gpr/tls.h index b8eefea2458..09da92cde92 100644 --- a/src/core/lib/gpr/tls.h +++ b/src/core/lib/gpr/tls.h @@ -28,12 +28,6 @@ Usage is the same as C++ thread_local. Declaring a thread local: static GPR_THREAD_LOCAL(uint32_t) foo; - Initializing a thread local (must be done at library initialization time): - gpr_tls_init(foo); - - Destroying a thread local: - gpr_tls_destroy(foo); - ALL functions here may be implemented as macros. */ namespace grpc_core { @@ -67,13 +61,43 @@ namespace grpc_core { template class PthreadTlsImpl : TlsTypeConstrainer { public: - PthreadTlsImpl() = default; PthreadTlsImpl(const PthreadTlsImpl&) = delete; PthreadTlsImpl& operator=(const PthreadTlsImpl&) = delete; - void Init() { GPR_ASSERT(0 == pthread_key_create(&key_, nullptr)); } - - void Destroy() { GPR_ASSERT(0 == pthread_key_delete(key_)); } + // Achtung! This class emulates C++ `thread_local` using pthread keys. Each + // instance of this class is a stand in for a C++ `thread_local`. Think of + // each `thread_local` as a *global* pthread_key_t and a type tag. An + // important consequence of this is that the lifetime of a `pthread_key_t` + // is precisely the lifetime of an instance of this class. To understand why + // this is, consider the following scenario given a fictional implementation + // of this class which creates and destroys its `pthread_key_t` each time + // a given block of code runs (all actions take place on a single thread): + // + // - instance 1 (type tag = T*) is initialized, is assigned `pthread_key_t` 1 + // - instance 2 (type tag = int) is initialized, is assigned `pthread_key_t` 2 + // - instances 1 and 2 store and retrieve values; all is well + // - instances 1 and 2 are de-initialized; their keys are released to the pool + // + // - another run commences + // - instance 1 receives key 2 + // - a value is read from instance 1, it observes a value of type int, but + // interprets it as T*; undefined behavior, kaboom + // + // To properly ensure these invariants are upheld the `pthread_key_t` must be + // `const`, which means it can only be released in the destructor. This is a + // a violation of the style guide, since these objects are always static (see + // footnote) but this code is used in sufficiently narrow circumstances to + // justify the deviation. + // + // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables + PthreadTlsImpl() + : key_([]() { + pthread_key_t key; + GPR_ASSERT(0 == pthread_key_create(&key, nullptr)); + return key; + }()) {} + PthreadTlsImpl(T t) : PthreadTlsImpl() { *this = t; } + ~PthreadTlsImpl() { GPR_ASSERT(0 == pthread_key_delete(key_)); } operator T() const { return Cast(pthread_getspecific(key_)); } @@ -110,46 +134,18 @@ class PthreadTlsImpl : TlsTypeConstrainer { return reinterpret_cast(uintptr_t(t)); } - pthread_key_t key_; -}; - -// This class is never instantiated. It exists to statically ensure that all -// TLS usage is compatible with the most restrictive implementation, allowing -// developers to write correct code regardless of the platform they develop on. -template -class TriviallyDestructibleAsserter { - // This type is often used as a global; since the type itself is hidden by the - // macros, enforce compliance with the style guide here rather than at the - // caller. See - // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables. - static_assert(std::is_trivially_destructible::value, - "TLS wrapper must be trivially destructible"); - - public: - using Type = T; + const pthread_key_t key_; }; } // namespace grpc_core -#define GPR_THREAD_LOCAL(type) \ - grpc_core::TriviallyDestructibleAsserter< \ - grpc_core::PthreadTlsImpl>::Type - -#define gpr_tls_init(tls) (tls).Init() -#define gpr_tls_destroy(tls) (tls).Destroy() +#define GPR_THREAD_LOCAL(type) grpc_core::PthreadTlsImpl #else #define GPR_THREAD_LOCAL(type) \ thread_local grpc_core::TlsTypeConstrainer::Type -#define gpr_tls_init(tls) \ - do { \ - } while (0) -#define gpr_tls_destroy(tls) \ - do { \ - } while (0) - #endif #endif /* GRPC_CORE_LIB_GPR_TLS_H */ diff --git a/src/core/lib/iomgr/ev_epoll1_linux.cc b/src/core/lib/iomgr/ev_epoll1_linux.cc index f0e404503fd..467105fb74e 100644 --- a/src/core/lib/iomgr/ev_epoll1_linux.cc +++ b/src/core/lib/iomgr/ev_epoll1_linux.cc @@ -515,8 +515,6 @@ static size_t choose_neighborhood(void) { } static grpc_error_handle pollset_global_init(void) { - gpr_tls_init(g_current_thread_pollset); - gpr_tls_init(g_current_thread_worker); gpr_atm_no_barrier_store(&g_active_poller, 0); global_wakeup_fd.read_fd = -1; grpc_error_handle err = grpc_wakeup_fd_init(&global_wakeup_fd); @@ -538,8 +536,6 @@ static grpc_error_handle pollset_global_init(void) { } static void pollset_global_shutdown(void) { - gpr_tls_destroy(g_current_thread_pollset); - gpr_tls_destroy(g_current_thread_worker); if (global_wakeup_fd.read_fd != -1) grpc_wakeup_fd_destroy(&global_wakeup_fd); for (size_t i = 0; i < g_num_neighborhoods; i++) { gpr_mu_destroy(&g_neighborhoods[i].mu); diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 68bf9f87219..da5b1f3256d 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -648,15 +648,11 @@ static GPR_THREAD_LOCAL(grpc_pollset_worker*) g_current_thread_worker; /* Global state management */ static grpc_error_handle pollset_global_init(void) { - gpr_tls_init(g_current_thread_pollset); - gpr_tls_init(g_current_thread_worker); return pollable_create(PO_EMPTY, &g_empty_pollable); } static void pollset_global_shutdown(void) { POLLABLE_UNREF(g_empty_pollable, "g_empty_pollable"); - gpr_tls_destroy(g_current_thread_pollset); - gpr_tls_destroy(g_current_thread_worker); } /* pollset->mu must be held while calling this function */ diff --git a/src/core/lib/iomgr/ev_poll_posix.cc b/src/core/lib/iomgr/ev_poll_posix.cc index 2e8a191094a..c8cf2ec82a3 100644 --- a/src/core/lib/iomgr/ev_poll_posix.cc +++ b/src/core/lib/iomgr/ev_poll_posix.cc @@ -831,16 +831,9 @@ static grpc_error_handle pollset_kick(grpc_pollset* p, /* global state management */ -static grpc_error_handle pollset_global_init(void) { - gpr_tls_init(g_current_thread_poller); - gpr_tls_init(g_current_thread_worker); - return GRPC_ERROR_NONE; -} +static grpc_error_handle pollset_global_init(void) { return GRPC_ERROR_NONE; } -static void pollset_global_shutdown(void) { - gpr_tls_destroy(g_current_thread_poller); - gpr_tls_destroy(g_current_thread_worker); -} +static void pollset_global_shutdown(void) {} /* main interface */ diff --git a/src/core/lib/iomgr/exec_ctx.cc b/src/core/lib/iomgr/exec_ctx.cc index 53d89a8133b..b551106a117 100644 --- a/src/core/lib/iomgr/exec_ctx.cc +++ b/src/core/lib/iomgr/exec_ctx.cc @@ -138,7 +138,6 @@ ApplicationCallbackExecCtx::callback_exec_ctx_; // WARNING: for testing purposes only! void ExecCtx::TestOnlyGlobalInit(gpr_timespec new_val) { g_start_time = new_val; - gpr_tls_init(exec_ctx_); } void ExecCtx::GlobalInit(void) { @@ -149,7 +148,6 @@ void ExecCtx::GlobalInit(void) { g_start_time = gpr_now(GPR_CLOCK_MONOTONIC); const gpr_cycle_counter cycle_after = gpr_get_cycle_counter(); g_start_cycle = (cycle_before + cycle_after) / 2; - gpr_tls_init(exec_ctx_); } bool ExecCtx::Flush() { diff --git a/src/core/lib/iomgr/exec_ctx.h b/src/core/lib/iomgr/exec_ctx.h index c90a5724084..1465c5a4394 100644 --- a/src/core/lib/iomgr/exec_ctx.h +++ b/src/core/lib/iomgr/exec_ctx.h @@ -216,7 +216,7 @@ class ExecCtx { static void GlobalInit(void); /** Global shutdown for ExecCtx. Called by iomgr. */ - static void GlobalShutdown(void) { gpr_tls_destroy(exec_ctx_); } + static void GlobalShutdown(void) {} /** Gets pointer to current exec_ctx. */ static ExecCtx* Get() { return exec_ctx_; } @@ -357,10 +357,10 @@ class ApplicationCallbackExecCtx { } /** Global initialization for ApplicationCallbackExecCtx. Called by init. */ - static void GlobalInit(void) { gpr_tls_init(callback_exec_ctx_); } + static void GlobalInit(void) {} /** Global shutdown for ApplicationCallbackExecCtx. Called by init. */ - static void GlobalShutdown(void) { gpr_tls_destroy(callback_exec_ctx_); } + static void GlobalShutdown(void) {} static bool Available() { return Get() != nullptr; } diff --git a/src/core/lib/iomgr/executor.cc b/src/core/lib/iomgr/executor.cc index f2edbef2387..50edabfed4c 100644 --- a/src/core/lib/iomgr/executor.cc +++ b/src/core/lib/iomgr/executor.cc @@ -464,6 +464,6 @@ void Executor::SetThreadingDefault(bool enable) { executors[static_cast(ExecutorType::DEFAULT)]->SetThreading(enable); } -void grpc_executor_global_init() { gpr_tls_init(g_this_thread_state); } +void grpc_executor_global_init() {} } // namespace grpc_core diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index d8f9ce39a1e..c4d38c8fd4a 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -270,7 +270,7 @@ static void timer_list_init() { g_shared_mutables.min_timer = grpc_core::ExecCtx::Get()->Now(); #if GPR_ARCH_64 - gpr_tls_init(g_last_seen_min_timer); + g_last_seen_min_timer = 0; #endif @@ -303,7 +303,7 @@ static void timer_list_shutdown() { gpr_mu_destroy(&g_shared_mutables.mu); #if GPR_ARCH_64 - gpr_tls_destroy(g_last_seen_min_timer); + #endif gpr_free(g_shards); diff --git a/src/core/lib/surface/completion_queue.cc b/src/core/lib/surface/completion_queue.cc index 3c3b7b7cd74..37df534d617 100644 --- a/src/core/lib/surface/completion_queue.cc +++ b/src/core/lib/surface/completion_queue.cc @@ -439,10 +439,7 @@ grpc_core::TraceFlag grpc_cq_pluck_trace(false, "queue_pluck"); static void on_pollset_shutdown_done(void* arg, grpc_error_handle error); -void grpc_cq_global_init() { - gpr_tls_init(g_cached_event); - gpr_tls_init(g_cached_cq); -} +void grpc_cq_global_init() {} void grpc_completion_queue_thread_local_cache_init(grpc_completion_queue* cq) { if (g_cached_cq == nullptr) { diff --git a/test/core/gpr/tls_test.cc b/test/core/gpr/tls_test.cc index bcf6b6b0679..33f024f5792 100644 --- a/test/core/gpr/tls_test.cc +++ b/test/core/gpr/tls_test.cc @@ -52,8 +52,6 @@ int main(int argc, char* argv[]) { grpc::testing::TestEnvironment env(argc, argv); - gpr_tls_init(test_var); - for (auto& th : threads) { th = grpc_core::Thread("grpc_tls_test", thd_body, nullptr); th.Start(); @@ -62,7 +60,5 @@ int main(int argc, char* argv[]) { th.Join(); } - gpr_tls_destroy(test_var); - return 0; } diff --git a/test/cpp/end2end/nonblocking_test.cc b/test/cpp/end2end/nonblocking_test.cc index b64689726f6..7be3078b8e9 100644 --- a/test/cpp/end2end/nonblocking_test.cc +++ b/test/cpp/end2end/nonblocking_test.cc @@ -201,7 +201,6 @@ int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); ::testing::InitGoogleTest(&argc, argv); - gpr_tls_init(g_is_nonblocking_poll); // Start the nonblocking poll thread-local variable as false because the // thread that issues RPCs starts by picking a port (which has non-zero @@ -209,7 +208,7 @@ int main(int argc, char** argv) { g_is_nonblocking_poll = false; int ret = RUN_ALL_TESTS(); - gpr_tls_destroy(g_is_nonblocking_poll); + return ret; #else // GRPC_POSIX_SOCKET (void)argc;