Reset Utf8ByteBufCharsetDecoder splitCharBuffer, close AsyncHttpClient#1325

slandelle · slandelle · commit 4a946b592433 · 2017-01-15T09:34:19.000+01:00
Motivation:

Utf8ByteBufCharsetDecoder crashes with BufferOverflow when trying to
decode a char whose byte length is larger than the one of the first
split char that was decoded.

This happens because we only reset position while we should be
resetting capacity too.

Modifications:

User ByteBuffer reset instead os `position(0)`.

Result:

No more BufferOverflow
diff --git a/netty-utils/src/main/java/org/asynchttpclient/netty/util/Utf8ByteBufCharsetDecoder.java b/netty-utils/src/main/java/org/asynchttpclient/netty/util/Utf8ByteBufCharsetDecoder.java
@@ -24,6 +24,9 @@
 
 public class Utf8ByteBufCharsetDecoder {
 
+    private static final int INITIAL_CHAR_BUFFER_SIZE = 1024;
+    private static final int UTF_8_MAX_BYTES_PER_CHAR = 4;
+
     private static final ThreadLocal<Utf8ByteBufCharsetDecoder> POOL = new ThreadLocal<Utf8ByteBufCharsetDecoder>() {
         protected Utf8ByteBufCharsetDecoder initialValue() {
             return new Utf8ByteBufCharsetDecoder();
@@ -45,15 +48,8 @@ public static String decodeUtf8(ByteBuf... bufs) throws CharacterCodingException
     }
 
     private final CharsetDecoder decoder = UTF_8.newDecoder();
-    protected CharBuffer charBuffer = allocateCharBuffer(1024);
-    private ByteBuffer splitCharBuffer;
-
-    protected void initSplitCharBuffer() {
-        if (splitCharBuffer == null) {
-            // UTF-8 chars are 4 bytes max
-            splitCharBuffer = ByteBuffer.allocate(4);
-        }
-    }
+    protected CharBuffer charBuffer = allocateCharBuffer(INITIAL_CHAR_BUFFER_SIZE);
+    private ByteBuffer splitCharBuffer = ByteBuffer.allocate(UTF_8_MAX_BYTES_PER_CHAR);
 
     protected CharBuffer allocateCharBuffer(int l) {
         return CharBuffer.allocate(l);
@@ -75,6 +71,7 @@ private void ensureCapacity(int l) {
     public void reset() {
         decoder.reset();
         charBuffer.clear();
+        splitCharBuffer.clear();
     }
 
     private static int charSize(byte firstByte) throws CharacterCodingException {
@@ -119,8 +116,6 @@ private void handleSplitCharBuffer(ByteBuffer nioBuffer, boolean endOfInput) thr
             if (res.isError()) {
                 res.throwException();
             }
-
-            splitCharBuffer.position(0);
         }
     }
 
@@ -135,7 +130,6 @@ protected void decodePartial(ByteBuffer nioBuffer, boolean endOfInput) throws Ch
             CoderResult res = decoder.decode(nioBuffer, charBuffer, endOfInput);
             if (res.isUnderflow()) {
                 if (nioBuffer.remaining() > 0) {
-                    initSplitCharBuffer();
                     splitCharBuffer.put(nioBuffer);
                 }
             } else if (res.isError()) {
diff --git a/netty-utils/src/test/java/org/asynchttpclient/netty/util/ByteBufUtilsTest.java b/netty-utils/src/test/java/org/asynchttpclient/netty/util/ByteBufUtilsTest.java
@@ -13,7 +13,7 @@
  */
 package org.asynchttpclient.netty.util;
 
-import static java.nio.charset.StandardCharsets.US_ASCII;
+import static java.nio.charset.StandardCharsets.*;
 import static org.testng.Assert.assertEquals;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
@@ -25,17 +25,43 @@ public class ByteBufUtilsTest {
     @Test
     public void testByteBuf2BytesHasBackingArray() {
         byte[] inputBytes = "testdata".getBytes(US_ASCII);
-        ByteBuf inputBuf = Unpooled.wrappedBuffer(inputBytes);
-        byte[] output = ByteBufUtils.byteBuf2Bytes(inputBuf);
-        assertEquals(output, inputBytes);
+        ByteBuf buf = Unpooled.wrappedBuffer(inputBytes);
+        try {
+            byte[] output = ByteBufUtils.byteBuf2Bytes(buf);
+            assertEquals(output, inputBytes);
+        } finally {
+            buf.release();
+        }
     }
 
     @Test
     public void testByteBuf2BytesNoBackingArray() {
         byte[] inputBytes = "testdata".getBytes(US_ASCII);
-        ByteBuf inputBuf = Unpooled.directBuffer();
-        inputBuf.writeBytes(inputBytes);
-        byte[] output = ByteBufUtils.byteBuf2Bytes(inputBuf);
-        assertEquals(output, inputBytes);
+        ByteBuf buf = Unpooled.directBuffer();
+        try {
+            buf.writeBytes(inputBytes);
+            byte[] output = ByteBufUtils.byteBuf2Bytes(buf);
+            assertEquals(output, inputBytes);
+        } finally {
+            buf.release();
+        }
+    }
+
+    @Test
+    public void byteBufs2StringShouldBeAbleToDealWithCharsWithVariableBytesLength() throws Exception {
+        String inputString = "°ä–";
+        byte[] inputBytes = inputString.getBytes(UTF_8);
+
+        for (int i = 1; i < inputBytes.length - 1; i++) {
+            ByteBuf buf1 = Unpooled.wrappedBuffer(inputBytes, 0, i);
+            ByteBuf buf2 = Unpooled.wrappedBuffer(inputBytes, i, inputBytes.length - i);
+            try {
+                String s = ByteBufUtils.byteBuf2String(UTF_8, buf1, buf2);
+                assertEquals(s, inputString);
+            } finally {
+                buf1.release();
+                buf2.release();
+            }
+        }
     }
 }
