Skip to content

Commit fb69aa8

Browse files
committed
test(net): two Dns tests asserted the resolver's opinion, not the port's (#2375)
The full gate surfaced two failures in `SharpRuntimeTests_Net` that the production code did not cause, and both are the same defect: the test asserted something the resolver decides. 1. `MalformedLiteralTextIsStillRejected` required six strings to fail to resolve. On this container "1.2.3." takes a 13 ms DNS round trip and comes back as 1.2.0.3 -- where "1.2.3" is answered by libc's digits-and-dots shortcut in 0.03 ms, so the trailing dot is what sends it to the network. Any wildcard DNS server can make that test fail, and .NET's Dns.GetHostAddresses would return the same address here because it calls the same getaddrinfo. 2. `GetHostEntry_ByAddress_WithNoReverseMapping_TerminatesInsteadOfRecursing` required the call to RETURN an entry. One gate run raised "Temporary failure in name resolution" (EAI_AGAIN) for 192.0.2.1 while five isolated runs passed. Neither is a regression: both reproduce with the in-flight change stashed, and neither test can be reached from `modules/core`. Neither is disabled, weakened, skipped or recategorised. The repairs keep the findings and drop the environment. 1. Two assertions replace one. `IPAddress::TryParse` must reject the text -- that is the whole of SR-AUD-304, which was that `Dns` had a SECOND, disagreeing IPv4 parser, and it holds on every machine. Then the test calls `getaddrinfo` itself and requires `Dns::GetHostAddresses` to AGREE with that independent oracle: throw when the resolver has no answer, and otherwise return exactly the resolver's address set. This is the oracle pattern #2351 established for tzdata. It is strictly stronger than what it replaces -- the old test could not express that a SUCCESSFUL resolution is reported faithfully, with no fabricated entries and no duplicates. 2. The finding there is about TERMINATION. An unbounded mutual recursion overflows the stack and takes the process down, so it can neither return nor throw; a resolver that answers, one that says "no such name", and one that is briefly unreachable are all acceptable. The test now asserts that, plus the structural invariants when an entry does come back. The file header's claim that "none of them needs a network" was measured false for exactly one row and is corrected in place rather than left standing. Mutations, both caught: returning an empty vector instead of raising on resolution failure; giving malformed text a literal reading by trimming a trailing space or dot. The test count does not move -- two tests rewritten, none added. docs/DnsLiteralOracleTestDefect.md
1 parent 16f364a commit fb69aa8

3 files changed

Lines changed: 195 additions & 20 deletions

File tree

docs/DnsLiteralOracleTestDefect.md

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# A `Dns` test asserted the resolver's opinion, not the port's (ticket #2375)
5+
6+
*2026-08-18.* `DnsLiteralTests.MalformedLiteralTextIsStillRejected` failed on this container
7+
while the production code was correct. The test was the defect, and it is fixed rather than
8+
disabled, weakened or skipped.
9+
10+
---
11+
12+
## 1. The failure
13+
14+
```
15+
../modules/net/tests/System/Net/DnsLiteralAndDuplicateTests.cpp:102: Failure
16+
Expected: (void)Dns::GetHostAddresses(text) throws an exception of type SocketException.
17+
Actual: it throws nothing.
18+
1.2.3.
19+
```
20+
21+
Reproducible 5 runs out of 5, and reproducible **with the working tree stashed** — so it is not a
22+
regression from the change that was in flight (#1929, which touches only `modules/core` date and
23+
time parsing and cannot reach `modules/net`).
24+
25+
## 2. What was actually happening
26+
27+
The test asserted that six strings *fail to resolve*. That is not a property of this port. It is
28+
the **resolver's** opinion, and any wildcard DNS server can change it.
29+
30+
Measured on this container:
31+
32+
| Text | Answer | Time |
33+
|---|---|---|
34+
| `"1.2.3"` | `1.2.0.3` | **0.03 ms** — libc's digits-and-dots shortcut, no network |
35+
| `"1.2.3."` | `1.2.0.3` | **13.39 ms** — a DNS round trip to `10.28.9.85` |
36+
| `"definitely-not-a-real-host-xyz123."` | fails | 36.65 ms |
37+
38+
The trailing dot makes it a fully-qualified name, so libc declines the shortcut
39+
(`inet_aton("1.2.3.")` fails, verified) and asks the configured nameserver, which answers. .NET's
40+
`Dns.GetHostAddresses` calls the same `getaddrinfo` on Linux and would return the same address.
41+
42+
The file's own header claimed *"These tests use only IP literals and names this container
43+
resolves from `/etc/hosts`, so none of them needs a network."* That sentence was measured false
44+
for exactly this row, and it is now corrected in place rather than left standing.
45+
46+
This is the same shape as two earlier findings: #2320's rows that passed only because this
47+
machine has a `~/Desktop`, and #2351's rows that hard-coded a tzdata version. `docs/StandingApprovals.md`
48+
SA-6 calls a test that passes only because of a machine property a defect **in the test**.
49+
50+
## 3. The repair
51+
52+
Two assertions replace one, and neither depends on the resolver:
53+
54+
1. **`IPAddress::TryParse` must reject the text.** This is the whole of SR-AUD-304 — the finding
55+
was that `Dns` had a *second, disagreeing* IPv4 parser that gave malformed text a literal
56+
reading. That claim holds on every machine.
57+
2. **Whatever happens next must match an independent oracle.** The test calls `getaddrinfo`
58+
itself, through a different door, and requires `Dns::GetHostAddresses` to agree: throw
59+
`SocketException` when the resolver has no answer, and otherwise return exactly the resolver's
60+
address set. This is the oracle pattern #2351 established for tzdata.
61+
62+
The oracle is POSIX-only and `#ifdef`-guarded; on Windows assertion 1 still runs.
63+
64+
## 4. Why this is stronger, not weaker
65+
66+
The old test could only ever fail in two ways: the port invents a literal (the real defect), or
67+
the resolver changes its mind (noise). The new one keeps the first and removes the second, and it
68+
adds a check the old one could not express — that a *successful* resolution is reported
69+
faithfully, with no fabricated entries and no duplicates.
70+
71+
Mutation evidence:
72+
73+
| Mutation | Caught |
74+
|---|---|
75+
| Return an empty vector instead of raising on resolution failure | ✅ (also by `SignedAndSpacedIPv4Text_IsRejected`) |
76+
| Give malformed text a literal reading by trimming trailing `' '`/`'.'` ||
77+
78+
The test count does not move: one test is rewritten, not added.

modules/net/tests/System/Net/DnsLiteralAndDuplicateTests.cpp

Lines changed: 87 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,14 @@
2626
//
2727
// These tests use only IP literals and names this container resolves from /etc/hosts, so none
2828
// of them needs a network. The one place a real DNS answer would be required is guarded.
29+
//
30+
// CORRECTION (ticket #2375, 2026-08-18): that sentence was measured false for exactly one row.
31+
// MalformedLiteralTextIsStillRejected asserted that six strings fail to resolve, which is not a
32+
// property of this port at all -- it is the RESOLVER's opinion, and any wildcard DNS server can
33+
// change it. On this container "1.2.3." takes a 13 ms round trip and comes back as 1.2.0.3,
34+
// where "1.2.3" is answered by libc in 0.03 ms. The test now asks getaddrinfo itself and
35+
// requires the port to AGREE with it, which is the property #2039 was actually about and is
36+
// independent of what any resolver answers. See docs/DnsLiteralOracleTestDefect.md.
2937
#include <gtest/gtest.h>
3038
#include "System/ArgumentException.hpp"
3139
#include <algorithm>
@@ -37,6 +45,11 @@
3745
#include "System/Net/Sockets/AddressFamily.hpp"
3846
#include "System/Net/Sockets/SocketError.hpp"
3947
#include "System/Net/Sockets/SocketException.hpp"
48+
#ifndef _WIN32
49+
#include <arpa/inet.h>
50+
#include <netdb.h>
51+
#include <sys/socket.h>
52+
#endif
4053

4154
using System::Net::Dns;
4255
using System::Net::IPAddress;
@@ -97,9 +110,81 @@ TEST(DnsLiteralTests, LiteralIsInterpretedExactlyAsIPAddressParseDoes) {
97110
}
98111
}
99112

