Repository: netty/netty
Issue: Dead Loop in DNS resolver
Labels: defect

### Problem
There is a dead loop in DnsResolveContext.onExpectedResponse when there is a loop in the CNAMES
![image](https://user-images.githubusercontent.com/20608396/50428555-8ac8c780-08f3-11e9-908c-5ea0c8cf1d6a.png)

We found this problem because one DNS meet the problem
![image](https://user-images.githubusercontent.com/20608396/50428579-ca8faf00-08f3-11e9-829c-8927ebf72baf.png)

### Steps to reproduce
There is a small chance that the "www.altura.org" would response the loop CNAME. It's easy to reproduce the problem by your own DNS server.
The same case can be handled by the JDK DNS Resolver.

### Netty version
4.1.27

### JVM version (e.g. `java -version`)
JDK 1.8.181

### OS version (e.g. `uname -a`)
Centos 7.0

Issue comments:
@slggamerTrue thanks for reporting. Let me come up with a unit test and fix.
@slggamerTrue can you test https://github.com/netty/netty/pull/8691 ?
Thanks @normanmaurer. The fix is perfect. Looking forward to the new release. 
@slggamerTrue thanks for checking and reporting

Pull request review comments:
Use `nameWithDot` and `mappingWithDot` here?

Patch that resolved the issue:
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 e75f49c8d46..847bfb5b9f3 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
@@ -48,6 +48,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -651,10 +652,11 @@ private void onExpectedResponse(
 
             // Make sure the record is for the questioned domain.
             if (!recordName.equals(questionName)) {
+                Map<String, String> cnamesCopy = new HashMap<String, String>(cnames);
                 // Even if the record's name is not exactly same, it might be an alias defined in the CNAME records.
                 String resolved = questionName;
                 do {
-                    resolved = cnames.get(resolved);
+                    resolved = cnamesCopy.remove(resolved);
                     if (recordName.equals(resolved)) {
                         break;
                     }
@@ -749,8 +751,12 @@ private static Map<String, String> buildAliasMap(DnsResponse response, DnsCnameC
             String mapping = domainName.toLowerCase(Locale.US);
 
             // Cache the CNAME as well.
-            cache.cache(hostnameWithDot(name), hostnameWithDot(mapping), r.timeToLive(), loop);
-            cnames.put(name, mapping);
+            String nameWithDot = hostnameWithDot(name);
+            String mappingWithDot = hostnameWithDot(mapping);
+            if (!nameWithDot.equalsIgnoreCase(mappingWithDot)) {
+                cache.cache(nameWithDot, mappingWithDot, r.timeToLive(), loop);
+                cnames.put(name, mapping);
+            }
         }
 
         return cnames != null? cnames : Collections.<String, String>emptyMap();
@@ -875,12 +881,21 @@ private DnsServerAddressStream getNameServers(String hostname) {
 
     private void followCname(DnsQuestion question, String cname, DnsQueryLifecycleObserver queryLifecycleObserver,
                              Promise<List<T>> promise) {
+        Set<String> cnames = null;
         for (;;) {
             // Resolve from cnameCache() until there is no more cname entry cached.
             String mapping = cnameCache().get(hostnameWithDot(cname));
             if (mapping == null) {
                 break;
             }
+            if (cnames == null) {
+                // Detect loops.
+                cnames = new HashSet<String>(2);
+            }
+            if (!cnames.add(cname)) {
+                // Follow CNAME from cache would loop. Lets break here.
+                break;
+            }
             cname = mapping;
         }
 


Test patch:
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 70c8b6b52e0..1326a7975e8 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
@@ -96,6 +96,7 @@
 import static io.netty.handler.codec.dns.DnsRecordType.CNAME;
 import static io.netty.resolver.dns.DnsServerAddresses.sequential;
 import static java.util.Collections.singletonList;
+import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
@@ -2135,6 +2136,52 @@ public Set<ResourceRecord> getRecords(QuestionRecord question) {
         }
     }
 
+    @Test
+    public void testFollowCNAMELoop() throws IOException {
+        expectedException.expect(UnknownHostException.class);
+        TestDnsServer dnsServer2 = new TestDnsServer(new RecordStore() {
+
+            @Override
+            public Set<ResourceRecord> getRecords(QuestionRecord question) {
+                Set<ResourceRecord> records = new LinkedHashSet<ResourceRecord>(4);
+
+                records.add(new TestDnsServer.TestResourceRecord("x." + question.getDomainName(),
+                        RecordType.A, Collections.<String, Object>singletonMap(
+                                DnsAttribute.IP_ADDRESS.toLowerCase(), "10.0.0.99")));
+                records.add(new TestDnsServer.TestResourceRecord(
+                        "cname2.netty.io", RecordType.CNAME,
+                        Collections.<String, Object>singletonMap(
+                                DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname.netty.io")));
+                records.add(new TestDnsServer.TestResourceRecord(
+                        "cname.netty.io", RecordType.CNAME,
+                        Collections.<String, Object>singletonMap(
+                                DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname2.netty.io")));
+                records.add(new TestDnsServer.TestResourceRecord(
+                        question.getDomainName(), RecordType.CNAME,
+                        Collections.<String, Object>singletonMap(
+                                DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname.netty.io")));
+                return records;
+            }
+        });
+        dnsServer2.start();
+        DnsNameResolver resolver = null;
+        try {
+            DnsNameResolverBuilder builder = newResolver()
+                    .recursionDesired(false)
+                    .resolvedAddressTypes(ResolvedAddressTypes.IPV4_ONLY)
+                    .maxQueriesPerResolve(16)
+                    .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer2.localAddress()));
+
+            resolver = builder.build();
+            resolver.resolveAll("somehost.netty.io").syncUninterruptibly().getNow();
+        } finally {
+            dnsServer2.stop();
+            if (resolver != null) {
+                resolver.close();
+            }
+        }
+    }
+
     @Test
     public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() {
         expectedException.expect(UnknownHostException.class);