Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Implement DnsResolver for macOS - #131934

Open
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal
Open

Implement DnsResolver for macOS#131934
wfurt wants to merge 10 commits into
dotnet:mainfrom
wfurt:fix-macos-dns-pal

Conversation

@wfurt

@wfurtwfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member

Implements the macOS DNS PAL, following the Linux DNS resolver work in #129846 that just merged.

Overview

  • New file:DnsResolverPal.OSX.cs uses macOS's DNSServiceQueryRecord (mDNSResponder / DNS-SD via libSystem) when no explicit servers are configured, so the query goes through the system resolver and honours macOS resolver policy (search domains, split DNS, .local/mDNS, etc.).
  • When explicit DnsResolverOptions.Servers are set, the OSX PAL delegates to the shared managed DnsResolverPal.Managed.cs implementation added in Implement DnsResolver for Linux #129846, so custom-server queries share the same wire-format code as Linux.
  • Adds Interop.Dnssd bindings (DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult / DNSServiceRefDeallocate) under Common/src/Interop/OSX/.
  • Extends the Functional tests to also run on macOS and adds a loopback test that validates the custom-server (managed) path is used on OSX when servers are configured.
  • Extracts the DNS-SD rdata parsers (DnsSdRecord + TryParse*) into a standalone DnsSdRecordParsing.cs and adds direct unit tests for them so we don't need reflection into DnsResolverPal internals.

Relationship to prior work

Review feedback carried over

Mirroring the same class of feedback #129846 got, the reflection-based tests that reached into private DnsResolverPal members (DnsSdRecord / TryParseAddress / TryParseSrv / TryParseMx) have been replaced with straight unit tests against the extracted DnsSdRecordParsing type, linked into System.Net.NameResolution.Unit.Tests alongside the shared parsers.

Older review comments from #131152 remain worth reading — @gfoidl, @MihaZupan, @teo-tsirpanis feel free to re-post anything still open here.

Validation

Built and tested locally on macOS arm64:

  • System.Net.NameResolution.csproj builds clean on net11.0-osx, -unix, -windows, -browser, -wasi (0 warnings, 0 errors).
  • Unit tests (net11.0): 171 total / 171 passed / 0 failed — includes 16 new DnsSdRecordParsingTests.
  • Functional tests (net11.0-osx): 196 total / 187 passed / 9 skipped ([OuterLoop]) / 0 failed.
  • PAL tests (net11.0-unix): 34 / 34 passed.

/cc @liveans@rzikm@MihaZupan@gfoidl@teo-tsirpanis

Note

Parts of this PR (the rebase, the DNS-SD parser extraction, the new unit tests, and this description) were prepared with GitHub Copilot.

Ahmet İbrahim Aksoyand others added 2 commits August 6, 2026 12:05
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f11fb54e-638e-4c9e-ad82-e5d9ff5c20e4
Move DnsSdRecord and the DNSServiceQueryRecord rdata parsers out of
DnsResolverPal.OSX into a new DnsSdRecordParsing static class. Link the
parsing file (and DnsRecords.cs) into System.Net.NameResolution.Unit.Tests
alongside the other production parsers, and add direct unit tests for the
interface-index handling, root name / MX / SRV parsing, TXT framing, and
name-validation edge cases.
The reflection-based tests in DnsResolverTest.cs that reached into private
PAL members are removed in favor of the new unit tests.
CopilotAI lite review requested due to automatic review settings August 6, 2026 12:30
@wfurt
wfurt marked this pull request as ready for review August 6, 2026 12:30
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS implementation of the System.Net.NameResolution DNS resolver PAL, introducing DNS-SD (DNSServiceQueryRecord) based querying for “system resolver” lookups on macOS, while continuing to use the shared managed resolver for explicit/custom server configurations. The PR also expands functional coverage to run on macOS and adds unit tests for the extracted DNS-SD rdata parsing helpers.

Changes:

  • Add DnsResolverPal.OSX.cs that queries via DNS-SD when no explicit servers are configured, and otherwise delegates to the shared managed resolver.
  • Extract DNS-SD rdata parsing into DnsSdRecordParsing.cs and add direct unit tests for it.
  • Extend functional tests to target -osx and add an OSX loopback test verifying the managed/custom-server path.

Reviewed changes

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

Show a summary per file
FileDescription
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.csNew macOS DNS PAL using DNS-SD for system-resolver queries and delegating to managed resolver for custom servers.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.csNew helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.csAdds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csprojAdds -osx target and wires in the macOS PAL + DNS-SD interop + poll interop.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csprojAdds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.csExpands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.csAdds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csprojLinks in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.csNew unit tests for DnsSdRecordParsing helpers (address/srv/mx/txt/etc.).
Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:288

  • This test’s goal is prompt completion; asserting ResponseCode == NoError is stricter than needed and may be unstable if macOS surfaces NxDomain for some resolver paths. Accepting either NoError or NxDomain keeps the test focused on the timing/empty-records behavior.
 Assert.Equal(DnsResponseCode.NoError, result.ResponseCode);

