From fe20550a223c7c00adc402cbb71b7330e7b9e45c Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Tue, 28 Apr 2020 10:47:10 +0200
Subject: [PATCH] Detect CNAME loops in the CNAME cache while trying to resolve
 (#10221)

Motivation:

We need to detect CNAME loops during lookup the DnsCnameCache as otherwise we may try to follow cnames forever.

Modifications:

- Correctly detect CNAME loops in the cache
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10220
---
 .../netty/resolver/dns/DnsResolveContext.java | 66 ++++++++++++-------
 .../resolver/dns/DnsNameResolverTest.java     | 28 ++++++++
 2 files changed, 71 insertions(+), 23 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 067c2dcd40..24f6e6f9bd 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
@@ -265,15 +265,52 @@ abstract class DnsResolveContext<T> {
         return name + '.';
     }
 
-    private void internalResolve(String name, Promise<List<T>> promise) {
-        for (;;) {
-            // Resolve from cnameCache() until there is no more cname entry cached.
-            String mapping = cnameCache().get(hostnameWithDot(name));
-            if (mapping == null) {
+    // Resolve the final name from the CNAME cache until there is nothing to follow anymore. This also
+    // guards against loops in the cache but early return once a loop is detected.
+    private String cnameResolveFromCache(String name) {
+        DnsCnameCache cnameCache = cnameCache();
+        String first = cnameCache.get(hostnameWithDot(name));
+        if (first == null) {
+            // Nothing in the cache at all
+            return name;
+        }
+
+        String second = cnameCache.get(hostnameWithDot(first));
+        if (second == null) {
+            // Nothing else to follow, return first match.
+            return first;
+        }
+
+        if (first.equals(second)) {
+            // Loop detected.... early return.
+            return first;
+        }
+
+        return cnameResolveFromCacheLoop(cnameCache, first, second);
+    }
+
+    private String cnameResolveFromCacheLoop(DnsCnameCache cnameCache, String first, String mapping) {
+        // Detect loops using a HashSet. We use this as last resort implementation to reduce allocations in the most
+        // common cases.
+        Set<String> cnames = new HashSet<>(4);
+        cnames.add(first);
+        cnames.add(mapping);
+
+        String name = mapping;
+        // Resolve from cnameCache() until there is no more cname entry cached.
+        while ((mapping = cnameCache.get(hostnameWithDot(name))) != null) {
+            if (!cnames.add(mapping)) {
+                // Follow CNAME from cache would loop. Lets break here.
                 break;
             }
             name = mapping;
         }
+        return name;
+    }
+
+    private void internalResolve(String name, Promise<List<T>> promise) {
+        // Resolve from cnameCache() until there is no more cname entry cached.
+        name = cnameResolveFromCache(name);
 
         try {
             DnsServerAddressStream nameServerAddressStream = getNameServers(name);
@@ -936,24 +973,7 @@ 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<>(2);
-            }
-            if (!cnames.add(cname)) {
-                // Follow CNAME from cache would loop. Lets break here.
-                break;
-            }
-            cname = mapping;
-        }
-
+        cname = cnameResolveFromCache(cname);
         DnsServerAddressStream stream = getNameServers(cname);
 
         final DnsQuestion cnameQuestion;
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 ee0866f22c..4acf4acbd7 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
@@ -2176,6 +2176,34 @@ public class DnsNameResolverTest {
         }
     }
 
+    @Test
+    public void testCNAMELoopInCache() throws Throwable {
+        expectedException.expect(UnknownHostException.class);
+        DnsNameResolver resolver = null;
+        try {
+            DnsNameResolverBuilder builder = newResolver()
+                    .recursionDesired(false)
+                    .resolvedAddressTypes(ResolvedAddressTypes.IPV4_ONLY)
+                    .maxQueriesPerResolve(16)
+                    .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer.localAddress()));
+
+            resolver = builder.build();
+            // Add a CNAME loop into the cache
+            String name = "somehost.netty.io.";
+            String name2 = "cname.netty.io.";
+
+            resolver.cnameCache().cache(name, name2, Long.MAX_VALUE, resolver.executor());
+            resolver.cnameCache().cache(name2, name, Long.MAX_VALUE, resolver.executor());
+            resolver.resolve(name).syncUninterruptibly().getNow();
+        } catch (CompletionException e) {
+            throw e.getCause();
+        } finally {
+            if (resolver != null) {
+                resolver.close();
+            }
+        }
+    }
+
     @Test
     public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() throws Throwable {
         expectedException.expect(UnknownHostException.class);