c-ares DNS resolver: fix logical race between resolution timeout/cancellation and fd readability #31443

apolcyn · 2022-10-24T19:07:20Z

This PR adds a test originally meant to hit the bug described in #31426. This test caught a more general, long-standing bug which is fixed here:

If we:

Have more than one DNS query outstanding on the same file descriptor (for example, an A query and a AAAA query)
Cancel the overall resolution (for example due to the overall timeout kicking in)
Concurrently with 2), receive data from the DNS server for one of those queries (for example, just the A query)

... then, we can hit a bug where we run the on_readable callback for the A query and process the data as if the resolution hasn't been cancelled. Even though we've already shut down the file descriptor with the still-pending AAAA query, the c-ares library thinks we still want to wait for this AAAA query to finish. But it never will finish, because we'll stop polling I/O at this point without ever having told c-ares to cancel the query.

The fix is to explicitly check for overall cancellation in the I/O callbacks and tell c-ares to cancel everything in that case.

While we're here:

fix the same race in resolver_component_test.cc that was fixed for other tests in Fix races in resolve_address and resolve_address_posix tests #25398
fix a trace log

markdroth · 2022-10-24T23:36:32Z

test/cpp/naming/resolver_component_test.cc

@@ -192,9 +192,9 @@ gpr_timespec TestDeadline(void) {

 struct ArgsStruct {
  gpr_event ev;
-  gpr_atm done_atm;
  gpr_mu* mu;


Can we convert this to use grpc_core::Mutex instead, so that we can use lock annotations and allow the compiler to enforce the lock guard?

We can do the same thing in the tests that were fixed in #25398.

I'd like to do that, but we can't because this gpr_mu actually belongs to the pollset, so we'd need to migrate pollset to grpc_core::Mutex first.

Ah, okay. This is probably something that we'll be able to address as part of migrating this code to EventEngine.

markdroth · 2022-10-24T23:39:18Z

test/cpp/naming/resolver_component_test.cc

  // Shutdown and join stress thread
  gpr_event_set(&done_ev, reinterpret_cast<void*>(1));
  socket_stress_thread.join();
 }

+TEST(ResolverComponentTest, TestDoesntCrashOrHangWith1MsTimeout) {
+  // Queries in this test could either complete successfully or time out


I guess there's no way to trigger exactly the sequence of events that we care about here without having a much more flexible fake DNS server, right?

Yeah, I think we'd actually need some kind of mocked endpoint to do that. While it would be nice to have a test like that, it's not obvious to me how we could structure something like that, given the way that the c-ares library interacts directly with file descriptors.

While the current test doesn't take a deterministic code path, it at least won't flake in a way that yields a false positive. And it's been useful so far (since it quickly surfaced this bug while I was trying to repro the separate issue of #31426, I think it's giving some previously-missing but important coverage).

Maybe what we need is a fake c-ares implementation. We could implement an interface that wraps around c-ares and have the DNS resolver code use c-ares via that interface. We could then also implement a fake version of that interface, so that we can have it respond to the DNS resolver code however we want.

But in any case, I think this is a project for another day.

Maybe what we need is a fake c-ares implementation. We could implement an interface that wraps around c-ares and have the DNS resolver code use c-ares via that interface.

Perhaps. But IMO this code pretty tightly coupled to the behavior of the c-ares library, and I think most of the meat of this code is about properly handling the behavior of the c-ares library. So I'd be weary of mocking at the c-ares API layer for the risk that we'd just end up testing our mock.

While I'm not really sure how to do it at the moment, I'd still be a lot more excited about something that mocked out only the "untrusted" parts of the code, e.g. timing and the bytes received off the wire.

markdroth · 2022-10-25T14:55:59Z

test/cpp/naming/resolver_component_test.cc

@@ -192,9 +192,9 @@ gpr_timespec TestDeadline(void) {

 struct ArgsStruct {
  gpr_event ev;
-  gpr_atm done_atm;
  gpr_mu* mu;


Ah, okay. This is probably something that we'll be able to address as part of migrating this code to EventEngine.

markdroth · 2022-10-25T14:58:37Z

test/cpp/naming/resolver_component_test.cc

  // Shutdown and join stress thread
  gpr_event_set(&done_ev, reinterpret_cast<void*>(1));
  socket_stress_thread.join();
 }

+TEST(ResolverComponentTest, TestDoesntCrashOrHangWith1MsTimeout) {
+  // Queries in this test could either complete successfully or time out


Maybe what we need is a fake c-ares implementation. We could implement an interface that wraps around c-ares and have the DNS resolver code use c-ares via that interface. We could then also implement a fake version of that interface, so that we can have it respond to the DNS resolver code however we want.

But in any case, I think this is a project for another day.

apolcyn added 3 commits October 24, 2022 18:38

Remove unnecessary call from c-ares resolver SRV handler

ce28be9

Fix race between c-ares resolution timeout and fd readability

bc17f91

format

9803c37

apolcyn requested a review from markdroth as a code owner October 24, 2022 19:07

github-actions bot added lang/c++ lang/core labels Oct 24, 2022

apolcyn assigned markdroth Oct 24, 2022

apolcyn added the release notes: yes Indicates if PR needs to be in release notes label Oct 24, 2022

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Oct 24, 2022

fix sanity

8c1ed25

markdroth reviewed Oct 24, 2022

View reviewed changes

markdroth approved these changes Oct 25, 2022

View reviewed changes

apolcyn added the kokoro:force-run label Oct 27, 2022

grpc-kokoro removed the kokoro:force-run label Oct 27, 2022

apolcyn merged commit e5f7b1b into grpc:master Oct 28, 2022

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c-ares DNS resolver: fix logical race between resolution timeout/cancellation and fd readability #31443

c-ares DNS resolver: fix logical race between resolution timeout/cancellation and fd readability #31443

apolcyn commented Oct 24, 2022 •

edited

markdroth Oct 24, 2022

apolcyn Oct 24, 2022

markdroth Oct 25, 2022

markdroth Oct 24, 2022

apolcyn Oct 25, 2022

markdroth Oct 25, 2022

apolcyn Oct 25, 2022

markdroth Oct 25, 2022

markdroth Oct 25, 2022

c-ares DNS resolver: fix logical race between resolution timeout/cancellation and fd readability #31443

c-ares DNS resolver: fix logical race between resolution timeout/cancellation and fd readability #31443

Conversation

apolcyn commented Oct 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apolcyn commented Oct 24, 2022 •

edited