Repository: cassandra Updated Branches: refs/heads/cassandra-3.0 b7da0031f -> b7fc5dc1c
Make Collections deserialization more robust patch by Benjamin Lerer; reviewed by Alex Petrov for CASSANDRA-12618 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/6dc595dd Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/6dc595dd Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/6dc595dd Branch: refs/heads/cassandra-3.0 Commit: 6dc595dd2ebf8477b4775473d1d2417fa3a0fcfd Parents: e9fe96f Author: Benjamin Lerer <b.le...@gmail.com> Authored: Tue Sep 27 14:02:10 2016 +0200 Committer: Benjamin Lerer <b.le...@gmail.com> Committed: Tue Sep 27 14:02:10 2016 +0200 ---------------------------------------------------------------------- CHANGES.txt | 4 ++ .../apache/cassandra/db/marshal/UserType.java | 8 ++-- .../cassandra/serializers/ListSerializer.java | 10 ++++- .../cassandra/serializers/MapSerializer.java | 10 ++++- .../cassandra/serializers/SetSerializer.java | 13 ++++++- .../cassandra/serializers/UTF8Serializer.java | 3 ++ .../validation/entities/CollectionsTest.java | 41 ++++++++++++++++++++ .../cql3/validation/entities/TupleTypeTest.java | 10 +++++ .../cql3/validation/entities/UserTypesTest.java | 11 ++++++ 9 files changed, 101 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index bc9fc5b..998849e 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,7 @@ +2.2.9 + * Make Collections deserialization more robust (CASSANDRA-12618) + + 2.2.8 * Fix exceptions when enabling gossip on nodes that haven't joined the ring (CASSANDRA-12253) * Fix authentication problem when invoking clqsh copy from a SOURCE command (CASSANDRA-12642) http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/src/java/org/apache/cassandra/db/marshal/UserType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/UserType.java b/src/java/org/apache/cassandra/db/marshal/UserType.java index 6102d26..187deeb 100644 --- a/src/java/org/apache/cassandra/db/marshal/UserType.java +++ b/src/java/org/apache/cassandra/db/marshal/UserType.java @@ -19,7 +19,6 @@ package org.apache.cassandra.db.marshal; import java.nio.ByteBuffer; import java.nio.charset.CharacterCodingException; -import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.*; @@ -28,8 +27,7 @@ import com.google.common.base.Objects; import org.apache.cassandra.cql3.*; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.SyntaxException; -import org.apache.cassandra.serializers.*; -import org.apache.cassandra.transport.Server; +import org.apache.cassandra.serializers.MarshalException; import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.Pair; @@ -123,7 +121,7 @@ public class UserType extends TupleType return; if (input.remaining() < 4) - throw new MarshalException(String.format("Not enough bytes to read size of %dth field %s", i, fieldName(i))); + throw new MarshalException(String.format("Not enough bytes to read size of %dth field %s", i, fieldNameAsString(i))); int size = input.getInt(); @@ -132,7 +130,7 @@ public class UserType extends TupleType continue; if (input.remaining() < size) - throw new MarshalException(String.format("Not enough bytes to read %dth field %s", i, fieldName(i))); + throw new MarshalException(String.format("Not enough bytes to read %dth field %s", i, fieldNameAsString(i))); ByteBuffer field = ByteBufferUtil.readBytes(input, size); types.get(i).validate(field); http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/src/java/org/apache/cassandra/serializers/ListSerializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/ListSerializer.java b/src/java/org/apache/cassandra/serializers/ListSerializer.java index aeee2b9..d2d0610 100644 --- a/src/java/org/apache/cassandra/serializers/ListSerializer.java +++ b/src/java/org/apache/cassandra/serializers/ListSerializer.java @@ -84,7 +84,15 @@ public class ListSerializer<T> extends CollectionSerializer<List<T>> { ByteBuffer input = bytes.duplicate(); int n = readCollectionSize(input, version); - List<T> l = new ArrayList<T>(n); + + if (n < 0) + throw new MarshalException("The data cannot be deserialized as a list"); + + // If the received bytes are not corresponding to a list, n might be a huge number. + // In such a case we do not want to initialize the list with that size as it can result + // in an OOM (see CASSANDRA-12618). On the other hand we do not want to have to resize the list + // if we can avoid it, so we put a reasonable limit on the initialCapacity. + List<T> l = new ArrayList<T>(Math.min(n, 256)); for (int i = 0; i < n; i++) { // We can have nulls in lists that are used for IN values http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/src/java/org/apache/cassandra/serializers/MapSerializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/MapSerializer.java b/src/java/org/apache/cassandra/serializers/MapSerializer.java index 8350f66..66831a3 100644 --- a/src/java/org/apache/cassandra/serializers/MapSerializer.java +++ b/src/java/org/apache/cassandra/serializers/MapSerializer.java @@ -94,7 +94,15 @@ public class MapSerializer<K, V> extends CollectionSerializer<Map<K, V>> { ByteBuffer input = bytes.duplicate(); int n = readCollectionSize(input, version); - Map<K, V> m = new LinkedHashMap<K, V>(n); + + if (n < 0) + throw new MarshalException("The data cannot be deserialized as a map"); + + // If the received bytes are not corresponding to a map, n might be a huge number. + // In such a case we do not want to initialize the map with that initialCapacity as it can result + // in an OOM when put is called (see CASSANDRA-12618). On the other hand we do not want to have to resize + // the map if we can avoid it, so we put a reasonable limit on the initialCapacity. + Map<K, V> m = new LinkedHashMap<K, V>(Math.min(n, 256)); for (int i = 0; i < n; i++) { ByteBuffer kbb = readValue(input, version); http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/src/java/org/apache/cassandra/serializers/SetSerializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/SetSerializer.java b/src/java/org/apache/cassandra/serializers/SetSerializer.java index 21f5075..4aaf36a 100644 --- a/src/java/org/apache/cassandra/serializers/SetSerializer.java +++ b/src/java/org/apache/cassandra/serializers/SetSerializer.java @@ -81,7 +81,16 @@ public class SetSerializer<T> extends CollectionSerializer<Set<T>> { ByteBuffer input = bytes.duplicate(); int n = readCollectionSize(input, version); - Set<T> l = new LinkedHashSet<T>(n); + + if (n < 0) + throw new MarshalException("The data cannot be deserialized as a set"); + + // If the received bytes are not corresponding to a set, n might be a huge number. + // In such a case we do not want to initialize the set with that initialCapacity as it can result + // in an OOM when add is called (see CASSANDRA-12618). On the other hand we do not want to have to resize + // the set if we can avoid it, so we put a reasonable limit on the initialCapacity. + Set<T> l = new LinkedHashSet<T>(Math.min(n, 256)); + for (int i = 0; i < n; i++) { ByteBuffer databb = readValue(input, version); @@ -94,7 +103,7 @@ public class SetSerializer<T> extends CollectionSerializer<Set<T>> } catch (BufferUnderflowException e) { - throw new MarshalException("Not enough bytes to read a list"); + throw new MarshalException("Not enough bytes to read a set"); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/src/java/org/apache/cassandra/serializers/UTF8Serializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/UTF8Serializer.java b/src/java/org/apache/cassandra/serializers/UTF8Serializer.java index e3ea2d5..7c41b94 100644 --- a/src/java/org/apache/cassandra/serializers/UTF8Serializer.java +++ b/src/java/org/apache/cassandra/serializers/UTF8Serializer.java @@ -52,6 +52,9 @@ public class UTF8Serializer extends AbstractTextSerializer // buf has already been sliced/duplicated. static boolean validate(ByteBuffer buf) { + if (buf == null) + return false; + buf = buf.slice(); int b = 0; State state = State.START; http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java index 6266fe7..115b755 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java @@ -607,4 +607,45 @@ public class CollectionsTest extends CQLTester assertRows(execute("select s_list from %s where k1='a'"), row(list(0))); } + @Test + public void testInvalidInputForList() throws Throwable + { + createTable("CREATE TABLE %s(pk int PRIMARY KEY, l list<text>)"); + assertInvalidMessage("Not enough bytes to read a list", + "INSERT INTO %s (pk, l) VALUES (?, ?)", 1, "test"); + assertInvalidMessage("Not enough bytes to read a list", + "INSERT INTO %s (pk, l) VALUES (?, ?)", 1, Long.MAX_VALUE); + assertInvalidMessage("Not enough bytes to read a list", + "INSERT INTO %s (pk, l) VALUES (?, ?)", 1, ""); + assertInvalidMessage("The data cannot be deserialized as a list", + "INSERT INTO %s (pk, l) VALUES (?, ?)", 1, -1); + } + + @Test + public void testInvalidInputForSet() throws Throwable + { + createTable("CREATE TABLE %s(pk int PRIMARY KEY, s set<text>)"); + assertInvalidMessage("Not enough bytes to read a set", + "INSERT INTO %s (pk, s) VALUES (?, ?)", 1, "test"); + assertInvalidMessage("String didn't validate.", + "INSERT INTO %s (pk, s) VALUES (?, ?)", 1, Long.MAX_VALUE); + assertInvalidMessage("Not enough bytes to read a set", + "INSERT INTO %s (pk, s) VALUES (?, ?)", 1, ""); + assertInvalidMessage("The data cannot be deserialized as a set", + "INSERT INTO %s (pk, s) VALUES (?, ?)", 1, -1); + } + + @Test + public void testInvalidInputForMap() throws Throwable + { + createTable("CREATE TABLE %s(pk int PRIMARY KEY, m map<text, text>)"); + assertInvalidMessage("Not enough bytes to read a map", + "INSERT INTO %s (pk, m) VALUES (?, ?)", 1, "test"); + assertInvalidMessage("String didn't validate.", + "INSERT INTO %s (pk, m) VALUES (?, ?)", 1, Long.MAX_VALUE); + assertInvalidMessage("Not enough bytes to read a map", + "INSERT INTO %s (pk, m) VALUES (?, ?)", 1, ""); + assertInvalidMessage("The data cannot be deserialized as a map", + "INSERT INTO %s (pk, m) VALUES (?, ?)", 1, -1); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java index 0e7084f..0783dd1 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java @@ -193,4 +193,14 @@ public class TupleTypeTest extends CQLTester row(0, 0, "b"), row(0, 0, "c")); } + + @Test + public void testInvalidInputForTuple() throws Throwable + { + createTable("CREATE TABLE %s(pk int PRIMARY KEY, t tuple<text, text>)"); + assertInvalidMessage("Not enough bytes to read 0th component", + "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, "test"); + assertInvalidMessage("Not enough bytes to read 0th component", + "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, Long.MAX_VALUE); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/6dc595dd/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java index cd87eda..3803e5c 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java @@ -45,6 +45,17 @@ public class UserTypesTest extends CQLTester } @Test + public void testInvalidInputForUserType() throws Throwable + { + String myType = createType("CREATE TYPE %s (f int)"); + createTable("CREATE TABLE %s(pk int PRIMARY KEY, t frozen<" + myType + ">)"); + assertInvalidMessage("Not enough bytes to read 0th field f", + "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, "test"); + assertInvalidMessage("Not enough bytes to read 0th field f", + "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, Long.MAX_VALUE); + } + + @Test public void testCassandra8105() throws Throwable { String ut1 = createType("CREATE TYPE %s (a int, b int)");