This is an automated email from the ASF dual-hosted git repository.
jonmeredith pushed a commit to branch cassandra-4.0
in repository https://gitbox.apache.org/repos/asf/cassandra.git
The following commit(s) were added to refs/heads/cassandra-4.0 by this push:
new ae995eb3d3 NPE when deserializing malformed collections from client
ae995eb3d3 is described below
commit ae995eb3d3cc1c98f61db0d071522b6f09443927
Author: Jon Meredith <[email protected]>
AuthorDate: Tue May 9 08:54:40 2023 -0600
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 +--
.../cql3/validation/entities/CollectionsTest.java | 4 +--
.../apache/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<T> extends
TypeSerializer<T>
return accessor.slice(input, offset + TypeSizes.INT_SIZE, size);
}
+ public static <V> V readNonNullValue(V input, ValueAccessor<V> 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<T> extends
CollectionSerializer<List<T>>
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<T> extends
CollectionSerializer<List<T>>
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
+ // 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<K, V> extends
CollectionSerializer<Map<K, V>>
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<K, V> extends
CollectionSerializer<Map<K, V>>
Map<K, V> m = new LinkedHashMap<K, V>(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<T> extends
CollectionSerializer<Set<T>>
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<T> extends
CollectionSerializer<Set<T>>
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<text>)");
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<text, text>)");
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<ByteBuffer> 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<ByteBuffer> 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<ByteBuffer> 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
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
