diff --git a/CHANGES.txt b/CHANGES.txt index cc1262d32b..e07fbaf44c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -155,6 +155,7 @@ Merged from 4.1: * Enforce CQL message size limit on multiframe messages (CASSANDRA-20052) * 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 QueryCancellationChecker 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/gms/VersionedValue.java b/src/java/org/apache/cassandra/gms/VersionedValue.java index 0c9a0138a0..b03211b05c 100644 --- a/src/java/org/apache/cassandra/gms/VersionedValue.java +++ b/src/java/org/apache/cassandra/gms/VersionedValue.java @@ -22,6 +22,7 @@ import java.io.DataOutputStream; import java.io.IOException; import java.net.InetAddress; import java.util.Collection; +import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.function.Supplier; @@ -138,6 +139,21 @@ public class VersionedValue implements Comparable return "Value(" + value + ',' + version + ')'; } + @Override + public boolean equals(Object o) + { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + VersionedValue that = (VersionedValue) o; + return version == that.version && value.equals(that.value); + } + + @Override + public int hashCode() + { + return Objects.hash(version, value); + } + public byte[] toBytes() { return value.getBytes(ISO_8859_1); diff --git a/src/java/org/apache/cassandra/transport/CBUtil.java b/src/java/org/apache/cassandra/transport/CBUtil.java index eb0d5a16f6..1c73206c67 100644 --- a/src/java/org/apache/cassandra/transport/CBUtil.java +++ b/src/java/org/apache/cassandra/transport/CBUtil.java @@ -32,6 +32,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.UUID; + +import com.google.common.base.Preconditions; + import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.ByteBufUtil; @@ -39,8 +42,8 @@ import io.netty.buffer.PooledByteBufAllocator; import io.netty.buffer.UnpooledByteBufAllocator; import io.netty.util.concurrent.FastThreadLocal; import org.apache.cassandra.db.ConsistencyLevel; -import org.apache.cassandra.db.TypeSizes; import org.apache.cassandra.utils.ByteBufferUtil; +import org.apache.cassandra.utils.LazyToString; import org.apache.cassandra.utils.Pair; import org.apache.cassandra.utils.TimeUUID; import org.apache.cassandra.utils.UUIDGen; @@ -161,14 +164,30 @@ public abstract class CBUtil public static void writeString(String str, ByteBuf cb) { - int length = TypeSizes.encodedUTF8Length(str); + int length = encodedUTF8Length(str); + Preconditions.checkArgument(length <= Short.MAX_VALUE, + LazyToString.lazy(() -> String.format("String too large; expected %d <= %d", length, Short.MAX_VALUE))); 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); } /** @@ -196,14 +215,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/gms/VersionedValueTest.java b/test/unit/org/apache/cassandra/gms/VersionedValueTest.java new file mode 100644 index 0000000000..ac47ee5496 --- /dev/null +++ b/test/unit/org/apache/cassandra/gms/VersionedValueTest.java @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.gms; + +import org.junit.Test; + +import accord.utils.Gen; +import org.apache.cassandra.db.TypeSizes; +import org.apache.cassandra.io.IVersionedSerializers; +import org.apache.cassandra.io.util.DataOutputBuffer; +import org.apache.cassandra.net.MessagingService; +import org.apache.cassandra.utils.CassandraGenerators; +import org.apache.cassandra.utils.Generators; + +import static accord.utils.Property.qt; + +public class VersionedValueTest +{ + @Test + public void serde() + { + DataOutputBuffer buffer = new DataOutputBuffer(); + qt().forAll(values()).check(state -> { + for (MessagingService.Version version : MessagingService.Version.supportedVersions()) + IVersionedSerializers.testSerde(buffer, VersionedValue.serializer, state, version.value); + }); + } + + private static Gen values() + { + return Generators.toGen(CassandraGenerators.partitioners().flatMap(CassandraGenerators::gossipStatusValue)) + // when status == "" this returns null... skip + .filter(vv -> vv != null) + // sometimes the text is too big, must not be larger than Short.MAX_VALUE + .filter(vv -> TypeSizes.encodedUTF8Length(vv.value) <= Short.MAX_VALUE); + } +} \ No newline at end of file diff --git a/test/unit/org/apache/cassandra/io/IVersionedSerializers.java b/test/unit/org/apache/cassandra/io/IVersionedSerializers.java new file mode 100644 index 0000000000..e17e0b7ce2 --- /dev/null +++ b/test/unit/org/apache/cassandra/io/IVersionedSerializers.java @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.io; + +import java.io.IOException; + +import org.apache.cassandra.io.util.DataInputBuffer; +import org.apache.cassandra.io.util.DataOutputBuffer; +import org.assertj.core.api.Assertions; + +public class IVersionedSerializers +{ + public static void testSerde(DataOutputBuffer output, IVersionedSerializer serializer, T input, int version) throws IOException + { + output.clear(); + long expectedSize = serializer.serializedSize(input, version); + serializer.serialize(input, output, version); + Assertions.assertThat(output.getLength()).describedAs("The serialized size and bytes written do not match").isEqualTo(expectedSize); + DataInputBuffer in = new DataInputBuffer(output.unsafeGetBufferAndFlip(), false); + T read = serializer.deserialize(in, version); + Assertions.assertThat(read).describedAs("The deserialized output does not match the serialized input").isEqualTo(input); + } +} \ No newline at end of file diff --git a/test/unit/org/apache/cassandra/transport/CBUtilTest.java b/test/unit/org/apache/cassandra/transport/CBUtilTest.java index c20efec8f8..4409655d33 100644 --- a/test/unit/org/apache/cassandra/transport/CBUtilTest.java +++ b/test/unit/org/apache/cassandra/transport/CBUtilTest.java @@ -22,9 +22,13 @@ import org.junit.After; import org.junit.Assert; import org.junit.Test; +import accord.utils.Gens; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; import io.netty.buffer.PooledByteBufAllocator; +import org.assertj.core.api.Assertions; + +import static accord.utils.Property.qt; public class CBUtilTest { @@ -38,6 +42,21 @@ public class CBUtilTest buf.release(buf.refCnt()); } + @Test + public void stringList() + { + qt().forAll(Gens.lists(Gens.strings().all().ofLengthBetween(0, 20)).ofSizeBetween(0, 10)).check(list -> { + int expectedSize = CBUtil.sizeOfStringList(list); + var 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() { diff --git a/test/unit/org/apache/cassandra/utils/CassandraGenerators.java b/test/unit/org/apache/cassandra/utils/CassandraGenerators.java index da93b264b7..5c3fd3790c 100644 --- a/test/unit/org/apache/cassandra/utils/CassandraGenerators.java +++ b/test/unit/org/apache/cassandra/utils/CassandraGenerators.java @@ -721,9 +721,13 @@ public final class CassandraGenerators ); } - private static Gen gossipStatusValue() + public static Gen gossipStatusValue() + { + return gossipStatusValue(DatabaseDescriptor.getPartitioner()); + } + + public static Gen gossipStatusValue(IPartitioner partitioner) { - IPartitioner partitioner = DatabaseDescriptor.getPartitioner(); Gen statusGen = gossipStatus(); Gen tokenGen = token(partitioner); Gen> tokensGen = tokens(partitioner);