From ef8dcae9afa672d6081f343754de577fd8e0f689 Mon Sep 17 00:00:00 2001
From: Trustin Lee <t@motd.kr>
Date: Mon, 7 Mar 2016 11:12:33 +0900
Subject: [PATCH] Fix potential infinite loop when resolving CNAME records

Related: #4771

Motivation:

A malicious or misconfigured DNS server can send the CNAME records that
resolve into each other, causing an unexpected infinite loop in
DnsNameResolverContext.onResponseCNAME().

Modifications:

- Remove the dereferenced CNAME from the alias map so that infinite loop
  is impossible.
- Fix inspection warnings and typos in DnsNameResolverTest

Result:

Fixes #4771
---
 .../resolver/dns/DnsNameResolverContext.java  |  6 ++++--
 .../resolver/dns/DnsNameResolverTest.java     | 21 +++++++++----------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java
index 15add82320..8d42d041bf 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java
@@ -283,8 +283,10 @@ abstract class DnsNameResolverContext<T> {
         final String name = question.name().toLowerCase(Locale.US);
         String resolved = name;
         boolean found = false;
-        for (;;) {
-            String next = cnames.get(resolved);
+        while (!cnames.isEmpty()) { // Do not attempt to call Map.remove() when the Map is empty
+                                    // because it can be Collections.emptyMap()
+                                    // whose remove() throws a UnsupportedOperationException.
+            final String next = cnames.remove(resolved);
             if (next != null) {
                 found = true;
                 resolved = next;
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 9218a0ce68..aef6fa7609 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
@@ -265,7 +265,7 @@ public class DnsNameResolverTest {
     private static final TestDnsServer dnsServer = new TestDnsServer();
     private static final EventLoopGroup group = new NioEventLoopGroup(1);
 
-    private DnsNameResolverBuilder newResolver() {
+    private static DnsNameResolverBuilder newResolver() {
         return new DnsNameResolverBuilder(group.next())
                 .channelType(NioDatagramChannel.class)
                 .nameServerAddresses(DnsServerAddresses.singleton(dnsServer.localAddress()))
@@ -273,7 +273,7 @@ public class DnsNameResolverTest {
                 .optResourceEnabled(false);
     }
 
-    private DnsNameResolverBuilder newResolver(InternetProtocolFamily... resolvedAddressTypes) {
+    private static DnsNameResolverBuilder newResolver(InternetProtocolFamily... resolvedAddressTypes) {
         return newResolver()
                 .resolvedAddressTypes(resolvedAddressTypes);
     }
@@ -349,7 +349,7 @@ public class DnsNameResolverTest {
         }
     }
 
-    private Map<String, InetAddress> testResolve0(DnsNameResolver resolver, Set<String> excludedDomains)
+    private static Map<String, InetAddress> testResolve0(DnsNameResolver resolver, Set<String> excludedDomains)
             throws InterruptedException {
 
         assertThat(resolver.isRecursionDesired(), is(true));
@@ -482,7 +482,7 @@ public class DnsNameResolverTest {
         }
     }
 
-    private UnknownHostException resolveNonExistentDomain(DnsNameResolver resolver) {
+    private static UnknownHostException resolveNonExistentDomain(DnsNameResolver resolver) {
         try {
             resolver.resolve("non-existent.netty.io").sync();
             fail();
@@ -505,8 +505,7 @@ public class DnsNameResolverTest {
         }
     }
 
-    private void resolve(DnsNameResolver resolver, Map<String, Future<InetAddress>> futures, String hostname) {
-
+    private static void resolve(DnsNameResolver resolver, Map<String, Future<InetAddress>> futures, String hostname) {
         futures.put(hostname, resolver.resolve(hostname));
     }
 
@@ -580,7 +579,7 @@ public class DnsNameResolverTest {
                             // This is a hack to allow to also test for AAAA resolution as DnsMessageEncoder
                             // does not support it and it is hard to extend, because the interesting methods
                             // are private...
-                            // In case of RecordType.AAAA we need to encode the RecordType by ourself.
+                            // In case of RecordType.AAAA we need to encode the RecordType by ourselves.
                             if (record.getRecordType() == RecordType.AAAA) {
                                 try {
                                     recordEncoder.put(buf, record);
@@ -639,10 +638,10 @@ public class DnsNameResolverTest {
             }
 
             private static String nextIp() {
-                return ippart() + "." + ippart() + '.' + ippart() + '.' + ippart();
+                return ipPart() + "." + ipPart() + '.' + ipPart() + '.' + ipPart();
             }
 
-            private static int ippart() {
+            private static int ipPart() {
                 return NUMBERS[index(NUMBERS.length)];
             }
 
@@ -672,10 +671,10 @@ public class DnsNameResolverTest {
                         } while (ThreadLocalRandom.current().nextBoolean());
                         break;
                     case MX:
-                        int prioritity = 0;
+                        int priority = 0;
                         do {
                             rm.put(DnsAttribute.DOMAIN_NAME, nextDomain());
-                            rm.put(DnsAttribute.MX_PREFERENCE, String.valueOf(++prioritity));
+                            rm.put(DnsAttribute.MX_PREFERENCE, String.valueOf(++priority));
                         } while (ThreadLocalRandom.current().nextBoolean());
                         break;
                     default: