Copilot commented on code in PR #8820:
URL: https://github.com/apache/hadoop/pull/8820#discussion_r4269022429
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestDNS.java:
##########
@@ -222,6 +226,52 @@ public void testReverseDnsIPv6WithInvalidServer() throws
Exception {
.isInstanceOf(NamingException.class);
}
+ /**
+ * Test that the reverse lookup fails with a NamingException, which the
+ * callers handle, when the DNS server answers without a PTR record.
+ */
+ @Test
+ public void testReverseDnsWithoutPtrRecord() throws Exception {
+ try (DatagramSocket server = new DatagramSocket(0,
InetAddress.getByName("127.0.0.1"))) {
+ Thread responder = new Thread(() -> answerWithoutRecords(server));
+ responder.setDaemon(true);
+ responder.start();
+ String ns = "127.0.0.1:" + server.getLocalPort();
+ for (String ip : new String[] {"192.0.2.1", "2001:db8::1"}) {
+ assertThatThrownBy(() -> DNS.reverseDns(InetAddress.getByName(ip), ns))
+ .isInstanceOf(NameNotFoundException.class)
+ .hasMessageContaining("No PTR record");
+ }
+ }
+ }
+
+ /**
+ * Answers every DNS query with NOERROR and no records, until the socket
+ * is closed.
+ */
+ private static void answerWithoutRecords(DatagramSocket server) {
+ byte[] buf = new byte[512];
+ try {
+ while (true) {
+ DatagramPacket query = new DatagramPacket(buf, buf.length);
+ server.receive(query);
+ // Keep the header and the question, drop anything after the question.
+ int end = 12;
+ while (buf[end] != 0) {
+ end += buf[end] + 1;
+ }
+ end += 5; // the root label, QTYPE and QCLASS
+ byte[] answer = Arrays.copyOf(buf, end);
Review Comment:
The QNAME parsing loop assumes an uncompressed, well-formed name and doesn’t
check bounds. If the client query uses name compression (0xC0 pointers) or if a
malformed/long packet is received, this can throw
`ArrayIndexOutOfBoundsException` and silently kill the responder thread,
potentially making the test flaky/hang. A more robust approach is to (a) parse
labels with bounds checks against `query.getLength()` and (b) handle
compression pointers (treat 0xC0 as a 2-byte pointer and stop advancing through
labels accordingly), or use a small DNS parsing helper that safely skips the
Question section.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/DNS.java:
##########
@@ -89,7 +91,12 @@ public static String reverseDns(InetAddress hostIp,
@Nullable String ns)
ictx.close();
}
- String hostname = attribute.get("PTR").get().toString();
+ Attribute ptr = attribute.get("PTR");
+ if (ptr == null) {
+ // The server answered, but without a PTR record.
+ throw new NameNotFoundException("No PTR record for " + reverseIP);
+ }
+ String hostname = ptr.get().toString();
Review Comment:
`ptr.get()` can still fail if the PTR attribute exists but contains zero
values (e.g., empty attribute). Consider also treating `ptr.size() == 0` as “no
PTR record” and throwing the same `NameNotFoundException` to avoid leaking a
different exception type from `reverseDns`.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestDNS.java:
##########
@@ -222,6 +226,52 @@ public void testReverseDnsIPv6WithInvalidServer() throws
Exception {
.isInstanceOf(NamingException.class);
}
+ /**
+ * Test that the reverse lookup fails with a NamingException, which the
+ * callers handle, when the DNS server answers without a PTR record.
+ */
+ @Test
+ public void testReverseDnsWithoutPtrRecord() throws Exception {
+ try (DatagramSocket server = new DatagramSocket(0,
InetAddress.getByName("127.0.0.1"))) {
+ Thread responder = new Thread(() -> answerWithoutRecords(server));
+ responder.setDaemon(true);
+ responder.start();
+ String ns = "127.0.0.1:" + server.getLocalPort();
+ for (String ip : new String[] {"192.0.2.1", "2001:db8::1"}) {
+ assertThatThrownBy(() -> DNS.reverseDns(InetAddress.getByName(ip), ns))
+ .isInstanceOf(NameNotFoundException.class)
+ .hasMessageContaining("No PTR record");
+ }
+ }
+ }
+
+ /**
+ * Answers every DNS query with NOERROR and no records, until the socket
+ * is closed.
+ */
+ private static void answerWithoutRecords(DatagramSocket server) {
+ byte[] buf = new byte[512];
+ try {
+ while (true) {
+ DatagramPacket query = new DatagramPacket(buf, buf.length);
+ server.receive(query);
+ // Keep the header and the question, drop anything after the question.
+ int end = 12;
+ while (buf[end] != 0) {
+ end += buf[end] + 1;
+ }
+ end += 5; // the root label, QTYPE and QCLASS
+ byte[] answer = Arrays.copyOf(buf, end);
+ answer[2] = (byte) 0x81; // QR and RD
+ answer[3] = (byte) 0x80; // RA, RCODE 0
+ Arrays.fill(answer, 6, 12, (byte) 0); // no answer, authority or
additional records
+ server.send(new DatagramPacket(answer, end, query.getSocketAddress()));
+ }
+ } catch (IOException closed) {
+ // The test closed the socket.
+ }
Review Comment:
Catching all `IOException` here and treating it as a normal shutdown can
mask unexpected failures (e.g., a send/receive error while the socket is still
open). Consider only swallowing the exception when `server.isClosed()` is true,
and otherwise rethrow (or wrap) to make responder failures visible during the
test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
