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

Narrow AddressFamily passed to getaddrinfo when IPv6 is unsupported #112642

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 18, 2025

When IPv6 is unsupported or disabled, we should avoid passing AF_UNSPEC to the platform calls since those will do AAAA resolve attempts which might result in failures surfacing to the user, see #107979. Instead, we can narrow down the query to AF_INET so failures are avoided.

The change has been validated by packet captures on Windows and Linux. There are no AAAA questions when System.Net.DisableIPv6 is set to true.

Fixes #107979.

@Copilot Copilot bot review requested due to automatic review settings February 18, 2025 01:31

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:390

  • The new method ValidateAddressFamily should be covered by tests to ensure its correctness.
private static bool ValidateAddressFamily(ref AddressFamily addressFamily, string hostName, bool justAddresses, [NotNullWhen(false)] out object? resultOnFailure)
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In #107979 (comment) you've mentioned that we filter out such results after the query. Should we be able to remove that logic, or can the OS still return AAAA results for IPv4-only calls?

@@ -386,10 +387,44 @@ private static IPHostEntry GetHostEntryCore(string hostName, AddressFamily addre
private static IPAddress[] GetHostAddressesCore(string hostName, AddressFamily addressFamily, NameResolutionActivity? activityOrDefault = default) =>
(IPAddress[])GetHostEntryOrAddressesCore(hostName, justAddresses: true, addressFamily, activityOrDefault);

private static bool ValidateAddressFamily(ref AddressFamily addressFamily, string hostName, bool justAddresses, [NotNullWhen(false)] out object? resultOnFailure)
{
if (!SocketProtocolSupportPal.OSSupportsIPv6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be symmetric e.g. should we apply same logic to IPv4?

Copy link
Member Author

@antonfirsov antonfirsov Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is OSSupportsIPv4 == false a practical real-life user scenario? I would be hesitant to add logic to handle it otherwise. It would be virtually untestable; we don't have a System.Net.DisableIPv4 switch, and I don't think it would make much sense to add one.

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabling IPV6 in .NET Core
3 participants