Conversation
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.
…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.
- 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.
- 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.
A CNAME query can legitimately return NODATA (NoError with no records) if the name only has A/AAAA records, so require only that any returned records are well-formed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Moderate functional-test coverage and CI-regression issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds macOS DNS resolution through DNS-SD while retaining the managed resolver for explicit servers.
Changes:
- Adds DNS-SD interop, PAL logic, socket handling, and record parsing.
- Adds macOS functional and parser unit-test coverage.
- Adds embedded-NUL and TXT-record validation.
File summaries
| File | Summary |
|---|---|
src/libraries/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csproj |
Includes DNS-SD parser tests. |
src/libraries/System.Net.NameResolution/tests/UnitTests/DnsSdRecordParsingTests.cs |
Tests DNS-SD record parsing. |
src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj |
Configures functional tests for macOS. |
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs |
Updates resolver functional coverage and platform conditions. |
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs |
Updates loopback resolver coverage. |
src/libraries/System.Net.NameResolution/src/System/Net/DnsSocket.cs |
Refactors socket access and readiness handling. |
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs |
Implements DNS-SD record parsers. |
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs |
Implements macOS DNS-SD resolution. |
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs |
Adds embedded-NUL validation. |
src/libraries/System.Net.NameResolution/src/System/Net/DnsRecordParsing.cs |
Exposes TXT parser validity. |
src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj |
Wires macOS sources and interop. |
src/libraries/Common/src/Interop/OSX/Interop.Dnssd.cs |
Adds DNS-SD native bindings. |
Review details
Suppressed comments (4)
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs:523
- The rationale is inaccurate for the managed resolver:
DnsEncodedName.TryEncodebuilds DNS wire data and rejects the embedded NUL as an invalid label; it does not pass the name to native code as a null-terminated string. Please describe the native and managed validation reasons separately so this comment does not document behavior that the Linux/explicit-server path does not have.
// Every underlying resolver (Windows DnsQueryEx, macOS DNSServiceQueryRecord,
// and the managed stub resolver on Linux) passes the name to native code as a
// null-terminated string, so an embedded NUL would silently truncate the query.
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:273
- Removing the existing Windows Server 2025 guard reintroduces the open CI failure tracked by #131188: this resolver can return
ServerFailurefor the.invalidname on that queue, but this test still requiresNxDomainon Windows. Please restore the guard or relax the assertion to accept the documented resolver-dependent negative result.
public async Task ResolveAddresses_NonExistent_ReturnsNxDomain(bool async)
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:452
- The removed non-Windows acceptance tests covered the managed PAL contract that nonstandard ports and mixed IPv4/IPv6 server lists are valid. The new OSX overload deliberately delegates explicit-server queries to that PAL, but the remaining loopback tests do not verify mixed-family constructor acceptance; please keep or replace this coverage for OSX as well.
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))]
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs:414
- This changes the existing static
Dns.ResolveAddressesnetwork test to Windows-only, so Linux and macOS no longer exercise the public static API even thoughDns.ResolveAddressesremains supported there (Dns.Resolve.cs:17-50) and this PR does not make it Windows-specific. Keep the prior supported-platform condition (or useIsSupportedPlatform) so this coverage is retained on Unix and macOS.
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))]
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
- Restore DnsResolver_UnsupportedPlatform_ThrowsPlatformNotSupported coverage for the Browser/WASI DnsResolverPal.Unsupported query path, updated to use the instance DnsResolver API. - Remove the now-unused Poll interop includes from System.Net.NameResolution.csproj; the OSX async path waits via DnsSocket.ReceiveAsync instead of polling. - Rename the misleading "Windows network tests" comment heading to reflect that these tests run on all supported platforms. - Simplify DnsSocket.Dispose to cast to IDisposable directly and drop the redundant UnsafeAccessor Dispose wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Address the two unresolved moderate correctness findings before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.OSX.cs:270
- Because this query requests
kDNSServiceFlagsReturnIntermediates, mDNSResponder can invoke the callback for a CNAME/intermediate record whoserrtypeis not_requestedType. Returning before processingkDNSServiceFlagsMoreComingleavesIsCompletefalse when that callback is the final callback, soDNSServiceProcessResultis called again and can block until timeout for CNAME-only/NODATA answers. Process the completion flag independently of whether the record is one of the requested types.
src/libraries/System.Net.NameResolution/src/System/Net/DnsSdRecordParsing.cs:34 TryParseAddressvalidates only the RDATA length, not that the length matchesrecord.Type. A malformed A record with 16 bytes is therefore returned as an IPv6AddressRecord, and a malformed AAAA record with 4 bytes is returned as IPv4. The managed parser rejects both cases (TryParseARecord/TryParseAAAARecord), so preserve that contract here by requiring A+4 or AAAA+16 before constructing the address (and add the corresponding parser tests).
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
This was referenced Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
replaces #131934, addresses all feedback on the original PR