From c9df35a4d18ace94cd0632cd8f6951cffe3112f8 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Thu, 23 May 2024 14:53:31 -0700 Subject: [PATCH] [Clean-up] Fix MSAN and UBSAN issues found by clang-19 (#36707) Fixed various MSAN and UBSAN issues found in an attempt to bump the clang version used for RBE. (https://github.com/grpc/grpc/pull/36685) As our xSAN tests are using RBE, it revealed a few new issues. This PR is to fix all of those. Closes #36707 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36707 from veblush:fix-xsan ebbebc2d4e47f05ad2a406cdae6b0f603ac5242f PiperOrigin-RevId: 636685138 --- include/grpcpp/impl/call_op_set.h | 4 +- src/core/lib/iomgr/ev_poll_posix.cc | 38 ++++++++++++------- .../end2end/multiple_server_queues_test.cc | 1 + test/core/iomgr/tcp_server_posix_test.cc | 13 ++++++- test/core/surface/completion_queue_test.cc | 22 +++++------ 5 files changed, 50 insertions(+), 28 deletions(-) diff --git a/include/grpcpp/impl/call_op_set.h b/include/grpcpp/impl/call_op_set.h index 56ce6289082..d18cafcc337 100644 --- a/include/grpcpp/impl/call_op_set.h +++ b/include/grpcpp/impl/call_op_set.h @@ -771,7 +771,9 @@ class CallOpRecvInitialMetadata { class CallOpClientRecvStatus { public: CallOpClientRecvStatus() - : recv_status_(nullptr), debug_error_string_(nullptr) {} + : metadata_map_(nullptr), + recv_status_(nullptr), + debug_error_string_(nullptr) {} void ClientRecvStatus(grpc::ClientContext* context, Status* status) { client_context_ = context; diff --git a/src/core/lib/iomgr/ev_poll_posix.cc b/src/core/lib/iomgr/ev_poll_posix.cc index 7d290d7d83e..d4c78557f06 100644 --- a/src/core/lib/iomgr/ev_poll_posix.cc +++ b/src/core/lib/iomgr/ev_poll_posix.cc @@ -267,19 +267,29 @@ struct grpc_pollset_set { // static void fork_fd_list_remove_node(grpc_fork_fd_list* node) { + gpr_mu_lock(&fork_fd_list_mu); + if (fork_fd_list_head == node) { + fork_fd_list_head = node->next; + } + if (node->prev != nullptr) { + node->prev->next = node->next; + } + if (node->next != nullptr) { + node->next->prev = node->prev; + } + gpr_free(node); + gpr_mu_unlock(&fork_fd_list_mu); +} + +static void fork_fd_list_remove_grpc_fd(grpc_fd* fd) { if (track_fds_for_fork) { - gpr_mu_lock(&fork_fd_list_mu); - if (fork_fd_list_head == node) { - fork_fd_list_head = node->next; - } - if (node->prev != nullptr) { - node->prev->next = node->next; - } - if (node->next != nullptr) { - node->next->prev = node->prev; - } - gpr_free(node); - gpr_mu_unlock(&fork_fd_list_mu); + fork_fd_list_remove_node(fd->fork_fd_list); + } +} + +static void fork_fd_list_remove_wakeup_fd(grpc_cached_wakeup_fd* fd) { + if (track_fds_for_fork) { + fork_fd_list_remove_node(fd->fork_fd_list); } } @@ -361,7 +371,7 @@ static void unref_by(grpc_fd* fd, int n) { if (old == n) { gpr_mu_destroy(&fd->mu); grpc_iomgr_unregister_object(&fd->iomgr_object); - fork_fd_list_remove_node(fd->fork_fd_list); + fork_fd_list_remove_grpc_fd(fd); if (fd->shutdown) { } fd->shutdown_error.~Status(); @@ -860,7 +870,7 @@ static void pollset_destroy(grpc_pollset* pollset) { CHECK(!pollset_has_workers(pollset)); while (pollset->local_wakeup_cache) { grpc_cached_wakeup_fd* next = pollset->local_wakeup_cache->next; - fork_fd_list_remove_node(pollset->local_wakeup_cache->fork_fd_list); + fork_fd_list_remove_wakeup_fd(pollset->local_wakeup_cache); grpc_wakeup_fd_destroy(&pollset->local_wakeup_cache->fd); gpr_free(pollset->local_wakeup_cache); pollset->local_wakeup_cache = next; diff --git a/test/core/end2end/multiple_server_queues_test.cc b/test/core/end2end/multiple_server_queues_test.cc index 4711a7a2147..4b49412649c 100644 --- a/test/core/end2end/multiple_server_queues_test.cc +++ b/test/core/end2end/multiple_server_queues_test.cc @@ -37,6 +37,7 @@ int main(int argc, char** argv) { attr.version = 1; attr.cq_completion_type = GRPC_CQ_NEXT; attr.cq_polling_type = GRPC_CQ_DEFAULT_POLLING; + attr.cq_shutdown_cb = nullptr; cq1 = grpc_completion_queue_create( grpc_completion_queue_factory_lookup(&attr), &attr, nullptr); diff --git a/test/core/iomgr/tcp_server_posix_test.cc b/test/core/iomgr/tcp_server_posix_test.cc index 469d6990502..235c789717a 100644 --- a/test/core/iomgr/tcp_server_posix_test.cc +++ b/test/core/iomgr/tcp_server_posix_test.cc @@ -39,6 +39,7 @@ #include #endif +#include #include #include "absl/log/log.h" @@ -330,8 +331,16 @@ static void test_connect(size_t num_connects, const grpc_channel_args* channel_args, test_addrs* dst_addrs, bool test_dst_addrs) { grpc_core::ExecCtx exec_ctx; - grpc_resolved_address resolved_addr; - grpc_resolved_address resolved_addr1; + // Use aligned_stroage to allocate grpc_resolved_address objects on stack + // to meet the alignment requirement of sockaddr_storage type. + std::aligned_storage::type resolved_addr_buffer; + std::aligned_storage::type resolved_addr1_buffer; + grpc_resolved_address& resolved_addr = + *reinterpret_cast(&resolved_addr_buffer); + grpc_resolved_address& resolved_addr1 = + *reinterpret_cast(&resolved_addr1_buffer); struct sockaddr_storage* const addr = reinterpret_cast(resolved_addr.addr); struct sockaddr_storage* const addr1 = diff --git a/test/core/surface/completion_queue_test.cc b/test/core/surface/completion_queue_test.cc index 82dbaed6dd6..9175196fa1a 100644 --- a/test/core/surface/completion_queue_test.cc +++ b/test/core/surface/completion_queue_test.cc @@ -78,7 +78,7 @@ TEST(GrpcCompletionQueueTest, TestNoOp) { grpc_cq_completion_type completion_types[] = {GRPC_CQ_NEXT, GRPC_CQ_PLUCK}; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; LOG_TEST("test_no_op"); attr.version = 1; @@ -97,7 +97,7 @@ TEST(GrpcCompletionQueueTest, TestPollsetConversion) { grpc_cq_polling_type polling_types[] = {GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING}; grpc_completion_queue* cq; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; LOG_TEST("test_pollset_conversion"); @@ -118,7 +118,7 @@ TEST(GrpcCompletionQueueTest, TestWaitEmpty) { grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; grpc_completion_queue* cc; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; grpc_event event; LOG_TEST("test_wait_empty"); @@ -145,7 +145,7 @@ TEST(GrpcCompletionQueueTest, TestCqEndOp) { grpc_cq_completion completion; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; void* tag = create_test_tag(); LOG_TEST("test_cq_end_op"); @@ -178,7 +178,7 @@ TEST(GrpcCompletionQueueTest, TestCqTlsCacheFull) { grpc_cq_completion completion; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; void* tag = create_test_tag(); void* res_tag; int ok; @@ -219,7 +219,7 @@ TEST(GrpcCompletionQueueTest, TestCqTlsCacheEmpty) { grpc_completion_queue* cc; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; void* res_tag; int ok; @@ -246,7 +246,7 @@ TEST(GrpcCompletionQueueTest, TestShutdownThenNextPolling) { grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; grpc_completion_queue* cc; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; grpc_event event; LOG_TEST("test_shutdown_then_next_polling"); @@ -268,7 +268,7 @@ TEST(GrpcCompletionQueueTest, TestShutdownThenNextWithTimeout) { grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; grpc_completion_queue* cc; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; grpc_event event; LOG_TEST("test_shutdown_then_next_with_timeout"); @@ -294,7 +294,7 @@ TEST(GrpcCompletionQueueTest, TestPluck) { grpc_cq_completion completions[GPR_ARRAY_SIZE(tags)]; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; unsigned i, j; LOG_TEST("test_pluck"); @@ -348,7 +348,7 @@ TEST(GrpcCompletionQueueTest, TestPluckAfterShutdown) { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; grpc_event ev; grpc_completion_queue* cc; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; LOG_TEST("test_pluck_after_shutdown"); @@ -372,7 +372,7 @@ TEST(GrpcCompletionQueueTest, TestCallback) { grpc_cq_completion completions[GPR_ARRAY_SIZE(tags)]; grpc_cq_polling_type polling_types[] = { GRPC_CQ_DEFAULT_POLLING, GRPC_CQ_NON_LISTENING, GRPC_CQ_NON_POLLING}; - grpc_completion_queue_attributes attr; + grpc_completion_queue_attributes attr = {}; unsigned i; static gpr_mu mu, shutdown_mu; static gpr_cv cv, shutdown_cv;