[
https://issues.apache.org/jira/browse/HADOOP-20020?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18127005#comment-18127005
]
ASF GitHub Bot commented on HADOOP-20020:
-----------------------------------------
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.
> DNS.reverseDns throws NullPointerException when the answer has no PTR record
> ----------------------------------------------------------------------------
>
> Key: HADOOP-20020
> URL: https://issues.apache.org/jira/browse/HADOOP-20020
> Project: Hadoop Common
> Issue Type: Sub-task
> Reporter: Siyao Meng
> Assignee: Siyao Meng
> Priority: Major
> Labels: pull-request-available
>
> {{DNS.reverseDns}} reads {{attribute.get("PTR").get()}} without a null check.
> When the DNS server answers the reverse query with NOERROR but no PTR record
> (a NODATA answer), JNDI returns no PTR attribute and {{reverseDns}} throws
> {{NullPointerException}}.
> The callers, {{DNS.getHosts}} and {{DNSDomainNameResolver.getHostnameByIP}},
> only catch {{NamingException}}, so the exception escapes instead of falling
> back. Through {{getHostnameByIP}} it reaches
> {{SecurityUtil.getServerPrincipal(String, InetAddress)}} and
> {{ServiceAuthorizationManager.authorize}}. A name that does not exist
> (NXDOMAIN) already fails with {{NameNotFoundException}} and is handled.
> Throw {{NameNotFoundException}} when the answer has no PTR record. This
> applies to IPv4 and IPv6 addresses alike. It was seen with a resolver that
> answers NODATA for the {{ip6.arpa}} name of {{::}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
