From a5e30b7df94f4aebb1ca1fd0470f8234c7ad63d7 Mon Sep 17 00:00:00 2001 From: Randy Stauner Date: Mon, 30 Mar 2026 15:10:17 -0700 Subject: [PATCH 1/2] Fix SIGABRT in AresResolver destructor during fork Fix a crash in AresResolver::~AresResolver() that occurs when Reset() is called via ReinitHandle during fork, followed by the resolver being destroyed without a subsequent Restart(). This can be triggered by Process._fork hooks in ruby code that shut down gRPC resources during fork. The fork handler calls Reset() on resolvers, which destroys channel_ and sets it to nullptr. If the application then tears down the resolver (Orphan) before Restart() runs, the destructor fires with channel_ == nullptr and hits GRPC_CHECK_NE(channel_, nullptr) -> SIGABRT. The fix replaces the unconditional CHECK with a conditional: only call ares_destroy() if channel_ is non-null. This is safe because Reset() already calls ares_destroy() before nullifying channel_. Includes regression tests: - C++ test: ares_resolver_fork_safety_test (Reset then Orphan without Restart, both with and without pending DNS lookups) - Ruby end2end test: concurrent_close_during_fork_test (channels destroyed on background thread during Process._fork) --- src/core/lib/event_engine/ares_resolver.cc | 5 +- .../concurrent_close_during_fork_test.rb | 108 +++++++++++++ test/core/event_engine/posix/BUILD | 25 +++ .../posix/ares_resolver_fork_safety_test.cc | 149 ++++++++++++++++++ tools/run_tests/run_tests.py | 2 + 5 files changed, 287 insertions(+), 2 deletions(-) create mode 100755 src/ruby/end2end/concurrent_close_during_fork_test.rb create mode 100644 test/core/event_engine/posix/ares_resolver_fork_safety_test.cc diff --git a/src/core/lib/event_engine/ares_resolver.cc b/src/core/lib/event_engine/ares_resolver.cc index 2d80ce22a43ff..33a834b163c35 100644 --- a/src/core/lib/event_engine/ares_resolver.cc +++ b/src/core/lib/event_engine/ares_resolver.cc @@ -282,8 +282,9 @@ AresResolver::AresResolver( AresResolver::~AresResolver() { GRPC_CHECK(fd_node_list_.empty()); GRPC_CHECK(callback_map_.empty()); - GRPC_CHECK_NE(channel_, nullptr); - ares_destroy(channel_); + if (channel_ != nullptr) { + ares_destroy(channel_); + } } void AresResolver::Orphan() { diff --git a/src/ruby/end2end/concurrent_close_during_fork_test.rb b/src/ruby/end2end/concurrent_close_during_fork_test.rb new file mode 100755 index 0000000000000..f4fdd6684c482 --- /dev/null +++ b/src/ruby/end2end/concurrent_close_during_fork_test.rb @@ -0,0 +1,108 @@ +#!/usr/bin/env ruby +# +# Copyright 2026 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. + +# Regression test for crash in AresResolver during Process.fork when +# channels are destroyed concurrently with fork's pthread_atfork handlers. +# +# Bug: When a Process._fork hook destroys gRPC channels on a background +# thread while fork() is in progress, the channel destruction (resolver +# Orphan) races with the fork handler's resolver Reset/Restart lifecycle. +# This corrupts resolver state, causing a SEGFAULT or SIGABRT +# (GRPC_CHECK_NE(channel_, nullptr) in AresResolver::~AresResolver). +# +# This simulates a production crash triggered by a Process._fork hook +# that shuts down gRPC resources during fork. +# +# Crashed with SEGFAULT (signal 11) or SIGABRT (signal 6) before the fix. + +ENV['GRPC_ENABLE_FORK_SUPPORT'] = "1" +fail "forking only supported on linux" unless RUBY_PLATFORM =~ /linux/ +# TODO(apolcyn): remove after this experiment is on by default +ENV['GRPC_EXPERIMENTS'] = "event_engine_fork" + +this_dir = File.expand_path(File.dirname(__FILE__)) +protos_lib_dir = File.join(this_dir, 'lib') +grpc_lib_dir = File.join(File.dirname(this_dir), 'lib') +$LOAD_PATH.unshift(grpc_lib_dir) unless $LOAD_PATH.include?(grpc_lib_dir) +$LOAD_PATH.unshift(protos_lib_dir) unless $LOAD_PATH.include?(protos_lib_dir) +$LOAD_PATH.unshift(this_dir) unless $LOAD_PATH.include?(this_dir) + +require 'sanity_check_dlopen' +require 'grpc' +require 'end2end_common' + +# Destroy gRPC channels on a background thread concurrently with fork. +# If something shuts down gRPC resources during Process._fork, +# this races with fork's pthread_atfork handlers that manage resolver lifecycle. +module ConcurrentCloseOnForkHook + def _fork + t = Thread.new do + ObjectSpace.each_object(GRPC::Core::Channel) do |ch| + ch.close + rescue StandardError + nil + end + end + pid = super + begin + t.kill + rescue StandardError + nil + end + pid + end +end +Process.singleton_class.prepend(ConcurrentCloseOnForkHook) + +def main + 5.times do |attempt| + # Create channels with active DNS resolution to populate resolver state + channels = 20.times.map do |i| + ch = GRPC::Core::Channel.new( + "dns:///test-#{attempt}-#{i}-#{rand(100_000)}.example.com:443", + {}, + :this_channel_is_insecure + ) + ch.connectivity_state(true) + ch + end + sleep 0.05 + + with_logging("attempt #{attempt}: fork") do + pid = fork { sleep 0.05 } + _, status = Process.wait2(pid) + sig = status.termsig + if sig + label = case sig + when 6 then "SIGABRT" + when 11 then "SEGFAULT" + else "signal #{sig}" + end + fail "child crashed with #{label} during fork with concurrent channel close" + end + end + + channels.each do |ch| + ch.close + rescue StandardError + nil + end + channels.clear + end + STDERR.puts "all forks completed without crash" +end + +main diff --git a/test/core/event_engine/posix/BUILD b/test/core/event_engine/posix/BUILD index 0e75f194471a2..c43e43b23e86e 100644 --- a/test/core/event_engine/posix/BUILD +++ b/test/core/event_engine/posix/BUILD @@ -504,6 +504,31 @@ grpc_cc_library( ], ) +grpc_cc_test( + name = "ares_resolver_fork_safety_test", + srcs = ["ares_resolver_fork_safety_test.cc"], + external_deps = [ + "gtest", + "absl/status", + "absl/status:statusor", + ], + tags = [ + "no_windows", + ], + uses_event_engine = True, + uses_polling = True, + deps = [ + ":dns_server", + "//:event_engine_base_hdrs", + "//:gpr_platform", + "//:grpc", + "//src/core:ares_resolver", + "//src/core:notification", + "//src/core:posix_event_engine", + "//test/core/test_util:grpc_test_util", + ], +) + grpc_cc_test( name = "dns_fork_test", srcs = ["dns_fork_test.cc"], diff --git a/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc b/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc new file mode 100644 index 0000000000000..d91686c75b880 --- /dev/null +++ b/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc @@ -0,0 +1,149 @@ +// Copyright 2026 The 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. + +// Regression test for SIGABRT in AresResolver::~AresResolver() when +// Reset() is called via ReinitHandle during fork, followed by resolver +// teardown without Restart(). +// +// Bug: Reset() unconditionally destroys channel_ and sets it to nullptr. +// If the resolver is then orphaned (e.g. application tears down gRPC +// resources during fork), the destructor fires with channel_ == nullptr +// and hits GRPC_CHECK_NE(channel_, nullptr) → SIGABRT. +// +// This test verifies that Reset() followed by Orphan() (without Restart) +// does NOT crash. +// +// Build: bazel test --config=fork_support \ +// //test/core/event_engine/posix:ares_resolver_fork_safety_test + +#include +#include +#include + +#include + +#include "src/core/lib/event_engine/ares_resolver.h" +#include "src/core/lib/event_engine/posix_engine/grpc_polled_fd_posix.h" +#include "src/core/lib/event_engine/posix_engine/posix_engine.h" +#include "src/core/util/notification.h" +#include "test/core/event_engine/posix/dns_server.h" +#include "test/core/test_util/port.h" +#include "test/core/test_util/test_config.h" +#include "gtest/gtest.h" +#include "absl/status/status.h" +#include "absl/status/statusor.h" + +namespace grpc_event_engine::experimental { + +#ifdef GRPC_ENABLE_FORK_SUPPORT + +// Named AresResolverTest to match PosixEventEngine's friend declaration, +// allowing access to the private poller_ member. +class AresResolverTest : public ::testing::Test { + protected: + static PosixEventPoller* GetPoller(PosixEventEngine* engine) { + return engine->poller_.get(); + } +}; + +// Verifies that an AresResolver can be safely destroyed after Reset() +// has been called without a subsequent Restart(). +// +// This simulates the production crash scenario where: +// 1. A fork handler calls Reset() on the resolver via ReinitHandle +// (e.g. PosixEventEngine::AfterForkInChild) +// 2. The application tears down gRPC resources during fork, which +// orphans the resolver before Restart() can be called +// 3. The resolver destructor should handle channel_ == nullptr gracefully +TEST_F(AresResolverTest, ResetThenOrphanDoesNotCrash) { + auto ee = std::static_pointer_cast(GetDefaultEventEngine()); + auto* poller = GetPoller(ee.get()); + + auto ares_resolver = AresResolver::CreateAresResolver( + "", std::make_unique(poller), ee); + ASSERT_TRUE(ares_resolver.ok()) << ares_resolver.status(); + + auto reinit_handle = ares_resolver->get()->GetReinitHandle().lock(); + ASSERT_NE(reinit_handle, nullptr); + + // Simulate fork handler: Reset destroys channel_ and sets to nullptr + reinit_handle->Reset(absl::CancelledError("simulated fork reset")); + + // Simulate application teardown: Orphan the resolver without Restart. + // Orphan() calls OnResolverGone() (makes future Restart() a no-op), + // then Unref(). This triggers ~AresResolver() which should not crash + // even though channel_ is nullptr. + ares_resolver->reset(); + + // If we get here, the resolver was safely destroyed. Clean up handle. + reinit_handle.reset(); +} + +// Verifies that an AresResolver with in-flight DNS lookups can be safely +// destroyed after Reset() without Restart(). +// +// This is a more realistic scenario: the resolver has active DNS queries +// when the fork handler fires and tears things down. +TEST_F(AresResolverTest, + ResetThenOrphanWithPendingLookupsDoesNotCrash) { + auto dns_server = DnsServer::Start(grpc_pick_unused_port_or_die()); + ASSERT_TRUE(dns_server.ok()) << dns_server.status(); + + auto ee = std::static_pointer_cast(GetDefaultEventEngine()); + auto* poller = GetPoller(ee.get()); + + auto ares_resolver = AresResolver::CreateAresResolver( + dns_server->address(), + std::make_unique(poller), ee); + ASSERT_TRUE(ares_resolver.ok()) << ares_resolver.status(); + + auto reinit_handle = ares_resolver->get()->GetReinitHandle().lock(); + ASSERT_NE(reinit_handle, nullptr); + + // Start DNS lookups to create in-flight resolver state + grpc_core::Notification lookup_done; + ares_resolver->get()->LookupHostname( + [&lookup_done](const auto& /*result*/) { lookup_done.Notify(); }, + "test.host.example.com", "443"); + + // Wait for the query to reach the DNS server + dns_server->WaitForQuestion("test.host.example.com"); + + // Now simulate the crash scenario: Reset then Orphan + reinit_handle->Reset(absl::CancelledError("simulated fork reset")); + ares_resolver->reset(); + reinit_handle.reset(); + + // Wait for the lookup callback (it was cancelled by Reset) + lookup_done.WaitForNotification(); +} + +#else // GRPC_ENABLE_FORK_SUPPORT + +TEST(AresResolverTest, Skipped) { + GTEST_SKIP() << "Fork support is disabled"; +} + +#endif // GRPC_ENABLE_FORK_SUPPORT + +} // namespace grpc_event_engine::experimental + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + grpc::testing::TestEnvironment env(&argc, argv); + grpc_init(); + auto result = RUN_ALL_TESTS(); + grpc_shutdown(); + return result; +} diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index 615506de944ff..154cf06b8b9f6 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -982,6 +982,7 @@ def test_specs(self): "src/ruby/end2end/killed_client_thread_test.rb", "src/ruby/end2end/forking_client_test.rb", "src/ruby/end2end/fork_test_repro_35489.rb", + "src/ruby/end2end/concurrent_close_during_fork_test.rb", "src/ruby/end2end/multiple_killed_watching_threads_test.rb", "src/ruby/end2end/client_memory_usage_test.rb", "src/ruby/end2end/package_with_underscore_test.rb", @@ -1004,6 +1005,7 @@ def test_specs(self): "src/ruby/end2end/prefork_without_using_grpc_test.rb", "src/ruby/end2end/prefork_postfork_loop_test.rb", "src/ruby/end2end/fork_test_repro_35489.rb", + "src/ruby/end2end/concurrent_close_during_fork_test.rb", ] and platform_string() == "mac" ): From 938d2a2d130da30ca2d1b4ec306d7d90c939c157 Mon Sep 17 00:00:00 2001 From: rwstauner <142719+rwstauner@users.noreply.github.com> Date: Mon, 30 Mar 2026 23:33:44 +0000 Subject: [PATCH 2/2] Automated change: Fix sanity tests --- CMakeLists.txt | 50 +++++++++++++++++++ build_autogenerated.yaml | 17 +++++++ .../posix/ares_resolver_fork_safety_test.cc | 11 ++-- tools/run_tests/generated/tests.json | 22 ++++++++ 4 files changed, 93 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 182064b71b128..acf707f87f68a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1300,6 +1300,9 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx alts_zero_copy_grpc_protector_test) add_dependencies(buildtests_cxx arena_promise_test) add_dependencies(buildtests_cxx arena_test) + if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) + add_dependencies(buildtests_cxx ares_resolver_fork_safety_test) + endif() add_dependencies(buildtests_cxx async_end2end_test) add_dependencies(buildtests_cxx auth_context_test) add_dependencies(buildtests_cxx auth_property_iterator_test) @@ -8399,6 +8402,53 @@ target_link_libraries(arena_test ) +endif() +if(gRPC_BUILD_TESTS) +if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) + + add_executable(ares_resolver_fork_safety_test + test/core/event_engine/posix/ares_resolver_fork_safety_test.cc + test/core/event_engine/posix/dns_server.cc + ) + if(WIN32 AND MSVC) + if(BUILD_SHARED_LIBS) + target_compile_definitions(ares_resolver_fork_safety_test + PRIVATE + "GPR_DLL_IMPORTS" + "GRPC_DLL_IMPORTS" + "GRPCXX_DLL_IMPORTS" + ) + endif() + endif() + target_compile_features(ares_resolver_fork_safety_test PUBLIC cxx_std_17) + target_include_directories(ares_resolver_fork_safety_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} + ) + + target_link_libraries(ares_resolver_fork_safety_test + ${_gRPC_ALLTARGETS_LIBRARIES} + gtest + grpc++ + grpc_test_util + ) + + +endif() endif() if(gRPC_BUILD_TESTS) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index cdd29cf7df596..b92b84a5fc932 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -6347,6 +6347,23 @@ targets: - gtest - grpc_test_util_unsecure uses_polling: false +- name: ares_resolver_fork_safety_test + gtest: true + build: test + language: c++ + headers: + - test/core/event_engine/posix/dns_server.h + src: + - test/core/event_engine/posix/ares_resolver_fork_safety_test.cc + - test/core/event_engine/posix/dns_server.cc + deps: + - gtest + - grpc++ + - grpc_test_util + platforms: + - linux + - posix + - mac - name: async_end2end_test gtest: true build: test diff --git a/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc b/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc index d91686c75b880..04479a3586ad3 100644 --- a/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc +++ b/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc @@ -95,8 +95,7 @@ TEST_F(AresResolverTest, ResetThenOrphanDoesNotCrash) { // // This is a more realistic scenario: the resolver has active DNS queries // when the fork handler fires and tears things down. -TEST_F(AresResolverTest, - ResetThenOrphanWithPendingLookupsDoesNotCrash) { +TEST_F(AresResolverTest, ResetThenOrphanWithPendingLookupsDoesNotCrash) { auto dns_server = DnsServer::Start(grpc_pick_unused_port_or_die()); ASSERT_TRUE(dns_server.ok()) << dns_server.status(); @@ -104,8 +103,8 @@ TEST_F(AresResolverTest, auto* poller = GetPoller(ee.get()); auto ares_resolver = AresResolver::CreateAresResolver( - dns_server->address(), - std::make_unique(poller), ee); + dns_server->address(), std::make_unique(poller), + ee); ASSERT_TRUE(ares_resolver.ok()) << ares_resolver.status(); auto reinit_handle = ares_resolver->get()->GetReinitHandle().lock(); @@ -131,9 +130,7 @@ TEST_F(AresResolverTest, #else // GRPC_ENABLE_FORK_SUPPORT -TEST(AresResolverTest, Skipped) { - GTEST_SKIP() << "Fork support is disabled"; -} +TEST(AresResolverTest, Skipped) { GTEST_SKIP() << "Fork support is disabled"; } #endif // GRPC_ENABLE_FORK_SUPPORT diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index bd6f53b1967b4..6b0ab7c6d00a4 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -545,6 +545,28 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "ares_resolver_fork_safety_test", + "platforms": [ + "linux", + "mac", + "posix" + ], + "uses_polling": true + }, { "args": [], "benchmark": false,