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

@wfurt wfurt 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 Aksoy and 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.
Copilot AI 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.

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated

Copilot AI 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
File Description
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs New 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.cs New helper parsing DNS-SD rdata into typed DNS record models.
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Adds DNS-SD P/Invoke bindings used by the macOS PAL.
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj Adds -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.csproj Adds -osx to functional test TFMs.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Expands functional coverage to include macOS for relevant tests and adds OSX-specific behavior assertions.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs Adds an OSX-only loopback test validating managed/custom-server behavior on macOS.
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csproj Links in additional production parsing files and includes new DNS-SD parsing unit tests.
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs New 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);
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
…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.
Copilot AI review requested due to automatic review settings August 6, 2026 14:05

Copilot AI 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.
Copilot AI review requested due to automatic review settings August 6, 2026 14:33

Copilot AI 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 thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment thread src/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.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.
Copilot AI review requested due to automatic review settings August 6, 2026 19:06
@wfurt

wfurt commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

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.

Copilot AI 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.

Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
@rzikm
rzikm self-requested a review August 7, 2026 13:57
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
Copilot AI review requested due to automatic review settings August 27, 2026 09:36

Copilot AI 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 thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs Outdated
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs Outdated
Copilot AI review requested due to automatic review settings August 27, 2026 10:04

Copilot AI 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));
Comment thread src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs Outdated
Copilot AI review requested due to automatic review settings August 27, 2026 11:49

Copilot AI 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.

Copilot AI 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) ----
Comment thread src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

5 participants