[ https://issues.apache.org/jira/browse/CASSANDRA-15790?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17102783#comment-17102783 ]
Yifan Cai commented on CASSANDRA-15790: --------------------------------------- Thanks for digging deeper into call sites for readValue. I agree that the overrides in {{EmptyType}} should never be called with the current usage. All the usage are guarded by empty value check and produce an {{EMPTY_BYTE_BUFFER}} inline (without even calling the {{readValue()}} method), except one. {{SinglePartitionReadCommand.Deserializer#deserialize}} does call the {{readValue()}} method regardlessly. However, the type is for partition keys, so I do not think it could be an EmptyType. I think returning a value (either {{EMPTY_BYTE_BUFFER}} or null) from readValue method sounds better. Because {{EmptyType}} cannot have the assumption on how the call sites do the check and skip calling the readValue method. - Returning null, eventually cause an exception to indicate the abnormal state. - Returning {{EMPTY_BYTE_BUFFER}} may silent the error. _I did not read carefully and thought {{maxValueSize}} was the length to read. Thanks for pointing it out._ > 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: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org