Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c-ares DNS resolver: remove unnecessary code in SRV callback #31426

Merged
merged 1 commit into from Oct 25, 2022

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Oct 21, 2022

This call to grpc_ares_notify_on_event_locked from the SRV callback is unnecessary:

There are only a few places that on_srv_query_done_locked callback can be invoked from, and we already call notify_on_event_locked (updating polled file descriptors) after each of them:

  1. Within the call to ares_query that initiated the SRV lookup
  1. When ares_process_fd is called from the c-ares backup poller.
  1. When ares_process_fd is called because an fd became readable.
  1. When ares_process_fd is called because an fd became writable.

Furthermore, this call to grpc_ares_notify_on_event_locked from the SRV callback was buggy, because in certain cases it could destroy the fd that generated the on_readable_locked callback it's being invoked from, which could then cause a use-after-free in here. So it looks like the direct cause of b/243676671.

@apolcyn apolcyn merged commit 16b8386 into grpc:master Oct 25, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants