You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "jonmeredith (via GitHub)" <gi...@apache.org> on 2023/05/08 16:50:44 UTC
[GitHub] [cassandra] jonmeredith opened a new pull request, #2312: NPE when deserializing malformed collections from client
jonmeredith opened a new pull request, #2312:
URL: https://github.com/apache/cassandra/pull/2312
See CASSANDRA-18505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "jonmeredith (via GitHub)" <gi...@apache.org>.
jonmeredith commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1188683950
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
Went with your suggestion - null in the input seems more likely than corruption so a better, cleaner exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187697886
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
I think need to leave the possibility of a null open for lists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187687426
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
Why not to throw that exception from that method? We do not need to throw it explicitly everywhere then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith closed pull request #2312: NPE when deserializing malformed collections from client
Posted by "jonmeredith (via GitHub)" <gi...@apache.org>.
jonmeredith closed pull request #2312: NPE when deserializing malformed collections from client
URL: https://github.com/apache/cassandra/pull/2312
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187680916
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
nit: This check has become unnecessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187683261
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
The comment above seems to think we should be able to have nulls?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187699158
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
For sets and maps, I would almost have a separate `readNonNullValue()` that does throw internally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a diff in pull request #2312: NPE when deserializing malformed collections from client
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #2312:
URL: https://github.com/apache/cassandra/pull/2312#discussion_r1187697886
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
I think we need to leave the possibility of a null open for lists.
##########
src/java/org/apache/cassandra/serializers/ListSerializer.java:
##########
@@ -104,6 +106,8 @@ public <V> List<T> deserializeForNativeProtocol(V input, ValueAccessor<V> access
{
// We can have nulls in lists that are used for IN values
V databb = readValue(input, accessor, offset, version);
+ if (databb == null)
+ throw new MarshalException("Not enough bytes to read value in list");
offset += sizeOfValue(databb, accessor, version);
if (databb != null)
Review Comment:
I think we need to leave the possibility of a null open for lists on a -1 size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org