EmptyType doesn't override writeValue so could attempt to write bytes when expected not to

Patch by David Capwell; Reviewed by Jordan West for CASSANDRA-15790

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)

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<Void>
 {
+    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<Void>
     {
         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);
+        }
+    }
 }

src/java/org/apache/cassandra/serializers/EmptySerializer.java

@@ -28,6 +28,7 @@ public class EmptySerializer implements TypeSerializer<Void>
 
     public Void deserialize(ByteBuffer bytes)
     {
+        validate(bytes);
         return null;
     }
 

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());
+        }
+    }
+}