From 211bd0e588c7c188fa131789620d40b3ebfa4fd2 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Thu, 14 Jun 2018 16:33:50 -0700 Subject: Prevent pollable from accessing a potentially orphaned/destroyed fd --- src/core/lib/iomgr/ev_epollex_linux.cc | 53 ++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 15 deletions(-) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 55a2b98372..927224fd2c 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -103,8 +103,10 @@ struct pollable { int epfd; grpc_wakeup_fd wakeup; - // only for type fd... one ref to the owner fd - grpc_fd* owner_fd; + // The following are relevant only for type PO_FD + grpc_fd* owner_fd; // Set to the owner_fd if the type is PO_FD + gpr_mu owner_orphan_mu; // Synchronizes access to owner_orphaned field + bool owner_orphaned; // Is the owner fd orphaned grpc_pollset_set* pollset_set; pollable* next; @@ -445,6 +447,17 @@ static void fd_orphan(grpc_fd* fd, grpc_closure* on_done, int* release_fd, gpr_mu_lock(&fd->orphan_mu); + // Get the fd->pollable_obj and set the owner_orphaned on that pollable to + // true so that the pollable will no longer access its owner_fd field. + gpr_mu_lock(&fd->pollable_mu); + pollable* pollable_obj = fd->pollable_obj; + gpr_mu_unlock(&fd->pollable_mu); + + if (pollable_obj) { + gpr_mu_lock(&pollable_obj->owner_orphan_mu); + pollable_obj->owner_orphaned = true; + } + fd->on_done_closure = on_done; /* If release_fd is not NULL, we should be relinquishing control of the file @@ -466,6 +479,10 @@ static void fd_orphan(grpc_fd* fd, grpc_closure* on_done, int* release_fd, GRPC_CLOSURE_SCHED(fd->on_done_closure, GRPC_ERROR_NONE); + if (pollable_obj) { + gpr_mu_unlock(&pollable_obj->owner_orphan_mu); + } + gpr_mu_unlock(&fd->orphan_mu); UNREF_BY(fd, 2, reason); /* Drop the reference */ @@ -550,6 +567,8 @@ static grpc_error* pollable_create(pollable_type type, pollable** p) { gpr_mu_init(&(*p)->mu); (*p)->epfd = epfd; (*p)->owner_fd = nullptr; + gpr_mu_init(&(*p)->owner_orphan_mu); + (*p)->owner_orphaned = false; (*p)->pollset_set = nullptr; (*p)->next = (*p)->prev = *p; (*p)->root_worker = nullptr; @@ -845,10 +864,15 @@ static void fd_become_writable(grpc_fd* fd) { fd->write_closure->SetReady(); } static void fd_has_errors(grpc_fd* fd) { fd->error_closure->SetReady(); } -static grpc_error* fd_get_or_become_pollable(grpc_fd* fd, pollable** p) { +/* Get the pollable_obj attached to this fd. If none is attached, create a new + * pollable object (of type PO_FD), attach it to the fd and return it + * + * Note that if a pollable object is already attached to the fd, it may be of + * either PO_FD or PO_MULTI type */ +static grpc_error* get_fd_pollable(grpc_fd* fd, pollable** p) { gpr_mu_lock(&fd->pollable_mu); grpc_error* error = GRPC_ERROR_NONE; - static const char* err_desc = "fd_get_or_become_pollable"; + static const char* err_desc = "get_fd_pollable"; if (fd->pollable_obj == nullptr) { if (append_error(&error, pollable_create(PO_FD, &fd->pollable_obj), err_desc)) { @@ -1185,7 +1209,7 @@ static grpc_error* pollset_transition_pollable_from_empty_to_fd_locked( } append_error(&error, pollset_kick_all(pollset), err_desc); POLLABLE_UNREF(pollset->active_pollable, "pollset"); - append_error(&error, fd_get_or_become_pollable(fd, &pollset->active_pollable), + append_error(&error, get_fd_pollable(fd, &pollset->active_pollable), err_desc); return error; } @@ -1229,17 +1253,15 @@ static grpc_error* pollset_add_fd_locked(grpc_pollset* pollset, grpc_fd* fd) { error = pollset_transition_pollable_from_empty_to_fd_locked(pollset, fd); break; case PO_FD: - gpr_mu_lock(&po_at_start->owner_fd->orphan_mu); - if ((gpr_atm_no_barrier_load(&pollset->active_pollable->owner_fd->refst) & - 1) == 0) { - error = - pollset_transition_pollable_from_empty_to_fd_locked(pollset, 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); } else { /* fd --> multipoller */ error = pollset_transition_pollable_from_fd_to_multi_locked(pollset, fd); } - gpr_mu_unlock(&po_at_start->owner_fd->orphan_mu); + gpr_mu_unlock(&po_at_start->owner_orphan_mu); break; case PO_MULTI: error = pollable_add_fd(pollset->active_pollable, fd); @@ -1275,16 +1297,17 @@ static grpc_error* pollset_as_multipollable_locked(grpc_pollset* pollset, append_error(&error, pollset_kick_all(pollset), err_desc); break; case PO_FD: - gpr_mu_lock(&po_at_start->owner_fd->orphan_mu); - if ((gpr_atm_no_barrier_load(&pollset->active_pollable->owner_fd->refst) & - 1) == 0) { + gpr_mu_lock(&po_at_start->owner_orphan_mu); + if (po_at_start->owner_orphaned) { + // Unlock before Unref'ing the pollable + gpr_mu_unlock(&po_at_start->owner_orphan_mu); POLLABLE_UNREF(pollset->active_pollable, "pollset"); error = pollable_create(PO_MULTI, &pollset->active_pollable); } else { error = pollset_transition_pollable_from_fd_to_multi_locked(pollset, nullptr); + gpr_mu_unlock(&po_at_start->owner_orphan_mu); } - gpr_mu_unlock(&po_at_start->owner_fd->orphan_mu); break; case PO_MULTI: break; -- cgit v1.2.3 From f5691a5fec26cabc4e448ead0fb431879c0c82b3 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Fri, 15 Jun 2018 14:03:16 -0700 Subject: add the missing gpr_mu_destroy --- src/core/lib/iomgr/ev_epollex_linux.cc | 1 + 1 file changed, 1 insertion(+) (limited to 'src/core') diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 927224fd2c..f3db72e85c 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -608,6 +608,7 @@ static void pollable_unref(pollable* p, int line, const char* reason) { GRPC_FD_TRACE("pollable_unref: Closing epfd: %d", p->epfd); close(p->epfd); grpc_wakeup_fd_destroy(&p->wakeup); + gpr_mu_destroy(&p->owner_orphan_mu); gpr_free(p); } } -- cgit v1.2.3 From 16ad9b828073579d0c6364570a3803b26e7fb39d Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Sat, 16 Jun 2018 12:14:30 -0700 Subject: Added a test to catch such things in future --- CMakeLists.txt | 33 ++++++ Makefile | 36 +++++++ build.yaml | 15 +++ src/core/lib/iomgr/ev_epollex_linux.cc | 30 ++++-- test/core/iomgr/BUILD | 21 +++- test/core/iomgr/ev_epollex_linux_test.cc | 115 +++++++++++++++++++++ tools/run_tests/generated/sources_and_headers.json | 17 +++ tools/run_tests/generated/tests.json | 20 ++++ 8 files changed, 273 insertions(+), 14 deletions(-) create mode 100644 test/core/iomgr/ev_epollex_linux_test.cc (limited to 'src/core') diff --git a/CMakeLists.txt b/CMakeLists.txt index 9f532b047d..ff46d9cfcc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -244,6 +244,9 @@ endif() add_dependencies(buildtests_c endpoint_pair_test) add_dependencies(buildtests_c error_test) if(_gRPC_PLATFORM_LINUX) +add_dependencies(buildtests_c ev_epollex_linux_test) +endif() +if(_gRPC_PLATFORM_LINUX) add_dependencies(buildtests_c ev_epollsig_linux_test) endif() add_dependencies(buildtests_c fake_resolver_test) @@ -6163,6 +6166,36 @@ endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) if(_gRPC_PLATFORM_LINUX) +add_executable(ev_epollex_linux_test + test/core/iomgr/ev_epollex_linux_test.cc +) + + +target_include_directories(ev_epollex_linux_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include + PRIVATE ${_gRPC_SSL_INCLUDE_DIR} + PRIVATE ${_gRPC_PROTOBUF_INCLUDE_DIR} + PRIVATE ${_gRPC_ZLIB_INCLUDE_DIR} + PRIVATE ${_gRPC_BENCHMARK_INCLUDE_DIR} + PRIVATE ${_gRPC_CARES_INCLUDE_DIR} + PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} + PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} +) + +target_link_libraries(ev_epollex_linux_test + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util + grpc + gpr_test_util + gpr +) + +endif() +endif (gRPC_BUILD_TESTS) +if (gRPC_BUILD_TESTS) +if(_gRPC_PLATFORM_LINUX) + add_executable(ev_epollsig_linux_test test/core/iomgr/ev_epollsig_linux_test.cc ) diff --git a/Makefile b/Makefile index bc51761d36..dd194dc006 100644 --- a/Makefile +++ b/Makefile @@ -970,6 +970,7 @@ dns_resolver_test: $(BINDIR)/$(CONFIG)/dns_resolver_test dualstack_socket_test: $(BINDIR)/$(CONFIG)/dualstack_socket_test endpoint_pair_test: $(BINDIR)/$(CONFIG)/endpoint_pair_test error_test: $(BINDIR)/$(CONFIG)/error_test +ev_epollex_linux_test: $(BINDIR)/$(CONFIG)/ev_epollex_linux_test ev_epollsig_linux_test: $(BINDIR)/$(CONFIG)/ev_epollsig_linux_test fake_resolver_test: $(BINDIR)/$(CONFIG)/fake_resolver_test fake_transport_security_test: $(BINDIR)/$(CONFIG)/fake_transport_security_test @@ -1415,6 +1416,7 @@ buildtests_c: privatelibs_c \ $(BINDIR)/$(CONFIG)/dualstack_socket_test \ $(BINDIR)/$(CONFIG)/endpoint_pair_test \ $(BINDIR)/$(CONFIG)/error_test \ + $(BINDIR)/$(CONFIG)/ev_epollex_linux_test \ $(BINDIR)/$(CONFIG)/ev_epollsig_linux_test \ $(BINDIR)/$(CONFIG)/fake_resolver_test \ $(BINDIR)/$(CONFIG)/fake_transport_security_test \ @@ -1935,6 +1937,8 @@ test_c: buildtests_c $(Q) $(BINDIR)/$(CONFIG)/endpoint_pair_test || ( echo test endpoint_pair_test failed ; exit 1 ) $(E) "[RUN] Testing error_test" $(Q) $(BINDIR)/$(CONFIG)/error_test || ( echo test error_test failed ; exit 1 ) + $(E) "[RUN] Testing ev_epollex_linux_test" + $(Q) $(BINDIR)/$(CONFIG)/ev_epollex_linux_test || ( echo test ev_epollex_linux_test failed ; exit 1 ) $(E) "[RUN] Testing ev_epollsig_linux_test" $(Q) $(BINDIR)/$(CONFIG)/ev_epollsig_linux_test || ( echo test ev_epollsig_linux_test failed ; exit 1 ) $(E) "[RUN] Testing fake_resolver_test" @@ -11046,6 +11050,38 @@ endif endif +EV_EPOLLEX_LINUX_TEST_SRC = \ + test/core/iomgr/ev_epollex_linux_test.cc \ + +EV_EPOLLEX_LINUX_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(EV_EPOLLEX_LINUX_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/ev_epollex_linux_test: openssl_dep_error + +else + + + +$(BINDIR)/$(CONFIG)/ev_epollex_linux_test: $(EV_EPOLLEX_LINUX_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LD) $(LDFLAGS) $(EV_EPOLLEX_LINUX_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/ev_epollex_linux_test + +endif + +$(OBJDIR)/$(CONFIG)/test/core/iomgr/ev_epollex_linux_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_ev_epollex_linux_test: $(EV_EPOLLEX_LINUX_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(EV_EPOLLEX_LINUX_TEST_OBJS:.o=.dep) +endif +endif + + EV_EPOLLSIG_LINUX_TEST_SRC = \ test/core/iomgr/ev_epollsig_linux_test.cc \ diff --git a/build.yaml b/build.yaml index e8620bfb4c..6a4304116c 100644 --- a/build.yaml +++ b/build.yaml @@ -2277,6 +2277,21 @@ targets: - gpr_test_util - gpr uses_polling: false +- name: ev_epollex_linux_test + cpu_cost: 3 + build: test + language: c + src: + - test/core/iomgr/ev_epollex_linux_test.cc + deps: + - grpc_test_util + - grpc + - gpr_test_util + - gpr + exclude_iomgrs: + - uv + platforms: + - linux - name: ev_epollsig_linux_test cpu_cost: 3 build: test diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index f3db72e85c..1ce4181143 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -339,21 +339,29 @@ static void ref_by(grpc_fd* fd, int n) { GPR_ASSERT(gpr_atm_no_barrier_fetch_add(&fd->refst, n) > 0); } +/* Uninitialize and add to the freelist */ static void fd_destroy(void* arg, grpc_error* error) { grpc_fd* fd = static_cast(arg); - /* Add the fd to the freelist */ grpc_iomgr_unregister_object(&fd->iomgr_object); POLLABLE_UNREF(fd->pollable_obj, "fd_pollable"); gpr_mu_destroy(&fd->pollable_mu); gpr_mu_destroy(&fd->orphan_mu); - gpr_mu_lock(&fd_freelist_mu); - fd->freelist_next = fd_freelist; - fd_freelist = fd; fd->read_closure->DestroyEvent(); 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 + + /* Add the fd to the freelist */ + gpr_mu_lock(&fd_freelist_mu); + fd->freelist_next = fd_freelist; + fd_freelist = fd; gpr_mu_unlock(&fd_freelist_mu); } @@ -409,20 +417,18 @@ static grpc_fd* fd_create(int fd, const char* name, bool track_err) { new_fd->error_closure.Init(); } - gpr_mu_init(&new_fd->pollable_mu); - gpr_mu_init(&new_fd->orphan_mu); - new_fd->pollable_obj = nullptr; - gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); new_fd->fd = fd; - new_fd->track_err = track_err; new_fd->salt = gpr_atm_no_barrier_fetch_add(&g_fd_salt, 1); + gpr_atm_rel_store(&new_fd->refst, (gpr_atm)1); + gpr_mu_init(&new_fd->orphan_mu); + gpr_mu_init(&new_fd->pollable_mu); + new_fd->pollable_obj = nullptr; new_fd->read_closure->InitEvent(); new_fd->write_closure->InitEvent(); new_fd->error_closure->InitEvent(); - gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); - new_fd->freelist_next = nullptr; new_fd->on_done_closure = nullptr; + gpr_atm_no_barrier_store(&new_fd->read_notifier_pollset, (gpr_atm)NULL); char* fd_name; gpr_asprintf(&fd_name, "%s fd=%d", name, fd); @@ -433,6 +439,8 @@ static grpc_fd* fd_create(int fd, const char* name, bool track_err) { } #endif gpr_free(fd_name); + + new_fd->track_err = track_err; return new_fd; } diff --git a/test/core/iomgr/BUILD b/test/core/iomgr/BUILD index bbf0815e6f..cc1b6aee5e 100644 --- a/test/core/iomgr/BUILD +++ b/test/core/iomgr/BUILD @@ -18,7 +18,10 @@ licenses(["notice"]) # Apache v2 load("//test/core/util:grpc_fuzzer.bzl", "grpc_fuzzer") -grpc_package(name = "test/core/iomgr", visibility = "public") # Useful for third party devs to test their io manager implementation. +grpc_package( + name = "test/core/iomgr", + visibility = "public", +) # Useful for third party devs to test their io manager implementation. grpc_cc_library( name = "endpoint_tests", @@ -72,16 +75,28 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "ev_epollex_linux_test", + srcs = ["ev_epollex_linux_test.cc"], + language = "C++", + deps = [ + "//:gpr", + "//:grpc", + "//test/core/util:gpr_test_util", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "ev_epollsig_linux_test", srcs = ["ev_epollsig_linux_test.cc"], + language = "C++", deps = [ "//:gpr", "//:grpc", "//test/core/util:gpr_test_util", "//test/core/util:grpc_test_util", ], - language = "C++", ) grpc_cc_test( @@ -221,13 +236,13 @@ grpc_cc_test( name = "tcp_server_posix_test", srcs = ["tcp_server_posix_test.cc"], language = "C++", + tags = ["manual"], # TODO(adelez): Remove once this works on Foundry. deps = [ "//:gpr", "//:grpc", "//test/core/util:gpr_test_util", "//test/core/util:grpc_test_util", ], - tags = ["manual"], # TODO(adelez): Remove once this works on Foundry. ) grpc_cc_test( diff --git a/test/core/iomgr/ev_epollex_linux_test.cc b/test/core/iomgr/ev_epollex_linux_test.cc new file mode 100644 index 0000000000..08d1e68b39 --- /dev/null +++ b/test/core/iomgr/ev_epollex_linux_test.cc @@ -0,0 +1,115 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +#include "src/core/lib/iomgr/port.h" + +/* This test only relevant on linux systems where epoll() is available */ +#if defined(GRPC_LINUX_EPOLL_CREATE1) && defined(GRPC_LINUX_EVENTFD) +#include "src/core/lib/iomgr/ev_epollex_linux.h" + +#include +#include +#include + +#include "test/core/util/test_config.h" + +static void pollset_destroy(void* ps, grpc_error* error) { + grpc_pollset_destroy(static_cast(ps)); + gpr_free(ps); +} + +// This test is added to cover the case found in bug: +// https://github.com/grpc/grpc/issues/15760 +static void test_pollable_owner_fd() { + grpc_core::ExecCtx exec_ctx; + int ev_fd1; + int ev_fd2; + grpc_fd* grpc_fd1; + grpc_fd* grpc_fd2; + grpc_pollset* ps; + gpr_mu* mu; + + // == Create two grpc_fds == + // All we need is two file descriptors. Doesn't matter what type. We use + // eventfd type here for the purpose of this test + ev_fd1 = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); + ev_fd2 = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); + if (ev_fd1 < 0 || ev_fd2 < 0) { + gpr_log(GPR_ERROR, "Error in creating event fds for the test"); + return; + } + grpc_fd1 = grpc_fd_create(ev_fd1, "epollex-test-fd1", false); + grpc_fd2 = grpc_fd_create(ev_fd2, "epollex-test-fd2", false); + grpc_core::ExecCtx::Get()->Flush(); + + // == Create a pollset == + ps = static_cast(gpr_zalloc(grpc_pollset_size())); + grpc_pollset_init(ps, &mu); + grpc_core::ExecCtx::Get()->Flush(); + + // == Add fd1 to pollset == + grpc_pollset_add_fd(ps, grpc_fd1); + grpc_core::ExecCtx::Get()->Flush(); + + // == Destroy fd1 == + grpc_fd_orphan(grpc_fd1, nullptr, nullptr, "test fd1 orphan"); + grpc_core::ExecCtx::Get()->Flush(); + + // = Add fd2 to pollset == + // + // Before https://github.com/grpc/grpc/issues/15760, the following line caused + // unexpected behavior (The previous grpc_pollset_add_fd(ps, grpc_fd1) created + // an underlying structure in epollex that held a reference to grpc_fd1 which + // was being accessed here even after grpc_fd_orphan(grpc_fd1) was called + grpc_pollset_add_fd(ps, grpc_fd2); + grpc_core::ExecCtx::Get()->Flush(); + + // == Destroy fd2 == + grpc_fd_orphan(grpc_fd2, nullptr, nullptr, "test fd2 orphan"); + grpc_core::ExecCtx::Get()->Flush(); + + // == Destroy pollset + grpc_closure ps_destroy_closure; + GRPC_CLOSURE_INIT(&ps_destroy_closure, pollset_destroy, ps, + grpc_schedule_on_exec_ctx); + grpc_pollset_shutdown(ps, &ps_destroy_closure); + grpc_core::ExecCtx::Get()->Flush(); +} + +int main(int argc, char** argv) { + const char* poll_strategy = nullptr; + grpc_test_init(argc, argv); + grpc_init(); + { + grpc_core::ExecCtx exec_ctx; + poll_strategy = grpc_get_poll_strategy_name(); + if (poll_strategy != nullptr && strcmp(poll_strategy, "epollex") == 0) { + test_pollable_owner_fd(); + } else { + gpr_log(GPR_INFO, + "Skipping the test. The test is only relevant for 'epollex' " + "strategy. and the current strategy is: '%s'", + poll_strategy); + } + } + + grpc_shutdown(); + return 0; +} +#else /* defined(GRPC_LINUX_EPOLL_CREATE1) && defined(GRPC_LINUX_EVENTFD) */ +int main(int argc, char** argv) { return 0; } +#endif diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 4b5f7d51ea..0162de8217 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -449,6 +449,23 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc_test_util" + ], + "headers": [], + "is_filegroup": false, + "language": "c", + "name": "ev_epollex_linux_test", + "src": [ + "test/core/iomgr/ev_epollex_linux_test.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index ba63352036..afd654bf33 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -561,6 +561,26 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux" + ], + "cpu_cost": 3, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "gtest": false, + "language": "c", + "name": "ev_epollex_linux_test", + "platforms": [ + "linux" + ], + "uses_polling": true + }, { "args": [], "benchmark": false, -- cgit v1.2.3 From fb082835793cacfd7b64eaebc68836baccf82895 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 19 Jun 2018 10:56:27 -0700 Subject: 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(-) (limited to 'src/core') diff --git a/CMakeLists.txt b/CMakeLists.txt index f8850a9402..2dd56cd627 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 39b9d36946..5ffabdc665 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 = -- cgit v1.2.3