diff --git a/CHANGES.txt b/CHANGES.txt index ee3ad12c76..25a1bbe81d 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.21 + * EmptyType doesn't override writeValue so could attempt to write bytes when expected not to (CASSANDRA-15790) * Fix index queries on partition key columns when some partitions contains only static data (CASSANDRA-13666) * Avoid creating duplicate rows during major upgrades (CASSANDRA-15789) * liveDiskSpaceUsed and totalDiskSpaceUsed get corrupted if IndexSummaryRedistribution gets interrupted (CASSANDRA-15674) diff --git a/src/java/org/apache/cassandra/db/marshal/EmptyType.java b/src/java/org/apache/cassandra/db/marshal/EmptyType.java index 806d1609d9..de087f51a6 100644 --- a/src/java/org/apache/cassandra/db/marshal/EmptyType.java +++ b/src/java/org/apache/cassandra/db/marshal/EmptyType.java @@ -18,14 +18,21 @@ package org.apache.cassandra.db.marshal; import java.nio.ByteBuffer; +import java.util.concurrent.TimeUnit; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.apache.cassandra.cql3.CQL3Type; import org.apache.cassandra.cql3.Constants; import org.apache.cassandra.cql3.Term; +import org.apache.cassandra.io.util.DataInputPlus; +import org.apache.cassandra.io.util.DataOutputPlus; import org.apache.cassandra.serializers.TypeSerializer; import org.apache.cassandra.serializers.EmptySerializer; import org.apache.cassandra.serializers.MarshalException; import org.apache.cassandra.utils.ByteBufferUtil; +import org.apache.cassandra.utils.NoSpamLogger; /** * A type that only accept empty data. @@ -33,6 +40,29 @@ import org.apache.cassandra.utils.ByteBufferUtil; */ public class EmptyType extends AbstractType { + private enum NonEmptyWriteBehavior { FAIL, LOG_DATA_LOSS, SILENT_DATA_LOSS } + + private static final Logger logger = LoggerFactory.getLogger(EmptyType.class); + private static final String KEY_EMPTYTYPE_NONEMPTY_BEHAVIOR = "cassandra.serialization.emptytype.nonempty_behavior"; + private static final NoSpamLogger NON_EMPTY_WRITE_LOGGER = NoSpamLogger.getLogger(logger, 1, TimeUnit.MINUTES); + private static final NonEmptyWriteBehavior NON_EMPTY_WRITE_BEHAVIOR = parseNonEmptyWriteBehavior(); + + private static NonEmptyWriteBehavior parseNonEmptyWriteBehavior() + { + String value = System.getProperty(KEY_EMPTYTYPE_NONEMPTY_BEHAVIOR); + if (value == null) + return NonEmptyWriteBehavior.FAIL; + try + { + return NonEmptyWriteBehavior.valueOf(value.toUpperCase().trim()); + } + catch (Exception e) + { + logger.warn("Unable to parse property " + KEY_EMPTYTYPE_NONEMPTY_BEHAVIOR + ", falling back to FAIL", e); + return NonEmptyWriteBehavior.FAIL; + } + } + public static final EmptyType instance = new EmptyType(); private EmptyType() {super(ComparisonType.CUSTOM);} // singleton @@ -87,4 +117,46 @@ public class EmptyType extends AbstractType { return 0; } + + @Override + public ByteBuffer readValue(DataInputPlus in) + { + return ByteBufferUtil.EMPTY_BYTE_BUFFER; + } + + @Override + public ByteBuffer readValue(DataInputPlus in, int maxValueSize) + { + return ByteBufferUtil.EMPTY_BYTE_BUFFER; + } + + @Override + public void writeValue(ByteBuffer value, DataOutputPlus out) + { + if (!value.hasRemaining()) + return; + // In 3.0 writeValue was added which required EmptyType to write data, and relied on caller to never do that; + // that behavior was unsafe so guard against it. There are configurable behaviors, but the only allowed cases + // should be *_DATA_LOSS (last resort... really should avoid this) and fail; fail should be preferred in nearly + // all cases. + // see CASSANDRA-15790 + switch (NON_EMPTY_WRITE_BEHAVIOR) + { + case LOG_DATA_LOSS: + NON_EMPTY_WRITE_LOGGER.warn("Dropping data...", new NonEmptyWriteException("Attempted to write a non-empty value using EmptyType")); + case SILENT_DATA_LOSS: + return; + case FAIL: + default: + throw new AssertionError("Attempted to write a non-empty value using EmptyType"); + } + } + + private static final class NonEmptyWriteException extends RuntimeException + { + NonEmptyWriteException(String message) + { + super(message); + } + } } diff --git a/src/java/org/apache/cassandra/serializers/EmptySerializer.java b/src/java/org/apache/cassandra/serializers/EmptySerializer.java index 2ccecc5c51..733e179ef0 100644 --- a/src/java/org/apache/cassandra/serializers/EmptySerializer.java +++ b/src/java/org/apache/cassandra/serializers/EmptySerializer.java @@ -28,6 +28,7 @@ public class EmptySerializer implements TypeSerializer public Void deserialize(ByteBuffer bytes) { + validate(bytes); return null; } diff --git a/test/unit/org/apache/cassandra/db/marshal/EmptyTypeTest.java b/test/unit/org/apache/cassandra/db/marshal/EmptyTypeTest.java new file mode 100644 index 0000000000..30f63b3d3a --- /dev/null +++ b/test/unit/org/apache/cassandra/db/marshal/EmptyTypeTest.java @@ -0,0 +1,94 @@ +package org.apache.cassandra.db.marshal; + +import java.nio.ByteBuffer; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.cassandra.io.util.DataInputPlus; +import org.apache.cassandra.io.util.DataOutputPlus; +import org.apache.cassandra.serializers.MarshalException; +import org.apache.cassandra.utils.ByteBufferUtil; +import org.mockito.Mockito; + + +public class EmptyTypeTest +{ + @Test + public void isFixed() + { + Assert.assertEquals(0, EmptyType.instance.valueLengthIfFixed()); + } + + @Test + public void writeEmptyAllowed() + { + DataOutputPlus output = Mockito.mock(DataOutputPlus.class); + EmptyType.instance.writeValue(ByteBufferUtil.EMPTY_BYTE_BUFFER, output); + + Mockito.verifyNoInteractions(output); + } + + @Test + public void writeNonEmpty() + { + DataOutputPlus output = Mockito.mock(DataOutputPlus.class); + ByteBuffer rejected = ByteBuffer.wrap("this better fail".getBytes()); + + boolean thrown = false; + try + { + EmptyType.instance.writeValue(rejected, output); + } + catch (AssertionError e) + { + thrown = true; + } + Assert.assertTrue("writeValue did not reject non-empty input", thrown); + + Mockito.verifyNoInteractions(output); + } + + @Test + public void read() + { + DataInputPlus input = Mockito.mock(DataInputPlus.class); + + ByteBuffer buffer = EmptyType.instance.readValue(input); + Assert.assertNotNull(buffer); + Assert.assertFalse("empty type returned back non-empty data", buffer.hasRemaining()); + + buffer = EmptyType.instance.readValue(input, 42); + Assert.assertNotNull(buffer); + Assert.assertFalse("empty type returned back non-empty data", buffer.hasRemaining()); + + Mockito.verifyNoInteractions(input); + } + + @Test + public void decompose() + { + ByteBuffer buffer = EmptyType.instance.decompose(null); + Assert.assertEquals(0, buffer.remaining()); + } + + @Test + public void composeEmptyInput() + { + Void result = EmptyType.instance.compose(ByteBufferUtil.EMPTY_BYTE_BUFFER); + Assert.assertNull(result); + } + + @Test + public void composeNonEmptyInput() + { + try + { + EmptyType.instance.compose(ByteBufferUtil.bytes("should fail")); + Assert.fail("compose is expected to reject non-empty values, but did not"); + } + catch (MarshalException e) { + Assert.assertEquals("EmptyType only accept empty values", e.getMessage()); + } + } +}