diff --git a/CHANGES.txt b/CHANGES.txt index 2c6d1522c8..baa7eee95c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -6,6 +6,7 @@ * Equality check for Paxos.Electorate should not depend on collection types (CASSANDRA-19935) * Fix race condition in DecayingEstimatedHistogramReservoir during rescale (CASSANDRA-19365) Merged from 4.0: + * CBUtil serialization of UTF8 does not handle all UTF8 properly (CASSANDRA-20234) * Make hint expiry use request start time rather than timeout time for TTL (CASSANDRA-20014) * Do not attach rows and partitions to CheckForAbort when already attached (CASSANDRA-20135) * Allow hint delivery during schema mismatch (CASSANDRA-20188) diff --git a/src/java/org/apache/cassandra/db/TypeSizes.java b/src/java/org/apache/cassandra/db/TypeSizes.java index 8df83b6ba2..54ed3eeac8 100644 --- a/src/java/org/apache/cassandra/db/TypeSizes.java +++ b/src/java/org/apache/cassandra/db/TypeSizes.java @@ -45,6 +45,16 @@ public final class TypeSizes return sizeof((short) length) + length; } + /** + * Java uses a Modified UTF-8 (see {@link java.io.DataOutput#writeUTF(String)}), and this method attempts to + * calculate the modified utf-8 length; this method only works when the utf-8 writing logic is java's modified utf-8 + * and will not work when normal utf-8 is written. + * + * If normal utf-8 is written, then {@link org.apache.cassandra.transport.CBUtil#encodedUTF8Length(String)} should be + * used instread of this one. + * + * @see Modified UTF 8 + */ public static int encodedUTF8Length(String st) { int strlen = st.length(); diff --git a/src/java/org/apache/cassandra/transport/CBUtil.java b/src/java/org/apache/cassandra/transport/CBUtil.java index 6cab63855e..dab6b49d9c 100644 --- a/src/java/org/apache/cassandra/transport/CBUtil.java +++ b/src/java/org/apache/cassandra/transport/CBUtil.java @@ -41,7 +41,6 @@ import io.netty.buffer.UnpooledByteBufAllocator; import io.netty.util.concurrent.FastThreadLocal; import org.apache.cassandra.config.Config; import org.apache.cassandra.db.ConsistencyLevel; -import org.apache.cassandra.db.TypeSizes; import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.Pair; import org.apache.cassandra.utils.TimeUUID; @@ -150,14 +149,28 @@ public abstract class CBUtil public static void writeString(String str, ByteBuf cb) { - int length = TypeSizes.encodedUTF8Length(str); + int length = encodedUTF8Length(str); cb.writeShort(length); ByteBufUtil.reserveAndWriteUtf8(cb, str, length); } public static int sizeOfString(String str) { - return 2 + TypeSizes.encodedUTF8Length(str); + return 2 + encodedUTF8Length(str); + } + + /** + * Java uses a Modified UTF-8, whereas Netty uses UTF-8 proper... this means that the encoded UTF8 lengths + * do not match, and you must be careful to use the correct length method... + * + * When using {@link ByteBufUtil#reserveAndWriteUtf8(ByteBuf, CharSequence, int)} or similiar logic, you must use + * this method. When using {@link java.io.DataOutput#writeUTF(String)} you must use {@link org.apache.cassandra.db.TypeSizes#encodedUTF8Length(String)}. + * + * @see Modified UTF 8 + */ + public static int encodedUTF8Length(String str) + { + return ByteBufUtil.utf8Bytes(str); } /** @@ -185,14 +198,14 @@ public abstract class CBUtil public static void writeLongString(String str, ByteBuf cb) { - int length = TypeSizes.encodedUTF8Length(str); + int length = encodedUTF8Length(str); cb.writeInt(length); ByteBufUtil.reserveAndWriteUtf8(cb, str, length); } public static int sizeOfLongString(String str) { - return 4 + TypeSizes.encodedUTF8Length(str); + return 4 + encodedUTF8Length(str); } public static byte[] readBytes(ByteBuf cb) diff --git a/test/unit/org/apache/cassandra/transport/CBUtilTest.java b/test/unit/org/apache/cassandra/transport/CBUtilTest.java index c20efec8f8..9e0cd74b83 100644 --- a/test/unit/org/apache/cassandra/transport/CBUtilTest.java +++ b/test/unit/org/apache/cassandra/transport/CBUtilTest.java @@ -25,6 +25,10 @@ import org.junit.Test; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.PooledByteBufAllocator; +import org.assertj.core.api.Assertions; +import org.quicktheories.generators.SourceDSL; + +import static org.quicktheories.QuickTheory.qt; public class CBUtilTest { @@ -38,6 +42,21 @@ public class CBUtilTest buf.release(buf.refCnt()); } + @Test + public void stringList() + { + qt().forAll(SourceDSL.lists().of(SourceDSL.strings().allPossible().ofLengthBetween(0, 20)).ofSizeBetween(0, 10)).checkAssert(list -> { + int expectedSize = CBUtil.sizeOfStringList(list); + ByteBuf body = CBUtil.allocator.buffer(expectedSize * 2); + CBUtil.writeStringList(list, body); + Assertions.assertThat(body.readableBytes()).isEqualTo(expectedSize); + // In CASSANDRA-20234 the sizeOf method now reflects the write method, but the work to do this in read + // was not done; read took a stance that it wants to limit things in CASSANDRA-8101 but that never made + // it to the other methods... write and read are not compatable with the full domain of UTF-8, and fixing + // that was higher bar than CASSANDRA-20234 wanted to tackle out of fear of unknown regressions it would cause. + }); + } + @Test public void writeAndReadString() {