…ask.Run
The previous async path wrapped the blocking Poll loop in Task.Run, which
pinned a thread-pool thread for the whole query duration. Wrap the
mDNSResponder fd (returned by DNSServiceRefSockFD) in a non-owning
System.Net.Sockets.Socket via the existing DnsSocket reflection cache and
await Socket.ReceiveAsync(Memory<byte>.Empty, ct) as a real async POLLIN.
DNSServiceProcessResult is still called synchronously when the wait
completes to consume + dispatch the record via the callback.
The sync path keeps Interop.Sys.Poll — a sync caller has already committed
a thread to blocking, and adding async plumbing there would only add
moving parts. Pre-canceled tokens on the async path return
Task.FromCanceled to preserve the TaskCanceledException surface the old
Task.Run(action, ct) shortcut produced.
Addresses feedback from teo-tsirpanis in dotnet#131934.
CopilotAI review requested due to automatic review settings August 6, 2026 14:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (8)

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:76

  • TryParseMx currently ignores trailing bytes after the exchange name. For MX records the exchange field should consume the rest of rdata; otherwise malformed data will be accepted. Capture bytesConsumed from TryParseDnsName and validate it matches the remaining length.
 if (data.Length >= 3 && TryParseDnsName(data.Slice(2), out string exchange, out _))
{

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:112

  • TryParseCName succeeds even if the DNS name terminates before the end of rdata (trailing bytes are ignored). To match the existing DNS wire parsers, require the parsed name to consume the entire rdata (bytesConsumed == record.Data.Length).
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new CNameRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:124

  • TryParsePtr should reject PTR rdata that has trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to avoid accepting malformed data.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new PtrRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:136

  • TryParseNs should reject NS rdata with trailing bytes after the domain name. Currently it accepts early termination. Require bytesConsumed == record.Data.Length to align with the existing DNS parsers in this library.
 if (TryParseDnsName(record.Data, out string name, out _))
{
parsed = new NsRecord(name, TimeSpan.FromSeconds(record.Ttl));

src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:79

  • The macOS PAL validates NUL in hostnames, but other platforms currently allow it. Since NUL can truncate UTF-8 / native interop strings, this validation should ideally be centralized in the shared DnsResolver.ValidateName path so behavior and security characteristics are consistent across platforms.
 ValidateServers(servers);
if (name.Contains('\0'))
{
throw new ArgumentException(SR.net_hostname_invalid_character, nameof(name));
}

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:162

  • This section header is now inaccurate because the tests below run on both Windows and macOS (IsWindowsOrOSX). Update the comment to avoid misleading future readers.
 // ---- Windows network tests (require outbound DNS) ----
[ConditionalFact(typeof(DnsResolverTest), nameof(IsWindowsOrOSX))]

src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:55

  • TryParseSrv accepts a DNS name that terminates early and ignores trailing bytes. Other parsers in this codebase require the DNS name to consume the entire remaining rdata (e.g., DnsRecordParsing.TryParseSingleDnsNameRecord checks bytesConsumed). This should reject records with trailing garbage by validating bytesConsumed.

This issue also appears in the following locations of the same file:

  • line 75
  • line 110
  • line 122
  • line 134
 if (data.Length >= 7 && TryParseDnsName(data.Slice(6), out string target, out _))
{

src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs:110

  • The new DNS-SD name parsers currently don't have a unit test ensuring trailing bytes after a terminated name are rejected. Adding at least one coverage case (e.g., CNAME with extra data) would prevent regressions and aligns with the existing wire-format parsers which require full rdata consumption.
 [Fact]
public void TryParseCName_ParsesDottedName()
{
byte[] data = { 3, (byte)'w', (byte)'w', (byte)'w', 7, (byte)'e', (byte)'x', (byte)'a', (byte)'m', (byte)'p', (byte)'l', (byte)'e', 0 };
DnsSdRecord record = new DnsSdRecord(5, data, ttl: 60, interfaceIndex: 0);

- Move the embedded-NUL check into DnsResolver.ValidateName so every
platform rejects NUL-injected names (Windows DnsQueryEx and macOS
DNSServiceQueryRecord both take null-terminated strings), and drop the
OSX-only copy.
- Widen DNS labels byte-by-byte in DnsSdRecordParsing.TryParseDnsName
instead of Encoding.UTF8.GetString, matching how the managed resolver
decodes response labels (deterministic output for non-UTF-8 bytes).
- Loosen ResolveAddresses_NonExistent_ReturnsNxDomain and the OSX
CompletesPromptly variant to accept either NoError or NxDomain, since
mDNSResponder can report NXDOMAIN as NoSuchName or NoSuchRecord
depending on version.
CopilotAI review requested due to automatic review settings August 6, 2026 14:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:76

  • ValidateName now rejects embedded NUL on all platforms, but this test is gated to macOS only. That leaves the cross-platform argument-validation behavior untested on Windows/Linux and makes regressions easier to miss.
 [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsOSX))]

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment threadsrc/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
- Interop.Dnssd now marshals the DNSServiceRef as SafeDnsServiceHandle
instead of raw IntPtr for DNSServiceQueryRecord/RefSockFD/ProcessResult,
so handle lifetime is managed by the LibraryImport source generator.
Move SafeDnsServiceHandle to file scope (internal) with a parameterless
ctor so the source generator can construct it for out params.
- Collapse the OSX PAL's Query/QueryCore/QueryRecord sync/async pairs
into single methods that take bool async and branch only at the actual
wait (Interop.Sys.Poll vs DnsSocket.WaitReadableAsync). Pre-canceled
check moves up into Query.
- DnsSocket.WaitReadableAsync: switch from Socket.ReceiveAsync with an
empty buffer to a 1-byte SocketFlags.Peek. An empty-buffer receive
completes synchronously with zero bytes on Unix (0-byte recv returns
immediately without ever waiting for POLLIN), so we'd have busy-looped
calling DNSServiceProcessResult without data. Peek leaves the byte in
the socket for DNSServiceProcessResult to consume. Dispose the scratch
Socket via ((IDisposable)socket).Dispose() instead of a reflected
Dispose delegate.
Verified locally with the OuterLoop async DNS-SD tests (A/AAAA/CNAME
chain, IPv4-only, non-existent, SRV) on macOS arm64 - all 12 pass.
CopilotAI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
MemberAuthor

Addressed the structural feedback in 9eb0fb2:

  • Interop.Dnssd now marshals DNSServiceRef as SafeDnsServiceHandle for DNSServiceQueryRecord / DNSServiceRefSockFD / DNSServiceProcessResult (moved to file scope with a parameterless ctor so the LibraryImport source generator can construct it for out params).
  • Unified Query / QueryCore / QueryRecord sync/async pairs into single methods taking bool async that branch only at the actual wait primitive; pre-canceled check hoisted into Query.
  • DnsSocket.WaitReadableAsync switched to a 1-byte SocketFlags.Peek receive — @copilot-pull-request-reviewer was right that an empty-buffer ReceiveAsync completes synchronously on Unix without waiting for POLLIN, so the previous code would have busy-looped calling DNSServiceProcessResult. Peek leaves the byte in the socket for DNSServiceProcessResult to consume normally. Dispose is now ((IDisposable)socket).Dispose().

Left the [UnsafeAccessor] refactor of DnsSocket as a follow-up — worth doing in one pass across both this file and the Linux managed PAL so the shape stays consistent.

Verified with the OuterLoop async DNS-SD tests (A/AAAA/CNAME chain, IPv4-only, non-existent, SRV) on macOS arm64 — all 12 pass, plus the full inner-loop suite.

Note

The code changes and this comment were generated by GitHub Copilot on my behalf; I reviewed them but did most of the driving from prompts rather than hand-editing.

CopilotAI reviewed Aug 6, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 09:36

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
CopilotAI review requested due to automatic review settings August 27, 2026 10:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:45

  • These argument-validation tests construct DnsResolver() even though the default ctor is unsupported on Android (it throws when Servers is empty). That makes the tests fail on Android even though they never send a query. Consider using an explicitly-configured loopback server endpoint for these tests (or reinstating the prior CreateResolver helper) so the tests remain platform-independent.
 using DnsResolver r = new DnsResolver();

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:169

  • This assertion is too specific: the managed Unix PAL uses CancellationToken.ThrowIfCancellationRequested(), which propagates OperationCanceledException (not necessarily TaskCanceledException). To avoid platform-dependent flakiness, use ThrowsAnyAsync as before.
 await Assert.ThrowsAsync<TaskCanceledException>(() => r.ResolveAddressesAsync(TestHost, cts.Token));

CopilotAI review requested due to automatic review settings August 27, 2026 11:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:283

  • ResolveAddresses_NonExistent_ReturnsNxDomain no longer has the Windows Server 2025 guard, but #131188 tracks that this test can return ServerFailure on that queue. As-is this is likely to reintroduce Helix failures; either re-add the guard or relax the assertion for that platform.
 // mDNSResponder can surface a negative answer as either NoSuchName (NxDomain) or
// NoSuchRecord (mapped to NoError with no records); accept either on macOS.
if (PlatformDetection.IsOSX)
{
Assert.Contains(result.ResponseCode, new[] { DnsResponseCode.NoError, DnsResponseCode.NxDomain });

src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:167

  • This section header says "Windows network tests" but the tests below are gated by IsSupportedPlatform (non-mobile/non-browser/non-wasi) and run on Linux/macOS as well. Updating the comment avoids confusion when diagnosing failures on non-Windows platforms.
 // ---- Windows network tests (require outbound DNS) ----

Sign up for freeto 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.

5 participants

@wfurt@AustinWise@teo-tsirpanis@rzikm