Author: frm Date: Tue Sep 4 09:10:25 2018 New Revision: 1840024 URL: http://svn.apache.org/viewvc?rev=1840024&view=rev Log: OAK-7721 - Check for too big records when allocating space
SegmentBufferWriter#prepare might allocate too much space to records and create a buffer which can't be serialized when SegmentBufferWriter#flush is invoked. SegmentBufferWriter#prepare now checks if the record being written is too big to fit in a segment, fails early and loud if the check fails, and prevents the buffer from being corrupted. Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriterTest.java Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java?rev=1840024&r1=1840023&r2=1840024&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java (original) +++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java Tue Sep 4 09:10:25 2018 @@ -426,17 +426,36 @@ public class SegmentBufferWriter impleme segmentSize = align(headerSize + recordSize + length, 16); } + // If the resulting segment buffer would be too big we need to allocate + // additional space. Allocating additional space is a recursive + // operation guarded by the `dirty` flag. The recursion can iterate at + // most two times. The base case happens when the `dirty` flag is + // `false`: the current buffer is empty, the record is too big to fit in + // an empty segment, and we fail with an `IllegalArgumentException`. The + // recursive step happens when the `dirty` flag is `true`: + // the current buffer is non-empty, we flush it, allocate a new buffer + // for an empty segment, and invoke `prepare()` once more. + if (segmentSize > buffer.length) { - LOG.debug("Flushing full segment {} (headerSize={}, recordSize={}, length={}, segmentSize={})", + if (dirty) { + LOG.debug("Flushing full segment {} (headerSize={}, recordSize={}, length={}, segmentSize={})", segment.getSegmentId(), headerSize, recordSize, length, segmentSize); - flush(store); + flush(store); + return prepare(type, size, ids, store); + } + throw new IllegalArgumentException(String.format( + "Record too big: type=%s, size=%s, recordIds=%s, total=%s", + type, + size, + ids.size(), + recordSize + )); } statistics.recordCount++; length += recordSize; position = buffer.length - length; - checkState(position >= 0); int recordNumber = recordNumbers.addRecord(type, position); return new RecordId(segment.getSegmentId(), recordNumber); Modified: jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriterTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriterTest.java?rev=1840024&r1=1840023&r2=1840024&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriterTest.java (original) +++ jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriterTest.java Tue Sep 4 09:10:25 2018 @@ -24,7 +24,9 @@ import static org.junit.Assert.assertEqu import static org.junit.Assert.assertNotEquals; import java.io.File; +import java.util.Collections; import java.util.List; +import java.util.Optional; import org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState; import org.apache.jackrabbit.oak.segment.file.FileStore; @@ -96,4 +98,42 @@ public class SegmentBufferWriterTest { assertNotEquals(before, after); } + @Test + public void tooBigRecord() throws Exception { + + // See OAK-7721 to understand why this test exists. + + try (FileStore store = openFileStore()) { + + // Please don't change anything from the following statement yet. + // Read the next comment to understand why. + + SegmentBufferWriter writer = new SegmentBufferWriter( + store.getSegmentIdProvider(), + store.getReader(), + "t", + store.getRevisions().getHead().getSegment().getGcGeneration() + ); + + // The size of the record is chosen with the precise intention to + // fool `writer` into having enough space to write the record. In + // particular, at the end of `prepare()`, `writer` will have + // `this.length = 262144`, which is `MAX_SEGMENT_SIZE`, and + // `this.position = 0`. This result is particularly sensitive to the + // initial content of the segment, which in turn is influenced by + // the segment info. Try to change the writer ID in the constructor + // of `SegmentBufferWriter` to a longer string, and you will have + // `prepare()` throw ISEs because the writer ID is embedded in the + // segment info. + + Optional<IllegalArgumentException> error = Optional.empty(); + try { + writer.prepare(RecordType.BLOCK, 262101, Collections.emptyList(), store); + } catch (IllegalArgumentException e) { + error = Optional.of(e); + } + assertEquals("Record too big: type=BLOCK, size=262101, recordIds=0, total=262104", error.map(Exception::getMessage).orElse(null)); + } + } + }