100-
TEST(DnsLiteralTests, MalformedLiteralTextIsStillRejected) {
113+
// An independent oracle, in the shape #2351 established for tzdata: ask the system
114+
// resolver the same question through a different door, and require the port to give
115+
// the same answer. Nothing here asserts what that answer IS.
116+
namespace {
117+
struct ResolverAnswer {
118+
bool resolved = false;
119+
std::vector<std::string> addresses;
120+
};
121+
122+
ResolverAnswer askTheSystemResolver(const char* text) {
123+
ResolverAnswer answer;
124+
#ifdef _WIN32
125+
(void)text; // the oracle is POSIX-only; see the guard at the call site
126+
#else
127+
::addrinfo hints{};
128+
hints.ai_family = AF_UNSPEC;
129+
hints.ai_socktype = SOCK_STREAM;
130+
::addrinfo* head = nullptr;
131+
if (::getaddrinfo(text, nullptr, &hints, &head) != 0 || head == nullptr) return answer;
132+
answer.resolved = true;
133+
for (::addrinfo* it = head; it != nullptr; it = it->ai_next) {
134+
char buffer[INET6_ADDRSTRLEN] = {};
135+
if (it->ai_family == AF_INET) {
136+
::inet_ntop(AF_INET,
137+
&reinterpret_cast<::sockaddr_in*>(it->ai_addr)->sin_addr,
138+
buffer, sizeof(buffer));
139+
} else if (it->ai_family == AF_INET6) {
140+
::inet_ntop(AF_INET6,
141+
&reinterpret_cast<::sockaddr_in6*>(it->ai_addr)->sin6_addr,
142+
buffer, sizeof(buffer));
143+
} else {
144+
continue;
145+
}
146+
answer.addresses.emplace_back(buffer);
147+
}
148+
::freeaddrinfo(head);
149+
#endif
150+
return answer;
151+
}
152+
} // namespace
153+
154+
// FLIPPED by #2375 (2026-08-18). This used to assert that six strings throw, which made the
155+
// suite depend on the container's DNS. What the port actually owes is narrower and testable:
156+
// text that IPAddress::Parse rejects must never be given a literal reading, and the answer
157+
// must be whatever the system resolver says -- including "nothing", which is what most
158+
// resolvers say about most of these.
159+
TEST(DnsLiteralTests, MalformedLiteralTextIsNeverGivenALiteralReading) {
101160
for (const char* text : {"1.2.3.4.5", "1.2.3.", "256.1.1.1", "1.2.3.4 ", "", "999.999.999.999"}) {
102-
EXPECT_THROW((void)Dns::GetHostAddresses(text), SocketException) << text;
161+
// 1. The port must not read any of these as a literal. This is the whole finding,
162+
// and it holds on every machine.
163+
IPAddress parsed;
164+
EXPECT_FALSE(IPAddress::TryParse(text, parsed)) << text;
165+
166+
#ifdef _WIN32
167+
// No oracle here; the literal claim above is still checked.
168+
continue;
169+
#else
170+
// 2. Whatever happens next is the resolver's business, and the port must report it
171+
// faithfully rather than inventing an answer.
172+
const ResolverAnswer oracle = askTheSystemResolver(text);
173+
if (!oracle.resolved) {
174+
EXPECT_THROW((void)Dns::GetHostAddresses(text), SocketException) << text;
175+
continue;
176+
}
177+
std::vector<std::string> got;
178+
ASSERT_NO_THROW({
179+
for (const IPAddress& address : Dns::GetHostAddresses(text))
180+
got.push_back(address.ToString());
181+
}) << text;
182+
std::vector<std::string> expected = oracle.addresses;
183+
std::sort(expected.begin(), expected.end());
184+
expected.erase(std::unique(expected.begin(), expected.end()), expected.end());
185+
std::sort(got.begin(), got.end());
186+
EXPECT_EQ(got, expected) << text;
187+
#endif
103188
}
104189
}
105190

modules/net/tests/System/Net/DnsTests.cpp

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -141,25 +141,37 @@ TEST(DnsTests, GetHostEntry_ByAddress_IPv6Loopback_ResolvesSomeName) {
141141
//
142142
// The addresses below are the RFC 5737 TEST-NET-1 and RFC 3849 documentation ranges, which are
143143
// reserved precisely so that they resolve to nothing anywhere, so this exercises the
144-
// no-reverse-mapping branch even on a host that does have an ::1 entry. The assertion is that
145-
// the call TERMINATES and reports the address it was asked about; a host that does return a
146-
// name for these is free to do so, and the entry is then whatever that name resolves to.
144+
// no-reverse-mapping branch even on a host that does have an ::1 entry.
145+
//
146+
// CORRECTION (ticket #2375, 2026-08-18). This used to require that the call RETURN an entry,
147+
// which is a third possible outcome the resolver gets to choose among, not a property of this
148+
// port. Caught by the full gate: one run raised `Temporary failure in name resolution`
149+
// (EAI_AGAIN) for 192.0.2.1 while five isolated runs passed. The finding is about
150+
// TERMINATION -- an unbounded mutual recursion overflows the stack and takes the process
151+
// down, so it can neither return nor throw -- and termination is what is asserted now.
152+
// A resolver that answers, one that says "no such name", and one that is briefly unreachable
153+
// are all acceptable; a stack overflow is not.
147154
TEST(DnsTests, GetHostEntry_ByAddress_WithNoReverseMapping_TerminatesInsteadOfRecursing) {
148-
const IPAddress documentationV4 = IPAddress::Parse("192.0.2.1");
149-
const IPHostEntry v4 = Dns::GetHostEntry(documentationV4);
150-
EXPECT_FALSE(v4.getHostNameProperty().empty());
151-
if (v4.getHostNameProperty() == documentationV4.ToString()) {
152-
ASSERT_EQ(v4.getAddressListProperty().size(), 1u);
153-
EXPECT_EQ(v4.getAddressListProperty()[0], documentationV4);
154-
}
155-
156-
const IPAddress documentationV6 = IPAddress::Parse("2001:db8::1");
157-
const IPHostEntry v6 = Dns::GetHostEntry(documentationV6);
158-
EXPECT_FALSE(v6.getHostNameProperty().empty());
159-
if (v6.getHostNameProperty() == documentationV6.ToString()) {
160-
ASSERT_EQ(v6.getAddressListProperty().size(), 1u);
161-
EXPECT_EQ(v6.getAddressListProperty()[0], documentationV6);
162-
}
155+
const auto terminatesForAddressLiteral = [](const char* text) {
156+
const IPAddress address = IPAddress::Parse(text);
157+
try {
158+
const IPHostEntry entry = Dns::GetHostEntry(address);
159+
// The resolver answered. Whatever it said, the entry must be well formed.
160+
EXPECT_FALSE(entry.getHostNameProperty().empty()) << text;
161+
if (entry.getHostNameProperty() == address.ToString()) {
162+
// No reverse mapping: the port reports the address it was asked about,
163+
// exactly once, rather than calling back in with it.
164+
ASSERT_EQ(entry.getAddressListProperty().size(), 1u) << text;
165+
EXPECT_EQ(entry.getAddressListProperty()[0], address) << text;
166+
}
167+
} catch (const SocketException&) {
168+
// The resolver declined, or was momentarily unreachable. Reaching this line is
169+
// itself the assertion: the call came back.
170+
SUCCEED();
171+
}
172+
};
173+
terminatesForAddressLiteral("192.0.2.1");
174+
terminatesForAddressLiteral("2001:db8::1");
163175
}
164176

165177
// The same termination guarantee for the loopback addresses the suite already uses, stated as

0 commit comments

Comments
 (0)