Fix IndexOutOfBoundsException caused by consuming the buffer multiple…

… times in DatagramDnsQueryDecoder (netty#11592)

Motivation:

ccef8fe introduced some changes to share code but did introduce a regression when decoding queries.

Modifications:

- Correctly only decode one time.
- Adjust unit test

Result:

Fixes netty#11591

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

@@ -15,11 +15,9 @@
  */
 package io.netty.handler.codec.dns;
 
-import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.socket.DatagramPacket;
-import io.netty.handler.codec.CorruptedFrameException;
 import io.netty.handler.codec.MessageToMessageDecoder;
 import io.netty.util.internal.UnstableApi;
 
@@ -52,71 +50,13 @@ public DatagramDnsQueryDecoder(DnsRecordDecoder recordDecoder) {
 
     @Override
     protected void decode(ChannelHandlerContext ctx, final DatagramPacket packet, List<Object> out) throws Exception {
-        final ByteBuf buf = packet.content();
-
-        DnsMessageUtil.decodeDnsQuery(recordDecoder, buf, new DnsMessageUtil.DnsQueryFactory() {
+        DnsQuery query = DnsMessageUtil.decodeDnsQuery(recordDecoder, packet.content(),
+                new DnsMessageUtil.DnsQueryFactory() {
             @Override
             public DnsQuery newQuery(int id, DnsOpCode dnsOpCode) {
                 return new DatagramDnsQuery(packet.sender(), packet.recipient(), id, dnsOpCode);
             }
         });
-
-        final DnsQuery query = newQuery(packet, buf);
-        boolean success = false;
-        try {
-            final int questionCount = buf.readUnsignedShort();
-            final int answerCount = buf.readUnsignedShort();
-            final int authorityRecordCount = buf.readUnsignedShort();
-            final int additionalRecordCount = buf.readUnsignedShort();
-
-            decodeQuestions(query, buf, questionCount);
-            decodeRecords(query, DnsSection.ANSWER, buf, answerCount);
-            decodeRecords(query, DnsSection.AUTHORITY, buf, authorityRecordCount);
-            decodeRecords(query, DnsSection.ADDITIONAL, buf, additionalRecordCount);
-
-            out.add(query);
-            success = true;
-        } finally {
-            if (!success) {
-                query.release();
-            }
-        }
-    }
-
-    private static DnsQuery newQuery(DatagramPacket packet, ByteBuf buf) {
-        final int id = buf.readUnsignedShort();
-
-        final int flags = buf.readUnsignedShort();
-        if (flags >> 15 == 1) {
-            throw new CorruptedFrameException("not a query");
-        }
-        final DnsQuery query =
-            new DatagramDnsQuery(
-                packet.sender(),
-                packet.recipient(),
-                id,
-                DnsOpCode.valueOf((byte) (flags >> 11 & 0xf)));
-        query.setRecursionDesired((flags >> 8 & 1) == 1);
-        query.setZ(flags >> 4 & 0x7);
-        return query;
-    }
-
-    private void decodeQuestions(DnsQuery query, ByteBuf buf, int questionCount) throws Exception {
-        for (int i = questionCount; i > 0; i--) {
-            query.addRecord(DnsSection.QUESTION, recordDecoder.decodeQuestion(buf));
-        }
-    }
-
-    private void decodeRecords(
-        DnsQuery query, DnsSection section, ByteBuf buf, int count) throws Exception {
-        for (int i = count; i > 0; i--) {
-            final DnsRecord r = recordDecoder.decodeRecord(buf);
-            if (r == null) {
-                // Truncated response
-                break;
-            }
-
-            query.addRecord(section, r);
-        }
+        out.add(query);
     }
 }

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

@@ -27,15 +27,19 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class DnsQueryTest {
 
     @Test
-    public void writeQueryTest() throws Exception {
+    public void testEncodeAndDecodeQuery() {
         InetSocketAddress addr = SocketUtils.socketAddress("8.8.8.8", 53);
-        EmbeddedChannel embedder = new EmbeddedChannel(new DatagramDnsQueryEncoder());
+        EmbeddedChannel writeChannel = new EmbeddedChannel(new DatagramDnsQueryEncoder());
+        EmbeddedChannel readChannel = new EmbeddedChannel(new DatagramDnsQueryDecoder());
+
         List<DnsQuery> queries = new ArrayList<DnsQuery>(5);
         queries.add(new DatagramDnsQuery(null, addr, 1).setRecord(
                 DnsSection.QUESTION,
@@ -59,12 +63,17 @@ public void writeQueryTest() throws Exception {
             assertThat(query.count(DnsSection.AUTHORITY), is(0));
             assertThat(query.count(DnsSection.ADDITIONAL), is(0));
 
-            embedder.writeOutbound(query);
+            assertTrue(writeChannel.writeOutbound(query));
 
-            DatagramPacket packet = embedder.readOutbound();
+            DatagramPacket packet = writeChannel.readOutbound();
             assertTrue(packet.content().isReadable());
-            packet.release();
-            assertNull(embedder.readOutbound());
+            assertTrue(readChannel.writeInbound(packet));
+            assertEquals(query, readChannel.readInbound());
+            assertNull(writeChannel.readOutbound());
+            assertNull(readChannel.readInbound());
         }
+
+        assertFalse(writeChannel.finish());
+        assertFalse(readChannel.finish());
     }
 }