diff --git a/CHANGES.txt b/CHANGES.txt index d1fb5626dd..67a78a0a7e 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -10,6 +10,7 @@ * Clean up ScheduledExecutors, CommitLog, and MessagingService shutdown for in-JVM dtests (CASSANDRA-17731) * Remove extra write to system table for prepared statements (CASSANDRA-17764) Merged from 3.11: + * Fix potential IndexOutOfBoundsException in PagingState in mixed mode clusters (CASSANDRA-17840) * Document usage of closed token intervals in manual compaction (CASSANDRA-17575) Merged from 3.0: * Improve libjemalloc resolution in bin/cassandra (CASSANDRA-15767) diff --git a/src/java/org/apache/cassandra/service/pager/PagingState.java b/src/java/org/apache/cassandra/service/pager/PagingState.java index 4dde936310..2b160329de 100644 --- a/src/java/org/apache/cassandra/service/pager/PagingState.java +++ b/src/java/org/apache/cassandra/service/pager/PagingState.java @@ -136,39 +136,68 @@ public class PagingState return out.buffer(false); } - private static boolean isModernSerialized(ByteBuffer bytes) + @VisibleForTesting + static boolean isModernSerialized(ByteBuffer bytes) { int index = bytes.position(); int limit = bytes.limit(); - long partitionKeyLen = getUnsignedVInt(bytes, index, limit); + int partitionKeyLen = toIntExact(getUnsignedVInt(bytes, index, limit)); if (partitionKeyLen < 0) return false; - index += computeUnsignedVIntSize(partitionKeyLen) + partitionKeyLen; - if (index >= limit) + index = addNonNegative(index, computeUnsignedVIntSize(partitionKeyLen), partitionKeyLen); + if (index >= limit || index < 0) return false; - long rowMarkerLen = getUnsignedVInt(bytes, index, limit); + int rowMarkerLen = toIntExact(getUnsignedVInt(bytes, index, limit)); if (rowMarkerLen < 0) return false; - index += computeUnsignedVIntSize(rowMarkerLen) + rowMarkerLen; - if (index >= limit) + index = addNonNegative(index, computeUnsignedVIntSize(rowMarkerLen), rowMarkerLen); + if (index >= limit || index < 0) return false; - long remaining = getUnsignedVInt(bytes, index, limit); + int remaining = toIntExact(getUnsignedVInt(bytes, index, limit)); if (remaining < 0) return false; - index += computeUnsignedVIntSize(remaining); - if (index >= limit) + index = addNonNegative(index, computeUnsignedVIntSize(remaining)); + if (index >= limit || index < 0) return false; long remainingInPartition = getUnsignedVInt(bytes, index, limit); if (remainingInPartition < 0) return false; - index += computeUnsignedVIntSize(remainingInPartition); + index = addNonNegative(index, computeUnsignedVIntSize(remainingInPartition)); return index == limit; } + // Following operations are similar to Math.{addExact/toIntExact}, but without using exceptions for control flow. + // Since we're operating non-negative numbers, we can use -1 return value as an error code. + private static int addNonNegative(int x, int y) + { + int sum = x + y; + if (sum < 0) + return -1; + return sum; + } + + private static int addNonNegative(int x, int y, int z) + { + int sum = x + y; + if (sum < 0) + return -1; + sum += z; + if (sum < 0) + return -1; + return sum; + } + + private static int toIntExact(long value) + { + if ((int)value != value) + return -1; + return (int)value; + } + @SuppressWarnings({ "resource", "RedundantSuppression" }) private static PagingState modernDeserialize(ByteBuffer bytes, ProtocolVersion protocolVersion) throws IOException { @@ -216,7 +245,8 @@ public class PagingState return out.buffer(false); } - private static boolean isLegacySerialized(ByteBuffer bytes) + @VisibleForTesting + static boolean isLegacySerialized(ByteBuffer bytes) { int index = bytes.position(); int limit = bytes.limit(); diff --git a/src/java/org/apache/cassandra/utils/vint/VIntCoding.java b/src/java/org/apache/cassandra/utils/vint/VIntCoding.java index 6961d9f191..c35f8345b3 100644 --- a/src/java/org/apache/cassandra/utils/vint/VIntCoding.java +++ b/src/java/org/apache/cassandra/utils/vint/VIntCoding.java @@ -105,6 +105,9 @@ public class VIntCoding } public static long getUnsignedVInt(ByteBuffer input, int readerIndex, int readerLimit) { + if (readerIndex < 0) + throw new IllegalArgumentException("Reader index should be non-negative, but was " + readerIndex); + if (readerIndex >= readerLimit) return -1; diff --git a/test/unit/org/apache/cassandra/service/pager/RandomizedPagingStateTest.java b/test/unit/org/apache/cassandra/service/pager/RandomizedPagingStateTest.java new file mode 100644 index 0000000000..1ef3b956d2 --- /dev/null +++ b/test/unit/org/apache/cassandra/service/pager/RandomizedPagingStateTest.java @@ -0,0 +1,104 @@ +/* + * 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.service.pager; + +import java.nio.ByteBuffer; +import java.util.Random; + +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.cql3.ColumnIdentifier; +import org.apache.cassandra.db.Clustering; +import org.apache.cassandra.db.marshal.BytesType; +import org.apache.cassandra.db.marshal.LongType; +import org.apache.cassandra.db.rows.BTreeRow; +import org.apache.cassandra.db.rows.BufferCell; +import org.apache.cassandra.db.rows.Row; +import org.apache.cassandra.schema.ColumnMetadata; +import org.apache.cassandra.schema.TableMetadata; +import org.apache.cassandra.transport.ProtocolVersion; +import org.apache.cassandra.utils.ByteBufferUtil; + +public class RandomizedPagingStateTest +{ + private static final Random rnd = new Random(); + private static final int ROUNDS = 50_000; + private static final int MAX_PK_SIZE = 3000; + private static final int MAX_CK_SIZE = 3000; + private static final int MAX_REMAINING = 5000; + + @BeforeClass + public static void beforeClass() + { + DatabaseDescriptor.daemonInitialization(); + } + + @Test + public void testFormatChecksPkOnly() + { + rnd.setSeed(1); + TableMetadata metadata = TableMetadata.builder("ks", "tbl") + .addPartitionKeyColumn("pk", BytesType.instance) + .addRegularColumn("v", LongType.instance) + .build(); + Row row = BTreeRow.emptyRow(Clustering.EMPTY); + for (int i = 0; i < ROUNDS; i++) + checkState(metadata, MAX_PK_SIZE, row); + } + + @Test + public void testFormatChecksPkAndCk() + { + rnd.setSeed(1); + TableMetadata metadata = TableMetadata.builder("ks", "tbl") + .addPartitionKeyColumn("pk", BytesType.instance) + .addClusteringColumn("ck", BytesType.instance) + .addRegularColumn("v", LongType.instance) + .build(); + ColumnMetadata def = metadata.getColumn(new ColumnIdentifier("v", false)); + + for (int i = 0; i < ROUNDS; i++) + { + ByteBuffer ckBytes = ByteBuffer.allocate(rnd.nextInt(MAX_CK_SIZE) + 1); + for (int j = 0; j < ckBytes.limit(); j++) + ckBytes.put((byte) rnd.nextInt()); + ckBytes.flip().rewind(); + + Clustering c = Clustering.make(ckBytes); + Row row = BTreeRow.singleCellRow(c, BufferCell.live(def, 0, ByteBufferUtil.bytes(0L))); + + checkState(metadata, 1, row); + } + } + private static void checkState(TableMetadata metadata, int maxPkSize, Row row) + { + PagingState.RowMark mark = PagingState.RowMark.create(metadata, row, ProtocolVersion.V3); + ByteBuffer pkBytes = ByteBuffer.allocate(rnd.nextInt(maxPkSize) + 1); + for (int j = 0; j < pkBytes.limit(); j++) + pkBytes.put((byte) rnd.nextInt()); + pkBytes.flip().rewind(); + + PagingState state = new PagingState(pkBytes, mark, rnd.nextInt(MAX_REMAINING) + 1, rnd.nextInt(MAX_REMAINING) + 1); + ByteBuffer serialized = state.serialize(ProtocolVersion.V3); + Assert.assertTrue(PagingState.isLegacySerialized(serialized)); + Assert.assertFalse(PagingState.isModernSerialized(serialized)); + } +}