From ae995eb3d3cc1c98f61db0d071522b6f09443927 Mon Sep 17 00:00:00 2001 From: Jon Meredith Date: Tue, 9 May 2023 08:54:40 -0600 Subject: [PATCH] NPE when deserializing malformed collections from client patch by Jon Meredith; reviewed by Caleb Rackliffe for CASSANDRA-18505 --- CHANGES.txt | 1 + .../serializers/CollectionSerializer.java | 8 +++++ .../cassandra/serializers/ListSerializer.java | 9 +++-- .../cassandra/serializers/MapSerializer.java | 8 ++--- .../cassandra/serializers/SetSerializer.java | 4 +-- .../validation/entities/CollectionsTest.java | 4 +-- .../cassandra/transport/SerDeserTest.java | 36 +++++++++++++++++++ 7 files changed, 60 insertions(+), 10 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index a10b0f67df..ac053c3f5b 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.0.10 + * NPE when deserializing malformed collections from client (CASSANDRA-18505) * Improve 'Not enough space for compaction' logging messages (CASSANDRA-18260) * Incremental repairs fail on mixed IPv4/v6 addresses serializing SyncRequest (CASSANDRA-18474) * Deadlock updating sstable metadata if disk boundaries need reloading (CASSANDRA-18443) diff --git a/src/java/org/apache/cassandra/serializers/CollectionSerializer.java b/src/java/org/apache/cassandra/serializers/CollectionSerializer.java index eb2991b8d7..1722c3d109 100644 --- a/src/java/org/apache/cassandra/serializers/CollectionSerializer.java +++ b/src/java/org/apache/cassandra/serializers/CollectionSerializer.java @@ -127,6 +127,14 @@ public abstract class CollectionSerializer extends TypeSerializer return accessor.slice(input, offset + TypeSizes.INT_SIZE, size); } + public static V readNonNullValue(V input, ValueAccessor accessor, int offset, ProtocolVersion version) + { + V value = readValue(input, accessor, offset, version); + if (value == null) + throw new MarshalException("Null value read when not allowed"); + return value; + } + protected static void skipValue(ByteBuffer input, ProtocolVersion version) { int size = input.getInt(); diff --git a/src/java/org/apache/cassandra/serializers/ListSerializer.java b/src/java/org/apache/cassandra/serializers/ListSerializer.java index 240e0a62dc..35c31414e2 100644 --- a/src/java/org/apache/cassandra/serializers/ListSerializer.java +++ b/src/java/org/apache/cassandra/serializers/ListSerializer.java @@ -71,7 +71,7 @@ public class ListSerializer extends CollectionSerializer> int offset = sizeOfCollectionSize(n, version); for (int i = 0; i < n; i++) { - V value = readValue(input, accessor, offset, version); + V value = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(value, accessor, version); elements.validate(value, accessor); } @@ -102,7 +102,12 @@ public class ListSerializer extends CollectionSerializer> List l = new ArrayList(Math.min(n, 256)); for (int i = 0; i < n; i++) { - // We can have nulls in lists that are used for IN values + // CASSANDRA-6839: "We can have nulls in lists that are used for IN values" + // CASSANDRA-8613 checks IN clauses and throws an exception if null is in the list. + // Leaving for this as-is for now in case there is some unknown use + // for it, but should likely be changed to readNonNull. Validate has been + // changed to throw on null elements as otherwise it would NPE, and it's unclear + // if callers could handle null elements. V databb = readValue(input, accessor, offset, version); offset += sizeOfValue(databb, accessor, version); if (databb != null) diff --git a/src/java/org/apache/cassandra/serializers/MapSerializer.java b/src/java/org/apache/cassandra/serializers/MapSerializer.java index 9eae598003..9c0a001534 100644 --- a/src/java/org/apache/cassandra/serializers/MapSerializer.java +++ b/src/java/org/apache/cassandra/serializers/MapSerializer.java @@ -89,11 +89,11 @@ public class MapSerializer extends CollectionSerializer> int offset = sizeOfCollectionSize(n, version); for (int i = 0; i < n; i++) { - T key = readValue(input, accessor, offset, version); + T key = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(key, accessor, version); keys.validate(key, accessor); - T value = readValue(input, accessor, offset, version); + T value = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(value, accessor, version); values.validate(value, accessor); } @@ -123,11 +123,11 @@ public class MapSerializer extends CollectionSerializer> Map m = new LinkedHashMap(Math.min(n, 256)); for (int i = 0; i < n; i++) { - I key = readValue(input, accessor, offset, version); + I key = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(key, accessor, version); keys.validate(key, accessor); - I value = readValue(input, accessor, offset, version); + I value = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(value, accessor, version); values.validate(value, accessor); diff --git a/src/java/org/apache/cassandra/serializers/SetSerializer.java b/src/java/org/apache/cassandra/serializers/SetSerializer.java index 0b7a2a5fa2..bd5e0188be 100644 --- a/src/java/org/apache/cassandra/serializers/SetSerializer.java +++ b/src/java/org/apache/cassandra/serializers/SetSerializer.java @@ -80,7 +80,7 @@ public class SetSerializer extends CollectionSerializer> int offset = sizeOfCollectionSize(n, version); for (int i = 0; i < n; i++) { - V value = readValue(input, accessor, offset, version); + V value = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(value, accessor, version); elements.validate(value, accessor); } @@ -111,7 +111,7 @@ public class SetSerializer extends CollectionSerializer> for (int i = 0; i < n; i++) { - V value = readValue(input, accessor, offset, version); + V value = readNonNullValue(input, accessor, offset, version); offset += sizeOfValue(value, accessor, version); elements.validate(value, accessor); l.add(elements.deserialize(value, accessor)); 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 b1596e346b..bec2ccdd98 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java @@ -975,7 +975,7 @@ public class CollectionsTest extends CQLTester createTable("CREATE TABLE %s(pk int PRIMARY KEY, s set)"); assertInvalidMessage("Not enough bytes to read a set", "INSERT INTO %s (pk, s) VALUES (?, ?)", 1, "test"); - assertInvalidMessage("String didn't validate.", + assertInvalidMessage("Null value read when not allowed", "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, ""); @@ -989,7 +989,7 @@ public class CollectionsTest extends CQLTester createTable("CREATE TABLE %s(pk int PRIMARY KEY, m map)"); assertInvalidMessage("Not enough bytes to read a map", "INSERT INTO %s (pk, m) VALUES (?, ?)", 1, "test"); - assertInvalidMessage("String didn't validate.", + assertInvalidMessage("Null value read when not allowed", "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, ""); diff --git a/test/unit/org/apache/cassandra/transport/SerDeserTest.java b/test/unit/org/apache/cassandra/transport/SerDeserTest.java index da76070b6f..603320311d 100644 --- a/test/unit/org/apache/cassandra/transport/SerDeserTest.java +++ b/test/unit/org/apache/cassandra/transport/SerDeserTest.java @@ -34,6 +34,7 @@ import org.apache.cassandra.cql3.*; import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.marshal.*; import org.apache.cassandra.serializers.CollectionSerializer; +import org.apache.cassandra.serializers.MarshalException; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.service.QueryState; import org.apache.cassandra.transport.Event.TopologyChange; @@ -107,6 +108,41 @@ public class SerDeserTest assertEquals(m, mt.getSerializer().deserializeForNativeProtocol(CollectionSerializer.pack(mb, m.size(), version), version)); } + @Test(expected = MarshalException.class) + public void setsMayNotContainNullsTest() + { + ProtocolVersion version = ProtocolVersion.MIN_SUPPORTED_VERSION; + SetType st = SetType.getInstance(UTF8Type.instance, true); + List sb = new ArrayList<>(1); + sb.add(null); + + st.getSerializer().deserializeForNativeProtocol(CollectionSerializer.pack(sb, sb.size(), version), version); + } + + @Test(expected = MarshalException.class) + public void mapKeysMayNotContainNullsTest() + { + ProtocolVersion version = ProtocolVersion.MIN_SUPPORTED_VERSION; + MapType mt = MapType.getInstance(UTF8Type.instance, LongType.instance, true); + List mb = new ArrayList<>(2); + mb.add(null); + mb.add(LongType.instance.decompose(999L)); + + mt.getSerializer().deserializeForNativeProtocol(CollectionSerializer.pack(mb, mb.size(), version), version); + } + + @Test(expected = MarshalException.class) + public void mapValueMayNotContainNullsTest() + { + ProtocolVersion version = ProtocolVersion.MIN_SUPPORTED_VERSION; + MapType mt = MapType.getInstance(UTF8Type.instance, LongType.instance, true); + List mb = new ArrayList<>(2); + mb.add(UTF8Type.instance.decompose("danger")); + mb.add(null); + + mt.getSerializer().deserializeForNativeProtocol(CollectionSerializer.pack(mb, mb.size(), version), version); + } + @Test public void eventSerDeserTest() throws Exception {