Fix CompositeMetadataFlyweight check of ASCII only custom mime (rsocket#651)

simonbasle · robertroeser · commit c9a4ea174c13 · 2019-06-13T21:12:01.000-07:00
* Fix CompositeMetadataFlyweight check of ASCII only custom mime

This fixes potential false positives in the encodeMetadataHeader
method "ASCII only" check that results in IllegalArgumentException.

The first byte is readable but not actually written when performing
the check, yet we would consider it in `isText` (looking at a random
byte value).

Signed-off-by: Simon Baslé &lt;sbasle@pivotal.io&gt;

* add a test for the ascii check bug

Signed-off-by: Simon Baslé &lt;sbasle@pivotal.io&gt;

* forgot googleDataFormat (again)

Signed-off-by: Simon Baslé &lt;sbasle@pivotal.io&gt;
diff --git a/rsocket-core/src/main/java/io/rsocket/metadata/CompositeMetadataFlyweight.java b/rsocket-core/src/main/java/io/rsocket/metadata/CompositeMetadataFlyweight.java
@@ -329,13 +329,15 @@ static ByteBuf encodeMetadataHeader(
       ByteBufAllocator allocator, String customMime, int metadataLength) {
     ByteBuf metadataHeader = allocator.buffer(4 + customMime.length());
     // reserve 1 byte for the customMime length
+    // /!\ careful not to read that first byte, which is random at this point
     int writerIndexInitial = metadataHeader.writerIndex();
     metadataHeader.writerIndex(writerIndexInitial + 1);
 
     // write the custom mime in UTF8 but validate it is all ASCII-compatible
     // (which produces the right result since ASCII chars are still encoded on 1 byte in UTF8)
     int customMimeLength = ByteBufUtil.writeUtf8(metadataHeader, customMime);
-    if (!ByteBufUtil.isText(metadataHeader, CharsetUtil.US_ASCII)) {
+    if (!ByteBufUtil.isText(
+        metadataHeader, metadataHeader.readerIndex() + 1, customMimeLength, CharsetUtil.US_ASCII)) {
       metadataHeader.release();
       throw new IllegalArgumentException("custom mime type must be US_ASCII characters only");
     }
diff --git a/rsocket-core/src/test/java/io/rsocket/metadata/CompositeMetadataFlyweightTest.java b/rsocket-core/src/test/java/io/rsocket/metadata/CompositeMetadataFlyweightTest.java
@@ -19,15 +19,9 @@
 import static io.rsocket.metadata.CompositeMetadataFlyweight.decodeMimeAndContentBuffersSlices;
 import static io.rsocket.metadata.CompositeMetadataFlyweight.decodeMimeIdFromMimeBuffer;
 import static io.rsocket.metadata.CompositeMetadataFlyweight.decodeMimeTypeFromMimeBuffer;
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
-import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
-
-import io.netty.buffer.ByteBuf;
-import io.netty.buffer.ByteBufAllocator;
-import io.netty.buffer.CompositeByteBuf;
-import io.netty.buffer.Unpooled;
-import io.netty.buffer.UnpooledByteBufAllocator;
+import static org.assertj.core.api.Assertions.*;
+
+import io.netty.buffer.*;
 import io.netty.util.CharsetUtil;
 import io.rsocket.test.util.ByteBufUtils;
 import io.rsocket.util.NumberUtils;
@@ -524,4 +518,37 @@ void knownMimeHeaderZero_avro() {
 
     assertThat(content.readableBytes()).as("no metadata content").isZero();
   }
+
+  @Test
+  void encodeCustomHeaderAsciiCheckSkipsFirstByte() {
+    final ByteBuf badBuf = Unpooled.copiedBuffer("é00000000000", CharsetUtil.UTF_8);
+    badBuf.writerIndex(0);
+    assertThat(badBuf.readerIndex()).isZero();
+
+    ByteBufAllocator allocator =
+        new AbstractByteBufAllocator() {
+          @Override
+          public boolean isDirectBufferPooled() {
+            return false;
+          }
+
+          @Override
+          protected ByteBuf newHeapBuffer(int initialCapacity, int maxCapacity) {
+            return badBuf;
+          }
+
+          @Override
+          protected ByteBuf newDirectBuffer(int initialCapacity, int maxCapacity) {
+            return badBuf;
+          }
+        };
+
+    assertThatCode(
+            () -> CompositeMetadataFlyweight.encodeMetadataHeader(allocator, "custom/type", 0))
+        .doesNotThrowAnyException();
+
+    assertThat(badBuf.readByte()).isEqualTo((byte) 10);
+    assertThat(badBuf.readCharSequence(11, CharsetUtil.UTF_8)).hasToString("custom/type");
+    assertThat(badBuf.readUnsignedMedium()).isEqualTo(0);
+  }
 }
