From fb082835793cacfd7b64eaebc68836baccf82895 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 19 Jun 2018 10:56:27 -0700 Subject: [PATCH] Fix bug in pollset_add_fd_locked and a tsan error --- CMakeLists.txt | 1 + src/core/lib/iomgr/ev_epollex_linux.cc | 31 ++++++++++++++++++++------ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f8850a94027..2dd56cd6276 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7545,6 +7545,7 @@ target_include_directories(handshake_verify_peer_options PRIVATE ${_gRPC_CARES_INCLUDE_DIR} PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + PRIVATE ${_gRPC_NANOPB_INCLUDE_DIR} ) target_link_libraries(handshake_verify_peer_options diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 39b9d369466..5ffabdc6654 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -340,6 +340,27 @@ static void ref_by(grpc_fd* fd, int n) { GPR_ASSERT(gpr_atm_no_barrier_fetch_add(&fd->refst, n) > 0); } +#ifndef NDEBUG +#define INVALIDATE_FD(fd) invalidate_fd(fd) +/* Since an fd is never really destroyed (i.e gpr_free() is not called), it is + * hard to cases where fd fields are accessed even after calling fd_destroy(). + * The following invalidates fd fields to make catching such errors easier */ +static void invalidate_fd(grpc_fd* fd) { + fd->fd = -1; + fd->salt = -1; + gpr_atm_no_barrier_store(&fd->refst, -1); + memset(&fd->orphan_mu, -1, sizeof(fd->orphan_mu)); + memset(&fd->pollable_mu, -1, sizeof(fd->pollable_mu)); + fd->pollable_obj = nullptr; + fd->on_done_closure = nullptr; + gpr_atm_no_barrier_store(&fd->read_notifier_pollset, 0); + memset(&fd->iomgr_object, -1, sizeof(fd->iomgr_object)); + fd->track_err = false; +} +#else +#define INVALIDATE_FD(fd) +#endif + /* Uninitialize and add to the freelist */ static void fd_destroy(void* arg, grpc_error* error) { grpc_fd* fd = static_cast(arg); @@ -352,12 +373,7 @@ static void fd_destroy(void* arg, grpc_error* error) { fd->write_closure->DestroyEvent(); fd->error_closure->DestroyEvent(); -#ifndef NDEBUG - // Since we do not call gpr_free on the fd (and put it in a freelist instead), - // the following will help us catch any 'use-after-fd_destroy()' errors in the - // code - memset(fd, 0xFF, sizeof(grpc_fd)); -#endif + INVALIDATE_FD(fd); /* Add the fd to the freelist */ gpr_mu_lock(&fd_freelist_mu); @@ -1265,7 +1281,8 @@ static grpc_error* pollset_add_fd_locked(grpc_pollset* pollset, grpc_fd* fd) { case PO_FD: gpr_mu_lock(&po_at_start->owner_orphan_mu); if (po_at_start->owner_orphaned) { - pollset_transition_pollable_from_empty_to_fd_locked(pollset, fd); + error = + pollset_transition_pollable_from_empty_to_fd_locked(pollset, fd); } else { /* fd --> multipoller */ error =