From a43c8cb75d47d5de31a257102b944ea36083c38c Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Tue, 7 Aug 2018 19:20:18 -0700
Subject: [PATCH] DnsNameResolver hangs if search domain results in invalid
 hostname

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 bc63e710e14..dbb1ab6bbff 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 @@ private void followCname(DnsQuestion question, String cname, DnsQueryLifecycleOb
 
         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 @@ private void followCname(DnsQuestion question, String cname, DnsQueryLifecycleOb
 
     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 dcda33c8d5a..36fcdf4842d 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.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.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 Set<ResourceRecord> getRecords(QuestionRecord question) {
             }
         }
     }
+
+    @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();
+        }
+    }
 }