From b3b04d0de2793b9964efc8eac9b95d559d02d679 Mon Sep 17 00:00:00 2001
From: Scott Mitchell <7562868+Scottmitch@users.noreply.github.com>
Date: Tue, 7 Aug 2018 23:14:18 -0700
Subject: [PATCH] DnsNameResolver hangs if search domain results in invalid
 hostname (#8180)

Motivation:
DnsNameResolver manages search domains and will retry the request with the different search domains provided to it. However if the query results in an invalid hostname, the Future corresponding to the resolve request will never be completed.

Modifications:
- If a resolve attempt results in an invalid hostname and the query isn't issued we should fail the associated promise

Result:
No more hang from DnsNameResolver if search domain results in invalid hostname.
---
 .../netty/resolver/dns/DnsResolveContext.java | 21 ++++++-------
 .../resolver/dns/DnsNameResolverTest.java     | 30 +++++++++++++++++++
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
index bc63e710e1..dbb1ab6bbf 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
@@ -749,7 +749,7 @@ abstract class DnsResolveContext<T> {
 
         final DnsQuestion cnameQuestion;
         try {
-            cnameQuestion = newQuestion(cname, question.type());
+            cnameQuestion = new DefaultDnsQuestion(cname, question.type(), dnsClass);
         } catch (Throwable cause) {
             queryLifecycleObserver.queryFailed(cause);
             PlatformDependent.throwException(cause);
@@ -760,23 +760,20 @@ abstract class DnsResolveContext<T> {
 
     private boolean query(String hostname, DnsRecordType type, DnsServerAddressStream dnsServerAddressStream,
                           Promise<List<T>> promise) {
-        final DnsQuestion question = newQuestion(hostname, type);
-        if (question == null) {
+        final DnsQuestion question;
+        try {
+            question = new DefaultDnsQuestion(hostname, type, dnsClass);
+        } catch (Throwable cause) {
+            // Assume a single failure means that queries will succeed. If the hostname is invalid for one type
+            // there is no case where it is known to be valid for another type.
+            promise.tryFailure(new IllegalArgumentException("Unable to create DNS Question for: [" + hostname + ", " +
+                    type + "]", cause));
             return false;
         }
         query(dnsServerAddressStream, 0, question, promise, null);
         return true;
     }
 
-    private DnsQuestion newQuestion(String hostname, DnsRecordType type) {
-        try {
-            return new DefaultDnsQuestion(hostname, type, dnsClass);
-        } catch (IllegalArgumentException e) {
-            // java.net.IDN.toASCII(...) may throw an IllegalArgumentException if it fails to parse the hostname
-            return null;
-        }
-    }
-
     /**
      * Holds the closed DNS Servers for a domain.
      */
diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
index dcda33c8d5..36fcdf4842 100644
--- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
+++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
@@ -58,7 +58,9 @@ import org.apache.directory.server.dns.store.RecordStore;
 import org.hamcrest.Matchers;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import java.io.IOException;
 import java.net.InetAddress;
@@ -88,6 +90,7 @@ import static io.netty.handler.codec.dns.DnsRecordType.AAAA;
 import static io.netty.handler.codec.dns.DnsRecordType.CNAME;
 import static io.netty.resolver.dns.DefaultDnsServerAddressStreamProvider.DNS_PORT;
 import static io.netty.resolver.dns.DnsServerAddresses.sequential;
+import static java.util.Collections.singletonList;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
@@ -298,6 +301,9 @@ public class DnsNameResolverTest {
     private static final TestDnsServer dnsServer = new TestDnsServer(DOMAINS_ALL);
     private static final EventLoopGroup group = new NioEventLoopGroup(1);
 
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
     private static DnsNameResolverBuilder newResolver(boolean decodeToUnicode) {
         return newResolver(decodeToUnicode, null);
     }
@@ -1682,4 +1688,28 @@ public class DnsNameResolverTest {
             }
         }
     }
+
+    @Test
+    public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() {
+        expectedException.expect(UnknownHostException.class);
+        testSearchDomainQueryFailureCompletes(ResolvedAddressTypes.IPV4_ONLY);
+    }
+
+    @Test
+    public void testSearchDomainQueryFailureForMultipleAddressTypeCompletes() {
+        expectedException.expect(UnknownHostException.class);
+        testSearchDomainQueryFailureCompletes(ResolvedAddressTypes.IPV4_PREFERRED);
+    }
+
+    private void testSearchDomainQueryFailureCompletes(ResolvedAddressTypes types) {
+        DnsNameResolver resolver = newResolver()
+                .resolvedAddressTypes(types)
+                .ndots(1)
+                .searchDomains(singletonList(".")).build();
+        try {
+            resolver.resolve("invalid.com").syncUninterruptibly();
+        } finally {
+            resolver.close();
+        }
+    }
 }