Correctly detect and handle CNAME loops. (#8691)

Motivation:

We do not correctly detect loops when follow CNAMEs and so may try to follow it without any success.

Modifications:

- Correctly detect CNAME loops
- Do not cache CNAME entries which point to itself
- Add unit test.

Result:

Fixes https://github.com/netty/netty/issues/8687.

resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java

@@ -48,6 +48,7 @@ import java.util.AbstractList;
 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 @@ abstract class DnsResolveContext<T> {
 
             // 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 @@ abstract class DnsResolveContext<T> {
             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 @@ abstract class DnsResolveContext<T> {
 
     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;
         }
 

resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java

@@ -96,6 +96,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.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 class DnsNameResolverTest {
         }
     }
 
+    @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);