[
https://issues.apache.org/jira/browse/CASSANDRA-15790?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17102703#comment-17102703
]
David Capwell commented on CASSANDRA-15790:
-------------------------------------------
Thanks for the review [~yifanc]!
bq. Should readValue(DataInputPlus in, int maxValueSize) raise an exception in
the case that maxValueSize is not 0? If it ever happens, the system is in a bad
state.
Here is a sample call site
{code}
// function org.apache.cassandra.db.rows.Cell.Serializer#deserialize
value = header.getType(column).readValue(in,
DatabaseDescriptor.getMaxValueSize());
{code}
So, maxValueSize is not expected to be 0, so don't think so. Based off the
current usage, we should never be called (call site is guarded by null check)
so exception works... not sure how I feel, I kinda think that exception makes
sense but so does returning null (we returned empty buffer before (see
org.apache.cassandra.utils.ByteBufferUtil#read) but serializer returns null...
so not consistent... (see
org.apache.cassandra.serializers.EmptySerializer#deserialize)
bq. Is the AssertionError intended? I can see the intention might be indicating
the severity.
Do you mean [this
line|https://github.com/apache/cassandra/compare/trunk...dcapwell:bug/CASSANDRA-15790#diff-7dd64369e759d811269ca1be2d14086cR153]?
If so yes, this means we have a bug and should NOT move forward (else we
cause data loss).
bq. nit: EmptyTypeTest.java has no new line at the EOF.
If only we ran check style =). fixed; [see
here|https://github.com/dcapwell/cassandra/commit/23f3a4f1691f6a76016f7b21d1e3a6ee4ae3c3ab]
> EmptyType doesn't override writeValue so could attempt to write bytes when
> expected not to
> ------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-15790
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15790
> Project: Cassandra
> Issue Type: Bug
> Components: CQL/Semantics
> Reporter: David Capwell
> Assignee: David Capwell
> Priority: Normal
> Fix For: 3.0.x, 3.11.x, 4.0-alpha
>
>
> EmptyType.writeValues is defined here
> https://github.com/apache/cassandra/blob/e394dc0bb32f612a476269010930c617dd1ed3cb/src/java/org/apache/cassandra/db/marshal/AbstractType.java#L407-L414
> {code}
> public void writeValue(ByteBuffer value, DataOutputPlus out) throws
> IOException
> {
> assert value.hasRemaining();
> if (valueLengthIfFixed() >= 0)
> out.write(value);
> else
> ByteBufferUtil.writeWithVIntLength(value, out);
> }
> {code}
> This is fine when the value is empty as the write of empty no-ops (the
> readValue also noops since the length is 0), but if the value is not empty
> (possible during upgrades or random bugs) then this could silently cause
> corruption; ideally this should throw a exception if the ByteBuffer has data.
> This was called from
> org.apache.cassandra.db.rows.BufferCell.Serializer#serialize, here we check
> to see if data is present or not and update the flags. If data is present
> then and only then do we call type.writeValue (which requires bytes is not
> empty). The problem is that EmptyType never expects writes to happen, but it
> still writes them; and does not read them (since it says it is fixed length
> of 0, so does read(buffer, 0)).
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
