Implement DnsResolver for Linux#129846
Conversation
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR adds a Unix/Linux implementation of the new DnsResolver / Dns.Resolve* DNS query APIs in System.Net.NameResolution, including a managed stub resolver (UDP with TCP fallback) and /etc/resolv.conf server discovery, plus new functional and PAL tests to validate parsing and resolver behavior.
Changes:
- Add managed DNS resolver PAL for Unix: build DNS wire-format queries, send over UDP with TCP fallback, parse responses into typed record results (incl. negative-cache TTL).
- Add
/etc/resolv.confparsing to discover system DNS servers when no custom servers are configured. - Add new test infrastructure (loopback DNS server + response builder) and functional/PAL tests for deterministic resolver behavior.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.NameResolution/tests/PalTests/System.Net.NameResolution.Pal.Tests.csproj | Includes ResolvConf production source + new PAL tests on Unix. |
| src/libraries/System.Net.NameResolution/tests/PalTests/ResolvConfTests.cs | Unit tests for parsing nameserver directives and edge cases. |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj | Adds new functional test sources for resolver + loopback server. |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/LoopbackDnsServer.cs | Minimal in-process DNS server used by resolver loopback tests. |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResponseBuilder.cs | Helper for constructing raw DNS responses for deterministic tests. |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs | API argument validation + (Windows-only) network coverage for new APIs. |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs | Deterministic resolver behavior tests using the loopback DNS server (UDP/TCP, parsing, TTL, metrics). |
| src/libraries/System.Net.NameResolution/src/System/Net/ResolvConf.cs | Reads/parses /etc/resolv.conf to get system DNS servers. |
| src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs | Extends telemetry answer-shaping to support string[] answers. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsWireEnums.cs | Adds internal enums for DNS wire constants (TYPE/CLASS/OPCODE/flags). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResult.cs | Adds DnsResult<T> envelope (ResponseCode, Records, NegativeCacheTtl). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResponseCode.cs | Adds DnsResponseCode public enum. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.Windows.cs | Windows PAL implementation (per request, Windows parts not reviewed here). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.Unsupported.cs | PNSE stubs for unsupported targets (e.g., WASI). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.Managed.cs | Unix/Linux managed resolver: query engine, UDP/TCP fallback, parsing. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverOptions.cs | Public options type (Servers list). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs | Public resolver implementation + telemetry integration + PTR arpa-name builder. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsRecords.cs | Public record structs (Address/Srv/Mx/Txt/CName/Ptr/Ns). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsRecordParsing.cs | Typed RDATA parsing helpers over decoded records. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsMessageWriter.cs | Writes DNS query messages (header + questions). |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsMessageReader.cs | Reads DNS header/questions/records from response buffers. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsMessageHeader.cs | Encodes/decodes DNS header flags + counters. |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsEncodedName.cs | DNS name encoding/decoding with compression-pointer support and IDN handling. |
| src/libraries/System.Net.NameResolution/src/System/Net/Dns.Resolve.cs | Adds Dns.Resolve* static APIs backed by a shared DnsResolver. |
| src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs | Makes Dns partial to enable Dns.Resolve.cs partial definition. |
| src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj | Wires in new sources, Unix managed resolver sources, and Unix-only project refs needed by implementation. |
| src/libraries/System.Net.NameResolution/src/Resources/Strings.resx | Adds SR strings for new exceptions and validation messages. |
| src/libraries/System.Net.NameResolution/ref/System.Net.NameResolution.cs | Adds new public surface area to the reference assembly contract. |
| src/libraries/Common/src/Interop/Windows/Interop.Libraries.cs | Windows interop library constant for dnsapi (per request, Windows parts not reviewed here). |
| src/libraries/Common/src/Interop/Windows/Dnsapi/Interop.DnsTypes.cs | Windows Dnsapi structs/constants (per request, Windows parts not reviewed here). |
| src/libraries/Common/src/Interop/Windows/Dnsapi/Interop.DnsApi.cs | Windows Dnsapi P/Invokes/constants (per request, Windows parts not reviewed here). |
MihaZupan
left a comment
There was a problem hiding this comment.
Just an initial glance, not a full review
Implements the DnsResolver PAL for the unix TFM as a managed stub resolver. It builds and parses DNS wire messages and talks to the configured servers over UDP (with TCP fallback on truncation) using System.Net.Sockets. When no servers are configured, the system servers from /etc/resolv.conf are used, falling back to loopback. - Add internal wire primitives: message header, reader, writer, encoded-name (with IDN/ACE support), and per-type record parsers. - Add DnsResolverPal.Managed.cs query engine with shared sync/async paths; the sync path uses blocking socket calls and returns a completed Task, preserving the task.IsCompleted invariant. - Add ResolvConf.cs nameserver discovery plus parser unit tests. - Wire the new sources into the unix ItemGroup; reference the System.Net.Sockets reference assembly to break the project cycle (Sockets references NameResolution); the implementation resolves from the shared framework at runtime. - Generalize the loopback tests to run on Linux by binding an ephemeral port on non-Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds System.Net.NameResolution.Unit.Tests covering the internal DNS wire-format types (DnsEncodedName, DnsMessageHeader/Reader/Writer, and typed RDATA parsing). The project links the production parsing sources so the internal types can be exercised directly. Also adds DnsEncodedName.GetFormattedLength() to expose the decoded dotted-string length (already computed internally) for buffer sizing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The managed Unix DNS stub resolver used System.Net.Sockets.Socket for UDP/TCP queries. Because System.Net.Sockets already depends on System.Net.NameResolution (Socket.Connect(host, port) resolves names through Dns), the emitted NameResolution.dll carried a real metadata reference back to Sockets, closing a dependency cycle that the shared-framework VerifyClosure task rejects. This built locally but failed the CoreCLR/Mono runtime-pack build legs on CI. Access Socket through a new internal DnsSocket reflection wrapper so NameResolution no longer statically references System.Net.Sockets. The wrapper binds the required Socket members to delegates so exceptions (e.g. SocketException) propagate directly instead of being wrapped in TargetInvocationException, and a DynamicDependency attribute preserves the members for trimming/AOT. SocketException, SocketError and AddressFamily live in System.Net.Primitives and continue to be used directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs:133
- Many tests in this file are now only gated by PlatformDetection.IsNotMobile, but the FunctionalTests project targets browser and wasi as well. On those TFMs, these loopback/socket-based tests are likely to run and fail (the first test already includes IsNotBrowser/IsNotWasi). Consider adding IsNotBrowser and IsNotWasi to the other ConditionalTheory/ConditionalFact attributes for consistency and to avoid running unsupported tests on those platforms.
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMobile))]
[InlineData(false)]
[InlineData(true)]
public async Task ResolveAddresses_IPv4Only_ReturnsOnlyV4(bool async)
{
…uccess - Gate the loopback DNS resolver tests on IsNotBrowser and IsNotWasi in addition to IsNotMobile. The functional test project multi-targets -browser and -wasi where DnsResolverPal is Unsupported, so these socket-based tests would otherwise run and fail there. - Assert success of DnsMessageReader.TryCreate/TryReadQuestion/TryReadRecord in the DnsRecordTypeTests GetAnswerRecord helper so malformed input surfaces clearly instead of failing in less obvious ways. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if (validation == ResponseValidation.Retry) | ||
| { | ||
| lastException = validationError; | ||
| continue; | ||
| } |
| Span<byte> nameBuffer = stackalloc byte[DnsEncodedName.MaxEncodedLength]; | ||
| OperationStatus status = DnsEncodedName.TryEncode(name, nameBuffer, out DnsEncodedName encodedName, out _); | ||
| if (status == OperationStatus.InvalidData) | ||
| { | ||
| throw new ArgumentException(SR.Format(SR.net_invalid_dns_name, name), nameof(name)); | ||
| } | ||
| Debug.Assert(status == OperationStatus.Done); |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "0ea17315d4c07c7cda8c16912c37e0a0b628eb62",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "75fa507f92d7560a3ec401ad35eee1897125f032",
"last_reviewed_commit": "0ea17315d4c07c7cda8c16912c37e0a0b628eb62",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "75fa507f92d7560a3ec401ad35eee1897125f032",
"last_recorded_worker_run_id": "29684010668",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "0ea17315d4c07c7cda8c16912c37e0a0b628eb62",
"review_id": 4730637858
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Well justified. This replaces the Unix DnsResolverPal.Unsupported stub with a real managed stub-resolver, giving the new DnsResolver/Dns.Resolve* API (introduced in #129845) a working Linux/Unix implementation. Without it those APIs would throw PlatformNotSupported everywhere except Windows.
Approach: Sound overall. A from-scratch managed DNS message writer/reader/parser plus a /etc/resolv.conf reader and a reflection-based DnsSocket shim (to avoid the Sockets↔NameResolution shared-framework cycle) is a reasonable design, and the wire parsing is carefully bounds-checked (compression-pointer cycle protection, RDLENGTH validation, hop limits). The main risks are (1) a public API surface that does not appear to be tied to an approved API proposal, and (2) several sync-vs-async behavioral divergences that make the two code paths hard to keep in lockstep.
Summary: AsyncWaitHandle, negative-cache TTL selection, cancellation token identity, unvalidated-name RDLENGTH) have already been flagged by existing reviewers; I have not duplicated those.
Detailed Findings
⚠️ Correctness — Non-ASCII response labels decode to garbage chars
See the inline comment on DnsEncodedName.cs. Response names are parsed with validateContent: false, so labels may contain non-ASCII octets (legal per RFC 1035). TryDecodeAscii discards the Ascii.ToUtf16 OperationStatus while still advancing charsWritten by the full label length, which can surface uninitialized/garbage chars in ToString()/TryDecode output.
⚠️ Consistency — Sync vs. async per-attempt timeout diverges
See the inline comment on DnsResolverPal.Managed.cs. The async path converts a per-attempt timeout into TimeoutException (which the retry loop swallows), while the sync UDP path relies on Socket.ReceiveTimeout/SendTimeout and surfaces SocketException(TimedOut). This yields a different observable exception from the public API depending on whether the caller used the sync or async entry point, and a different lastException classification within the retry loop.
⚠️ Cross-cutting — Duplicated sync/async plumbing is fragile
The query engine, TCP framing, connect-with-timeout, and send/receive-exact loops are each implemented twice (sync + async). Beyond the timeout divergence above, this duplication is where several already-reported issues live (undisposed AsyncWaitHandle in ConnectWithTimeout/SendTcpQuerySync, differing cancellation semantics). It is worth a maintainer deciding whether the sync paths should instead wrap the async implementation (as several other System.Net components do) to eliminate the divergence class entirely. Advisory / follow-up.
✅ Wire parsing safety
Compression-pointer handling only permits strictly-backward jumps with a 16-hop cap, ValidateName enforces the 255-byte wire and 253-char dotted limits, TryReadRecord validates RDLENGTH against the buffer, and DnsRecordParsing re-parses embedded names against a message slice bounded by DataOffset + Data.Length. This is the highest-risk area and it is handled carefully. (Note: DnsLabelEnumerator.MoveNext relies on prior ValidateName success and uses only Debug.Assert for pointer safety — correct given the documented precondition, but keep that invariant in mind if the enumerator ever becomes reachable on an unvalidated buffer.)
✅ Test coverage
The new unit tests (DnsEncodedNameTests, DnsMessageHeaderTests, DnsMessageReaderTests, DnsMessageWriterTests, DnsRecordTypeTests, ResolvConfTests) exercise encoding/decoding round-trips, IDN/ACE handling, compression pointers, malformed inputs, and resolv.conf parsing edge cases. Good breadth for the parsing core; the reflection-based DnsSocket and the retry/timeout engine are only covered indirectly via the loopback functional test.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 192.1 AIC · ⌖ 11.6 AIC · ⊞ 10K
| return false; | ||
| } | ||
|
|
||
| Ascii.ToUtf16(label, destination.Slice(charsWritten, label.Length), out _); |
There was a problem hiding this comment.
Ascii.ToUtf16 is discarded here, but charsWritten is unconditionally advanced by label.Length. Response names are parsed with ValidateName(..., validateContent: false) (see TryParse), so labels are only structurally validated — a label may legally contain bytes with the high bit set (RFC 1035 permits arbitrary octets in labels). When such a byte is present, Ascii.ToUtf16 stops at the first non-ASCII byte, returns OperationStatus.InvalidData, and leaves the remaining chars in the destination slice uninitialized, yet charsWritten still counts the full label length. The resulting ToString() / TryDecode output can then contain garbage char values. Consider checking the OperationStatus (and either failing or substituting/escaping non-LDH bytes) rather than assuming the label is valid ASCII.
|
|
||
| socket.Connect(server); | ||
| socket.Send(query.Span); | ||
| return socket.Receive(responseBuffer); |
There was a problem hiding this comment.
CancellationTokenSource.CancelAfter, which surfaces as OperationCanceledException and is translated to a TimeoutException (line 435) that the loop swallows and retries against the next server. In the sync UDP path the timeout is enforced via Socket.ReceiveTimeout/SendTimeout, which instead throws SocketException(SocketError.TimedOut) — a different exception type and, more importantly, a different retry/last-exception classification than the async path. The end result is that a timing-out server yields a TimeoutException in async but a SocketException in sync from the public API. Consider normalizing so both code paths report a consistent exception on per-attempt timeout.
| [DynamicDependency(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties, | ||
| "System.Net.Sockets.Socket", "System.Net.Sockets")] | ||
| private static SocketReflection CreateReflection() | ||
| { | ||
| Type socketType = Type.GetType("System.Net.Sockets.Socket, System.Net.Sockets", throwOnError: true)!; | ||
| Type socketTypeEnum = Type.GetType("System.Net.Sockets.SocketType, System.Net.Sockets", throwOnError: true)!; | ||
| Type protocolTypeEnum = Type.GetType("System.Net.Sockets.ProtocolType, System.Net.Sockets", throwOnError: true)!; | ||
|
|
||
| return new SocketReflection | ||
| { | ||
| SocketTypeDgram = Enum.Parse(socketTypeEnum, "Dgram"), | ||
| SocketTypeStream = Enum.Parse(socketTypeEnum, "Stream"), | ||
| ProtocolTypeUdp = Enum.Parse(protocolTypeEnum, "Udp"), | ||
| ProtocolTypeTcp = Enum.Parse(protocolTypeEnum, "Tcp"), | ||
| Constructor = socketType.GetConstructor(new[] { typeof(AddressFamily), socketTypeEnum, protocolTypeEnum })!, | ||
| ConnectAsyncMethod = socketType.GetMethod("ConnectAsync", new[] { typeof(EndPoint), typeof(CancellationToken) })!, | ||
| SendAsyncMethod = socketType.GetMethod("SendAsync", new[] { typeof(ReadOnlyMemory<byte>), typeof(CancellationToken) })!, | ||
| ReceiveAsyncMethod = socketType.GetMethod("ReceiveAsync", new[] { typeof(Memory<byte>), typeof(CancellationToken) })!, | ||
| ConnectMethod = socketType.GetMethod("Connect", new[] { typeof(EndPoint) })!, | ||
| SendMethod = socketType.GetMethod("Send", new[] { typeof(ReadOnlySpan<byte>) })!, | ||
| ReceiveMethod = socketType.GetMethod("Receive", new[] { typeof(Span<byte>) })!, | ||
| BeginConnectMethod = socketType.GetMethod("BeginConnect", new[] { typeof(EndPoint), typeof(AsyncCallback), typeof(object) })!, | ||
| EndConnectMethod = socketType.GetMethod("EndConnect", new[] { typeof(IAsyncResult) })!, | ||
| DisposeMethod = socketType.GetMethod("Dispose", Type.EmptyTypes)!, | ||
| SetSendTimeoutMethod = socketType.GetProperty("SendTimeout")!.GetSetMethod()!, | ||
| SetReceiveTimeoutMethod = socketType.GetProperty("ReceiveTimeout")!.GetSetMethod()!, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Can we use [UnsafeAccessor] for at least some of the APIs here? Also you are doing Type.GetType on a constant string; the trimmer should be able to figure this out. Does it warn if you remove the [DynamicDependency]?
Follow up on #129845