diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ab40a5dbb357..430059722cccd 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) @@ -8489,6 +8492,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 34177320ba521..52f8636b2c291 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -6371,6 +6371,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/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..3a5db49dfdb77 --- /dev/null +++ b/src/ruby/end2end/concurrent_close_during_fork_test.rb @@ -0,0 +1,105 @@ +#!/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/ +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..07d7f2ca1dcef 100644 --- a/test/core/event_engine/posix/BUILD +++ b/test/core/event_engine/posix/BUILD @@ -504,6 +504,32 @@ grpc_cc_library( ], ) +grpc_cc_test( + name = "ares_resolver_fork_safety_test", + srcs = ["ares_resolver_fork_safety_test.cc"], + copts = ["-DGRPC_ENABLE_FORK_SUPPORT"], + 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..04479a3586ad3 --- /dev/null +++ b/test/core/event_engine/posix/ares_resolver_fork_safety_test.cc @@ -0,0 +1,146 @@ +// 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/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, 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" ):