From beb9658dd5e18e3a6a4e8431b6549ae4c33365a9 Mon Sep 17 00:00:00 2001
From: Jeff Jirsa <jeff@jeffjirsa.net>
Date: Sun, 12 Mar 2017 21:54:04 -0700
Subject: [PATCH] Commitlog replay may fail if last mutation is within 4 bytes
 of end of segment

Patch by Jeff Jirsa; Reviewed by Branimir Lambov for CASSANDRA-13282
---
 CHANGES.txt                                   |  1 +
 .../db/commitlog/CommitLogReplayer.java       | 11 ++++++++++
 .../cassandra/db/commitlog/CommitLogTest.java | 20 +++++++++++++++----
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/CHANGES.txt b/CHANGES.txt
index 09e4039c78..283929146b 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -9,6 +9,7 @@
  * Fix failing COPY TO STDOUT (CASSANDRA-12497)
  * Fix ColumnCounter::countAll behaviour for reverse queries (CASSANDRA-13222)
  * Exceptions encountered calling getSeeds() breaks OTC thread (CASSANDRA-13018)
+ * Commitlog replay may fail if last mutation is within 4 bytes of end of segment (CASSANDRA-13282)
 Merged from 2.1:
  * Remove unused repositories (CASSANDRA-13278)
  * Log stacktrace of uncaught exceptions (CASSANDRA-13108)
diff --git a/src/java/org/apache/cassandra/db/commitlog/CommitLogReplayer.java b/src/java/org/apache/cassandra/db/commitlog/CommitLogReplayer.java
index a58aeb4f05..3cf4d0fa5e 100644
--- a/src/java/org/apache/cassandra/db/commitlog/CommitLogReplayer.java
+++ b/src/java/org/apache/cassandra/db/commitlog/CommitLogReplayer.java
@@ -439,6 +439,17 @@ public class CommitLogReplayer
             int serializedSize;
             try
             {
+                // We rely on reading serialized size == 0 (LEGACY_END_OF_SEGMENT_MARKER) to identify the end
+                // of a segment, which happens naturally due to the 0 padding of the empty segment on creation.
+                // However, with 2.1 era commitlogs it's possible that the last mutation ended less than 4 bytes 
+                // from the end of the file, which means that we'll be unable to read an a full int and instead 
+                // read an EOF here
+                if(end - reader.getFilePointer() < 4)
+                {
+                    logger.trace("Not enough bytes left for another mutation in this CommitLog segment, continuing");
+                    return false;
+                }
+
                 // any of the reads may hit EOF
                 serializedSize = reader.readInt();
                 if (serializedSize == LEGACY_END_OF_SEGMENT_MARKER)
diff --git a/test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java b/test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java
index 9999b42f41..9b63885b9c 100644
--- a/test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java
+++ b/test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java
@@ -136,13 +136,25 @@ public class CommitLogTest
         testRecoveryWithBadSizeArgument(100, 10);
     }
 
+    @Test
+    public void testRecoveryWithShortPadding() throws Exception
+    {
+        // If we have 0-3 bytes remaining, commitlog replayer
+        // should pass, because there's insufficient room
+        // left in the segment for the legacy size marker.
+        testRecovery(new byte[1], null);
+        testRecovery(new byte[2], null);
+        testRecovery(new byte[3], null);
+    }
+
     @Test
     public void testRecoveryWithShortSize() throws Exception
     {
-        runExpecting(new WrappedRunnable() {
-            public void runMayThrow() throws Exception
-            {
-                testRecovery(new byte[2], CommitLogDescriptor.VERSION_20);
+        runExpecting(new WrappedRunnable()  {
+            public void runMayThrow() throws Exception {
+                byte[] data = new byte[5];
+                data[3] = 1; // Not a legacy marker, give it a fake (short) size
+                testRecovery(data, CommitLogDescriptor.VERSION_20);
             }
         }, CommitLogReplayException.class);
     }