Repository: netty/netty
Issue: DnsNameResolver does not resolve property A+CNAME answer
Labels: defect

The current DnsNameResolver fails to resolve an A+CNAME answer. For example:

```
dig moose.rmq.cloudamqp.com

...
;; ANSWER SECTION:
moose.rmq.cloudamqp.com. 1800   IN  CNAME   ec2-54-152-221-139.compute-1.amazonaws.com.
ec2-54-152-221-139.compute-1.amazonaws.com. 583612 IN A 54.152.221.139
...
```

The resolver constructs a map of cnames but forgets the trailing "." in the values which lead to not resolve the A record: the DnsNameResolverContext#buildAliasMap returns a singleton map `moose.rmq.cloudamqp.com.->ec2-54-152-221-139.compute-1.amazonaws.com` without the trailing "." .

Then the `onResponseAorAAAA` cannot resolve `moose.rmq.cloudamqp.com` to the address `54.152.221.139`. as `if (rName.equals(resolved))` evalutes the false instead of true.

A possible fix is to change the last line of `decodeDomainName` to return `name.toString()` instead of `name.substring(0, name.length() - 1)`


Issue comments:
will check...

@vietj thanks for reporting!

it made my day epic :-)

DNS epic day

lol

Fixed by https://github.com/netty/netty/pull/5413


Pull request review comments:
Why need to create this `decodeName0` method? Is it for providing a hook method to allow people to change the behavior?

Yes

> Am 16.06.2016 um 22:38 schrieb Xiaoyan Lin notifications@github.com:
> 
> In codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java:
> 
> > @@ -106,7 +106,19 @@ protected DnsRecord decodeRecord(
> >       \* @param in the byte buffer containing the DNS packet
> >       \* @return the domain name for an entry
> >       */
> > -    protected String decodeName(ByteBuf in) {
> > -    protected String decodeName0(ByteBuf in) {
> >   Why need to create this decodeName0 method? Is it for providing a hook method to allow people to change the behavior?
> 
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub, or mute the thread.


Patch that resolved the issue:
diff --git a/codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java b/codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java
index d630413c843..3cafb477f35 100644
--- a/codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java
+++ b/codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java
@@ -92,7 +92,7 @@ protected DnsRecord decodeRecord(
 
         if (type == DnsRecordType.PTR) {
             in.setIndex(offset, offset + length);
-            return new DefaultDnsPtrRecord(name, dnsClass, timeToLive, decodeName(in));
+            return new DefaultDnsPtrRecord(name, dnsClass, timeToLive, decodeName0(in));
         }
         return new DefaultDnsRawRecord(
                 name, type, dnsClass, timeToLive, in.retainedDuplicate().setIndex(offset, offset + length));
@@ -106,7 +106,19 @@ protected DnsRecord decodeRecord(
      * @param in the byte buffer containing the DNS packet
      * @return the domain name for an entry
      */
-    protected String decodeName(ByteBuf in) {
+    protected String decodeName0(ByteBuf in) {
+        return decodeName(in);
+    }
+
+    /**
+     * Retrieves a domain name given a buffer containing a DNS packet. If the
+     * name contains a pointer, the position of the buffer will be set to
+     * directly after the pointer's index after the name has been read.
+     *
+     * @param in the byte buffer containing the DNS packet
+     * @return the domain name for an entry
+     */
+    public static String decodeName(ByteBuf in) {
         int position = -1;
         int checked = 0;
         final int end = in.writerIndex();
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 8a10f2e56a1..cfeb057e362 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
@@ -20,6 +20,7 @@
 import io.netty.buffer.ByteBufHolder;
 import io.netty.channel.AddressedEnvelope;
 import io.netty.channel.socket.InternetProtocolFamily;
+import io.netty.handler.codec.CorruptedFrameException;
 import io.netty.handler.codec.dns.DefaultDnsQuestion;
 import io.netty.handler.codec.dns.DefaultDnsRecordDecoder;
 import io.netty.handler.codec.dns.DnsResponseCode;
@@ -29,7 +30,6 @@
 import io.netty.handler.codec.dns.DnsRecord;
 import io.netty.handler.codec.dns.DnsRecordType;
 import io.netty.handler.codec.dns.DnsResponse;
-import io.netty.util.CharsetUtil;
 import io.netty.util.ReferenceCountUtil;
 import io.netty.util.concurrent.Future;
 import io.netty.util.concurrent.FutureListener;
@@ -438,53 +438,15 @@ private void finishResolve() {
     protected abstract boolean finishResolve(
             Class<? extends InetAddress> addressType, List<DnsCacheEntry> resolvedEntries);
 
-    /**
-     * Adapted from {@link DefaultDnsRecordDecoder#decodeName(ByteBuf)}.
-     */
-    static String decodeDomainName(ByteBuf buf) {
-        buf.markReaderIndex();
+    static String decodeDomainName(ByteBuf in) {
+        in.markReaderIndex();
         try {
-            int position = -1;
-            int checked = 0;
-            final int end = buf.writerIndex();
-            final StringBuilder name = new StringBuilder(buf.readableBytes() << 1);
-            for (int len = buf.readUnsignedByte(); buf.isReadable() && len != 0; len = buf.readUnsignedByte()) {
-                boolean pointer = (len & 0xc0) == 0xc0;
-                if (pointer) {
-                    if (position == -1) {
-                        position = buf.readerIndex() + 1;
-                    }
-
-                    final int next = (len & 0x3f) << 8 | buf.readUnsignedByte();
-                    if (next >= end) {
-                        // Should not happen.
-                        return null;
-                    }
-                    buf.readerIndex(next);
-
-                    // check for loops
-                    checked += 2;
-                    if (checked >= end) {
-                        // Name contains a loop; give up.
-                        return null;
-                    }
-                } else {
-                    name.append(buf.toString(buf.readerIndex(), len, CharsetUtil.UTF_8)).append('.');
-                    buf.skipBytes(len);
-                }
-            }
-
-            if (position != -1) {
-                buf.readerIndex(position);
-            }
-
-            if (name.length() == 0) {
-                return null;
-            }
-
-            return name.substring(0, name.length() - 1);
+            return DefaultDnsRecordDecoder.decodeName(in);
+        } catch (CorruptedFrameException e) {
+            // In this case we just return null.
+            return null;
         } finally {
-            buf.resetReaderIndex();
+            in.resetReaderIndex();
         }
     }
 


Test patch: