Fix incorrect name encoding/decoding in DNS records

Motivation:

- The decoded name should always end with a dot (.), but we currently
  strip it, which is incorrect.
  - (O) 0 -> "."
  - (X) 0 -> ""
  - (O) 5 netty 2 io 0 -> "netty.io."
  - (X) 5 netty 2 io 0 -> "netty.io"
- The encoded name should end with a null-label, which is a label whose
  length is 0, but we currently append an extra NUL, causing FORMERR(1)
  on a strict DNS server:
  - (O) . -> 0
  - (X) . -> 0 0
  - (O) netty.io. -> 5 netty 2 io 0
  - (X) netty.io. -> 5 netty 2 io 0 0

Modifications:

- Make sure to append '.' when decoding a name.
- Improve index checks so that the decoder can raise
  CorruptFrameException instead of IIOBE
- Do not encode extra NUL
- Add more tests

Result:

Robustness and correctness

codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java

@@ -27,6 +27,8 @@ import io.netty.util.internal.StringUtil;
  */
 public class DefaultDnsRecordDecoder implements DnsRecordDecoder {
 
+    static final String ROOT = ".";
+
     /**
      * Creates a new instance.
      */
@@ -117,16 +119,22 @@ public class DefaultDnsRecordDecoder implements DnsRecordDecoder {
         // - https://github.com/netty/netty/issues/5014
         // - https://www.ietf.org/rfc/rfc1035.txt , Section 3.1
         if (readable == 0) {
-            return StringUtil.EMPTY_STRING;
+            return ROOT;
         }
+
         final StringBuilder name = new StringBuilder(readable << 1);
-        for (int len = in.readUnsignedByte(); in.isReadable() && len != 0; len = in.readUnsignedByte()) {
+        while (in.isReadable()) {
-            boolean pointer = (len & 0xc0) == 0xc0;
+            final int len = in.readUnsignedByte();
+            final boolean pointer = (len & 0xc0) == 0xc0;
             if (pointer) {
                 if (position == -1) {
                     position = in.readerIndex() + 1;
                 }
 
+                if (!in.isReadable()) {
+                    throw new CorruptedFrameException("truncated pointer in a name");
+                }
+
                 final int next = (len & 0x3f) << 8 | in.readUnsignedByte();
                 if (next >= end) {
                     throw new CorruptedFrameException("name has an out-of-range pointer");
@@ -138,18 +146,29 @@ public class DefaultDnsRecordDecoder implements DnsRecordDecoder {
                 if (checked >= end) {
                     throw new CorruptedFrameException("name contains a loop.");
                 }
-            } else {
+            } else if (len != 0) {
+                if (!in.isReadable(len)) {
+                    throw new CorruptedFrameException("truncated label in a name");
+                }
                 name.append(in.toString(in.readerIndex(), len, CharsetUtil.UTF_8)).append('.');
                 in.skipBytes(len);
+            } else { // len == 0
+                break;
             }
         }
+
         if (position != -1) {
             in.readerIndex(position);
         }
+
         if (name.length() == 0) {
-            return StringUtil.EMPTY_STRING;
+            return ROOT;
         }
 
-        return name.substring(0, name.length() - 1);
+        if (name.charAt(name.length() - 1) != '.') {
+            name.append('.');
+        }
+
+        return name.toString();
     }
 }

codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordEncoder.java

@@ -20,6 +20,8 @@ import io.netty.buffer.ByteBufUtil;
 import io.netty.handler.codec.UnsupportedMessageTypeException;
 import io.netty.util.internal.StringUtil;
 
+import static io.netty.handler.codec.dns.DefaultDnsRecordDecoder.ROOT;
+
 /**
  * The default {@link DnsRecordEncoder} implementation.
  *
@@ -77,19 +79,24 @@ public class DefaultDnsRecordEncoder implements DnsRecordEncoder {
     }
 
     protected void encodeName(String name, ByteBuf buf) throws Exception {
-        String[] parts = StringUtil.split(name, '.');
+        if (ROOT.equals(name)) {
-        for (String part: parts) {
+            // Root domain
-            final int partLen = part.length();
+            buf.writeByte(0);
-            // We always need to write the length even if its 0.
+            return;
-            // See:
-            // - https://github.com/netty/netty/issues/5014
-            // - https://www.ietf.org/rfc/rfc1035.txt , Section 3.1
-            buf.writeByte(partLen);
-            if (partLen == 0) {
-                continue;
-            }
-            ByteBufUtil.writeAscii(buf, part);
         }
+
+        final String[] labels = StringUtil.split(name, '.');
+        for (String label : labels) {
+            final int labelLen = label.length();
+            if (labelLen == 0) {
+                // zero-length label means the end of the name.
+                break;
+            }
+
+            buf.writeByte(labelLen);
+            ByteBufUtil.writeAscii(buf, label);
+        }
+
         buf.writeByte(0); // marks end of name field
     }
 }

codec-dns/src/test/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoderTest.java

@@ -24,19 +24,46 @@ import org.junit.Test;
 public class DefaultDnsRecordDecoderTest {
 
     @Test
-    public void testDecodeEmptyName() {
+    public void testDecodeName() {
-        testDecodeEmptyName0(Unpooled.buffer().writeByte('0'));
+        testDecodeName("netty.io.", Unpooled.wrappedBuffer(new byte[] {
+                5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o', 0
+        }));
     }
 
     @Test
-    public void testDecodeEmptyNameNonRFC() {
+    public void testDecodeNameWithoutTerminator() {
-        testDecodeEmptyName0(Unpooled.EMPTY_BUFFER);
+        testDecodeName("netty.io.", Unpooled.wrappedBuffer(new byte[] {
+                5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o'
+        }));
     }
 
-    private static void testDecodeEmptyName0(ByteBuf buffer) {
+    @Test
+    public void testDecodeNameWithExtraTerminator() {
+        // Should not be decoded as 'netty.io..'
+        testDecodeName("netty.io.", Unpooled.wrappedBuffer(new byte[] {
+                5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o', 0, 0
+        }));
+    }
+
+    @Test
+    public void testDecodeEmptyName() {
+        testDecodeName(".", Unpooled.buffer().writeByte(0));
+    }
+
+    @Test
+    public void testDecodeEmptyNameFromEmptyBuffer() {
+        testDecodeName(".", Unpooled.EMPTY_BUFFER);
+    }
+
+    @Test
+    public void testDecodeEmptyNameFromExtraZeroes() {
+        testDecodeName(".", Unpooled.wrappedBuffer(new byte[] { 0, 0 }));
+    }
+
+    private static void testDecodeName(String expected, ByteBuf buffer) {
         try {
             DefaultDnsRecordDecoder decoder = new DefaultDnsRecordDecoder();
-            Assert.assertEquals(StringUtil.EMPTY_STRING, decoder.decodeName(buffer));
+            Assert.assertEquals(expected, decoder.decodeName(buffer));
         } finally {
             buffer.release();
         }

codec-dns/src/test/java/io/netty/handler/codec/dns/DefaultDnsRecordEncoderTest.java

@@ -16,6 +16,7 @@
 package io.netty.handler.codec.dns;
 
 import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.Unpooled;
 import io.netty.util.internal.StringUtil;
 import org.junit.Test;
@@ -24,18 +25,42 @@ import static org.junit.Assert.assertEquals;
 
 public class DefaultDnsRecordEncoderTest {
 
+    @Test
+    public void testEncodeName() throws Exception {
+        testEncodeName(new byte[] { 5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o', 0 }, "netty.io.");
+    }
+
+    @Test
+    public void testEncodeNameWithoutTerminator() throws Exception {
+        testEncodeName(new byte[] { 5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o', 0 }, "netty.io");
+    }
+
+    @Test
+    public void testEncodeNameWithExtraTerminator() throws Exception {
+        testEncodeName(new byte[] { 5, 'n', 'e', 't', 't', 'y', 2, 'i', 'o', 0 }, "netty.io..");
+    }
+
     // Test for https://github.com/netty/netty/issues/5014
     @Test
     public void testEncodeEmptyName() throws Exception {
+        testEncodeName(new byte[] { 0 }, StringUtil.EMPTY_STRING);
+    }
+
+    @Test
+    public void testEncodeRootName() throws Exception {
+        testEncodeName(new byte[] { 0 }, ".");
+    }
+
+    private static void testEncodeName(byte[] expected, String name) throws Exception {
         DefaultDnsRecordEncoder encoder = new DefaultDnsRecordEncoder();
         ByteBuf out = Unpooled.buffer();
+        ByteBuf expectedBuf = Unpooled.wrappedBuffer(expected);
         try {
-            encoder.encodeName(StringUtil.EMPTY_STRING, out);
+            encoder.encodeName(name, out);
-            assertEquals(2, out.readableBytes());
+            assertEquals(expectedBuf, out);
-            assertEquals(0, out.readByte());
-            assertEquals(0, out.readByte());
         } finally {
             out.release();
+            expectedBuf.release();
         }
     }
 }