From d353964b3a607c480f8e47e317703096fd426130 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Sat, 3 Oct 2026 01:03:51 +0800 Subject: [PATCH 1/4] Destroy keytable pool and tls key when Server::Start() fails early StartInternal() allocates _keytable_pool (and the tls key when a thread_local_data_factory is set) unconditionally, but on failures before _status reaches RUNNING - e.g. tcp_listen() fails because the port is in use - RevertServerStatus runs Stop(0)+Join(), both of which return immediately when the server is not RUNNING. The pool and the key leaked on every failed attempt, and a retry overwrote _keytable_pool with a fresh pool, permanently leaking the previous one. ~Server() had the same problem for a failed-start server. Add Server::DestroyPreStartAllocations() (no-op once RUNNING/STOPPING, where Join() handles cleanup) and call it from RevertServerStatus and ~Server(). Unlike Join() the pool struct is deleted too, since no connection can exist before RUNNING. Verified in ServerTest.sanity: after a port-conflict failure, _keytable_pool is nullptr and the tls key is invalid (on the old code both leak and the test fails), including across a failed retry. Full brpc_server_unittest suite shows no new failures. --- src/brpc/server.cpp | 26 ++++++++++++++ src/brpc/server.h | 6 ++++ test/brpc_server_unittest.cpp | 68 +++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/src/brpc/server.cpp b/src/brpc/server.cpp index 5a29ae236b..ae0df893c1 100644 --- a/src/brpc/server.cpp +++ b/src/brpc/server.cpp @@ -451,6 +451,8 @@ Server::Server(ProfilerLinker) Server::~Server() { Stop(0); Join(); + // Stop()/Join() are no-ops when a Start() failed before RUNNING. + DestroyPreStartAllocations(); ClearServices(); FreeSSLContexts(); delete _session_local_data_pool; @@ -744,6 +746,8 @@ struct RevertServerStatus { if (s != nullptr) { s->Stop(0); s->Join(); + // Stop()/Join() are no-ops when the start failed before RUNNING. + s->DestroyPreStartAllocations(); } } }; @@ -1370,6 +1374,28 @@ int Server::Join() { return 0; } +void Server::DestroyPreStartAllocations() { + if (_status == RUNNING || _status == STOPPING) { + // Handled by Join(). + return; + } + if (_keytable_pool != nullptr) { + // Keep the TLS key valid until its pooled data has been destroyed. + // Deleting the key first would disable DestroyServerTLS(). + CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool)); + // Unlike Join(), no connection (thus no keytable user) can exist + // when the start failed before RUNNING, so the pool struct can be + // deleted as well. + delete _keytable_pool; + _keytable_pool = nullptr; + } + // Delete tls_key created in StartInternal(). + if (_tl_options.tls_key != INVALID_BTHREAD_KEY) { + CHECK_EQ(0, bthread_key_delete(_tl_options.tls_key)); + _tl_options.tls_key = INVALID_BTHREAD_KEY; + } +} + int Server::AddServiceInternal(google::protobuf::Service* service, bool is_builtin_service, const ServiceOptions& svc_opt) { diff --git a/src/brpc/server.h b/src/brpc/server.h index 3c68641be5..d93d8a7a82 100644 --- a/src/brpc/server.h +++ b/src/brpc/server.h @@ -652,6 +652,12 @@ friend class BadMethodService; friend class ServerPrivateAccessor; friend class PrometheusMetricsService; friend class Controller; +friend struct RevertServerStatus; + + // Destroy the keytable pool and tls key allocated in StartInternal() + // when the start failed before RUNNING: Stop()/Join() are no-ops then, + // and a retry would overwrite _keytable_pool and leak the previous one. + void DestroyPreStartAllocations(); int AddServiceInternal(google::protobuf::Service* service, bool is_builtin_service, diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp index cc09761cb7..598a6b6807 100644 --- a/test/brpc_server_unittest.cpp +++ b/test/brpc_server_unittest.cpp @@ -220,6 +220,24 @@ class EvilService : public test::EchoService { const google::protobuf::ServiceDescriptor* _sd; }; +class CountingTLSDataFactory : public brpc::DataFactory { +public: + CountingTLSDataFactory() : created(0), destroyed(0) {} + + void* CreateData() const override { + created.fetch_add(1, butil::memory_order_relaxed); + return new int(0); + } + + void DestroyData(void* data) const override { + delete static_cast(data); + destroyed.fetch_add(1, butil::memory_order_relaxed); + } + + mutable butil::atomic created; + mutable butil::atomic destroyed; +}; + class ServerTest : public ::testing::Test{ protected: ServerTest() {}; @@ -307,6 +325,56 @@ TEST_F(ServerTest, sanity) { ASSERT_EQ(0, server.Join()); } +TEST_F(ServerTest, failed_start_cleans_reserved_thread_local_data) { + CountingTLSDataFactory factory; + brpc::ServerOptions options; + options.thread_local_data_factory = &factory; + options.reserved_thread_local_data = 4; + brpc::Server listener; + ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr)); + brpc::Server server; + + for (size_t attempt = 1; attempt <= 2; ++attempt) { + ASSERT_EQ(-1, server.Start(listener.listen_address(), &options)); + EXPECT_TRUE(server._keytable_pool == nullptr); + EXPECT_EQ(INVALID_BTHREAD_KEY, server._tl_options.tls_key); + EXPECT_EQ(attempt * options.reserved_thread_local_data, + factory.created.load(butil::memory_order_relaxed)); + EXPECT_EQ(factory.created.load(butil::memory_order_relaxed), + factory.destroyed.load(butil::memory_order_relaxed)); + } + + // A successful retry must create a fresh key and pool. + ASSERT_EQ(0, server.Start("127.0.0.1:0", &options)); + EXPECT_TRUE(server._keytable_pool != nullptr); + EXPECT_NE(INVALID_BTHREAD_KEY, server._tl_options.tls_key); + EXPECT_EQ(3 * options.reserved_thread_local_data, + factory.created.load(butil::memory_order_relaxed)); + ASSERT_EQ(0, server.Stop(0)); + ASSERT_EQ(0, server.Join()); + EXPECT_EQ(INVALID_BTHREAD_KEY, server._tl_options.tls_key); + EXPECT_EQ(factory.created.load(butil::memory_order_relaxed), + factory.destroyed.load(butil::memory_order_relaxed)); +} + +TEST_F(ServerTest, failed_start_destruction_cleans_thread_local_data) { + CountingTLSDataFactory factory; + brpc::ServerOptions options; + options.thread_local_data_factory = &factory; + options.reserved_thread_local_data = 4; + brpc::Server listener; + ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr)); + { + brpc::Server server; + ASSERT_EQ(-1, server.Start(listener.listen_address(), &options)); + // No successful Start(), Stop() or Join() before destruction. + } + EXPECT_EQ(options.reserved_thread_local_data, + factory.created.load(butil::memory_order_relaxed)); + EXPECT_EQ(factory.created.load(butil::memory_order_relaxed), + factory.destroyed.load(butil::memory_order_relaxed)); +} + TEST_F(ServerTest, invalid_protocol_in_enabled_protocols) { butil::EndPoint ep; ASSERT_EQ(0, str2endpoint("127.0.0.1:8613", &ep)); From f4e1efd997b01016e7b3b07ccec98702b823d4fe Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Sat, 10 Oct 2026 00:05:19 +0800 Subject: [PATCH 2/4] Bthreads spawned from bthread_init_fn with the inherited attr (bthread_getattr(bthread_self(), ...)) carry keytable_pool == _keytable_pool and may outlive a failed Server::Start(). They still call return_keytable() on exit, so deleting the pool struct in DestroyPreStartAllocations() causes a heap-use-after-free (reported by ASan at key.cpp:394). Do the same as Join(): destroy the pool contents with bthread_keytable_pool_destroy() but don't delete the struct. A destroyed pool makes late return_keytable() calls delete their KeyTable directly. The leftover struct is small, only leaked on failed starts, and already annotated in StartInternal(). Add failed_start_keeps_keytable_pool_for_init_bthreads, which spawns such a child from bthread_init_fn and fails Start() with EADDRINUSE. --- src/brpc/server.cpp | 10 +++++---- test/brpc_server_unittest.cpp | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/brpc/server.cpp b/src/brpc/server.cpp index ae0df893c1..39363e1cd7 100644 --- a/src/brpc/server.cpp +++ b/src/brpc/server.cpp @@ -1383,10 +1383,12 @@ void Server::DestroyPreStartAllocations() { // Keep the TLS key valid until its pooled data has been destroyed. // Deleting the key first would disable DestroyServerTLS(). CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool)); - // Unlike Join(), no connection (thus no keytable user) can exist - // when the start failed before RUNNING, so the pool struct can be - // deleted as well. - delete _keytable_pool; + // Same as Join(): don't delete the pool struct. Bthreads created + // with attr.keytable_pool == _keytable_pool may outlive + // the failed start and still call return_keytable() on exit. A + // destroyed pool makes them delete their KeyTables directly, but the + // struct itself must stay valid. The leak is annotated in + // StartInternal() and only happens on failed starts. _keytable_pool = nullptr; } // Delete tls_key created in StartInternal(). diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp index 598a6b6807..920e101eaa 100644 --- a/test/brpc_server_unittest.cpp +++ b/test/brpc_server_unittest.cpp @@ -375,6 +375,44 @@ TEST_F(ServerTest, failed_start_destruction_cleans_thread_local_data) { factory.destroyed.load(butil::memory_order_relaxed)); } +static bthread_key_t g_init_child_key = INVALID_BTHREAD_KEY; +static bthread_t g_init_child = INVALID_BTHREAD; + +static void* InitChildOutlivingFailedStart(void*) { + // Outlive the failed Start(), then borrow and return a KeyTable of the + // server's keytable pool. + bthread_usleep(100 * 1000); + EXPECT_EQ(0, bthread_setspecific(g_init_child_key, (void*)1)); + return nullptr; +} + +static bool InitFnSpawningChild(void*) { + bthread_attr_t attr; + // attr.keytable_pool is the server's _keytable_pool. + EXPECT_EQ(0, bthread_getattr(bthread_self(), &attr)); + return bthread_start_background(&g_init_child, &attr, + InitChildOutlivingFailedStart, + nullptr) == 0; +} + +TEST_F(ServerTest, failed_start_keeps_keytable_pool_for_init_bthreads) { + ASSERT_EQ(0, bthread_key_create(&g_init_child_key, nullptr)); + brpc::Server listener; + ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr)); + brpc::ServerOptions options; + options.bthread_init_fn = InitFnSpawningChild; + options.bthread_init_count = 1; + { + brpc::Server server; + ASSERT_EQ(-1, server.Start(listener.listen_address(), &options)); + } + // The child calls return_keytable() on the destroyed (but not deleted) + // pool. Deleting the pool on the failure path is a use-after-free here + // (reported by ASan). + ASSERT_EQ(0, bthread_join(g_init_child, nullptr)); + ASSERT_EQ(0, bthread_key_delete(g_init_child_key)); +} + TEST_F(ServerTest, invalid_protocol_in_enabled_protocols) { butil::EndPoint ep; ASSERT_EQ(0, str2endpoint("127.0.0.1:8613", &ep)); From 18358463990e9b1f671d7fc06d8fb892ebd9c8d0 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Wed, 14 Oct 2026 13:38:00 +0800 Subject: [PATCH 3/4] test(brpc): Synchronize failed-start regression test Replace the fixed sleep with a CountdownEvent so the child bthread continues only after the failed Start() returns and the Server is destroyed. This ensures that the regression test exercises access to the destroyed keytable pool. Use a nonfatal Start() assertion so the child is still signaled and joined if startup unexpectedly succeeds. --- test/brpc_server_unittest.cpp | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp index 920e101eaa..8073208692 100644 --- a/test/brpc_server_unittest.cpp +++ b/test/brpc_server_unittest.cpp @@ -31,6 +31,7 @@ #include "butil/macros.h" #include "butil/fd_guard.h" #include "butil/files/scoped_file.h" +#include "bthread/countdown_event.h" #include "brpc/socket.h" #include "butil/object_pool.h" #include "brpc/builtin/version_service.h" @@ -378,34 +379,39 @@ TEST_F(ServerTest, failed_start_destruction_cleans_thread_local_data) { static bthread_key_t g_init_child_key = INVALID_BTHREAD_KEY; static bthread_t g_init_child = INVALID_BTHREAD; -static void* InitChildOutlivingFailedStart(void*) { - // Outlive the failed Start(), then borrow and return a KeyTable of the - // server's keytable pool. - bthread_usleep(100 * 1000); +static void* InitChildOutlivingFailedStart(void* arg) { + // Wait until the failed Start() has returned and the Server is destroyed. + EXPECT_EQ(0, static_cast(arg)->wait()); + // Create a KeyTable that will be returned to the pool on bthread exit. EXPECT_EQ(0, bthread_setspecific(g_init_child_key, (void*)1)); return nullptr; } -static bool InitFnSpawningChild(void*) { +static bool InitFnSpawningChild(void* arg) { bthread_attr_t attr; // attr.keytable_pool is the server's _keytable_pool. EXPECT_EQ(0, bthread_getattr(bthread_self(), &attr)); return bthread_start_background(&g_init_child, &attr, InitChildOutlivingFailedStart, - nullptr) == 0; + arg) == 0; } TEST_F(ServerTest, failed_start_keeps_keytable_pool_for_init_bthreads) { ASSERT_EQ(0, bthread_key_create(&g_init_child_key, nullptr)); brpc::Server listener; ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr)); + bthread::CountdownEvent server_destroyed; brpc::ServerOptions options; options.bthread_init_fn = InitFnSpawningChild; + options.bthread_init_args = &server_destroyed; options.bthread_init_count = 1; { brpc::Server server; - ASSERT_EQ(-1, server.Start(listener.listen_address(), &options)); + // Keep signaling and joining the child even if Start() unexpectedly + // succeeds, so it cannot remain blocked on a destroyed event. + EXPECT_EQ(-1, server.Start(listener.listen_address(), &options)); } + server_destroyed.signal(); // The child calls return_keytable() on the destroyed (but not deleted) // pool. Deleting the pool on the failure path is a use-after-free here // (reported by ASan). From 2a9a2f098f88c9cdf90bfaa57d41c6f5f520fd34 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Sun, 18 Oct 2026 00:50:31 +0800 Subject: [PATCH 4/4] refactor(server): share thread-local cleanup Centralize keytable pool and TLS key cleanup for normal shutdown and failed-start recovery while preserving their destruction order and pool lifetime constraints. --- src/brpc/server.cpp | 33 +++++++++++---------------------- src/brpc/server.h | 3 +++ 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/brpc/server.cpp b/src/brpc/server.cpp index 39363e1cd7..19d58ac422 100644 --- a/src/brpc/server.cpp +++ b/src/brpc/server.cpp @@ -1341,24 +1341,9 @@ int Server::Join() { _session_local_data_pool->Reset(nullptr); } - if (_keytable_pool) { - // Destroy _keytable_pool to delete keytables inside. This has to be - // done here (before leaving Join) because it's legal for users to - // delete bthread keys after Join which makes related objects - // in KeyTables undeletable anymore and leaked. - CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool)); - // TODO: Can't delete _keytable_pool which may be accessed by - // still-running bthreads (created by the server). The memory is - // leaked but servers are unlikely to be started/stopped frequently, - // the leak is acceptable in most scenarios. - _keytable_pool = nullptr; - } - - // Delete tls_key as well since we don't need it anymore. - if (_tl_options.tls_key != INVALID_BTHREAD_KEY) { - CHECK_EQ(0, bthread_key_delete(_tl_options.tls_key)); - _tl_options.tls_key = INVALID_BTHREAD_KEY; - } + // Destroy pooled keytables before leaving Join(): users may delete + // bthread keys afterwards, making their data undeletable and leaked. + DestroyThreadLocalResources(); // Have to join _derivative_thread, which may assume that server is running // and services in server are not mutated, otherwise data race happens @@ -1379,16 +1364,20 @@ void Server::DestroyPreStartAllocations() { // Handled by Join(). return; } + DestroyThreadLocalResources(); +} + +void Server::DestroyThreadLocalResources() { if (_keytable_pool != nullptr) { // Keep the TLS key valid until its pooled data has been destroyed. // Deleting the key first would disable DestroyServerTLS(). CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool)); - // Same as Join(): don't delete the pool struct. Bthreads created - // with attr.keytable_pool == _keytable_pool may outlive - // the failed start and still call return_keytable() on exit. A + // Don't delete the pool struct. Bthreads created with + // attr.keytable_pool == _keytable_pool may outlive shutdown or + // a failed start and still call return_keytable() on exit. A // destroyed pool makes them delete their KeyTables directly, but the // struct itself must stay valid. The leak is annotated in - // StartInternal() and only happens on failed starts. + // StartInternal(). _keytable_pool = nullptr; } // Delete tls_key created in StartInternal(). diff --git a/src/brpc/server.h b/src/brpc/server.h index d93d8a7a82..31dc69af2f 100644 --- a/src/brpc/server.h +++ b/src/brpc/server.h @@ -659,6 +659,9 @@ friend struct RevertServerStatus; // and a retry would overwrite _keytable_pool and leak the previous one. void DestroyPreStartAllocations(); + // Destroy pooled keytables before deleting the server's TLS key. + void DestroyThreadLocalResources(); + int AddServiceInternal(google::protobuf::Service* service, bool is_builtin_service, const ServiceOptions& options);