From f5aefc69ba6a2e3c80d49b47d41f17ecd0134cf6 Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Tue, 29 Mar 2016 13:07:43 +0200 Subject: [PATCH] Make CommonAbstractStore check page cursor bounds --- .../impl/store/CommonAbstractStore.java | 71 +++- .../neo4j/kernel/impl/store/TokenStore.java | 2 +- .../CommonAbstractStoreBehaviourTest.java | 321 ++++++++++++++++++ 3 files changed, 387 insertions(+), 7 deletions(-) create mode 100644 community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java index 86a093f8cc6e0..b004b5ec58095 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java @@ -204,6 +204,12 @@ protected void initialiseNewStoreFile( PagedFile file ) throws IOException createHeaderRecord( pageCursor ); } while ( pageCursor.shouldRetry() ); + if ( pageCursor.checkAndClearBoundsFlag() ) + { + throw new UnderlyingStorageException( + "Out of page bounds when writing header; page size too small: " + + pageCache.pageSize() + " bytes."); + } } } } @@ -216,6 +222,27 @@ protected void initialiseNewStoreFile( PagedFile file ) throws IOException idGeneratorFactory.create( idFileName, getNumberOfReservedLowIds(), true ); } + protected void checkForOutOfBounds( PageCursor pageCursor, long recordId ) + { + if ( pageCursor.checkAndClearBoundsFlag() ) + { + throwOutOfBoundsException( recordId ); + } + } + + private void throwOutOfBoundsException( long recordId ) + { + RECORD record = newRecord(); + record.setId( recordId ); + long pageId = pageIdForRecord( recordId ); + int offset = offsetForId( recordId ); + throw new UnderlyingStorageException( + "Access to record " + record + " went out of bounds of the page. The record size is " + + recordSize + " bytes, and the access was at offset " + offset + " bytes into page " + + pageId + ", and the pages have a capacity of " + storeFile.pageSize() + " bytes. " + + "The mapped store file in question is " + storageFileName.getAbsolutePath() ); + } + protected void createHeaderRecord( PageCursor cursor ) throws IOException { int offset = cursor.getOffset(); @@ -263,6 +290,12 @@ private void extractHeaderRecord() throws IOException readHeaderAndInitializeRecordFormat( pageCursor ); } while ( pageCursor.shouldRetry() ); + if ( pageCursor.checkAndClearBoundsFlag() ) + { + throw new UnderlyingStorageException( + "Out of page bounds when reading header; page size too small: " + + pageCache.pageSize() + " bytes."); + } } } } @@ -293,8 +326,8 @@ public int getRecordsPerPage() public byte[] getRawRecordData( long id ) throws IOException { byte[] data = new byte[recordSize]; - long pageId = RecordPageLocationCalculator.pageIdForRecord( id, storeFile.pageSize(), recordSize ); - int offset = RecordPageLocationCalculator.offsetForId( id, storeFile.pageSize(), recordSize ); + long pageId = pageIdForRecord( id ); + int offset = offsetForId( id ); try ( PageCursor pageCursor = storeFile.io( pageId, PagedFile.PF_SHARED_READ_LOCK ) ) { if ( pageCursor.next() ) @@ -305,6 +338,7 @@ public byte[] getRawRecordData( long id ) throws IOException pageCursor.getBytes( data ); } while ( pageCursor.shouldRetry() ); + checkForOutOfBounds( pageCursor, id ); } } return data; @@ -365,6 +399,7 @@ public boolean isInUse( long id ) recordIsInUse = isInUse( cursor ); } while ( cursor.shouldRetry() ); + checkForOutOfBounds( cursor, id ); } return recordIsInUse; } @@ -396,7 +431,7 @@ final void rebuildIdGenerator() openIdGenerator(); long defraggedCount = 0; - boolean fastRebuild = doFastIdGeneratorRebuild(); + boolean fastRebuild = isOnlyFastIdGeneratorRebuildEnabled( configuration ); try { @@ -426,6 +461,11 @@ final void rebuildIdGenerator() } } + protected boolean isOnlyFastIdGeneratorRebuildEnabled( Config config ) + { + return config.get( Configuration.rebuild_idgenerators_fast ); + } + private long rebuildIdGeneratorSlow( PageCursor cursor, int recordsPerPage, int blockSize, long foundHighId ) throws IOException @@ -469,6 +509,7 @@ else if ( isRecordReserved( cursor ) ) } } while ( cursor.shouldRetry() ); + checkIdScanCursorBounds( cursor ); for ( int i = 0; i < defragged; i++ ) { @@ -480,9 +521,14 @@ else if ( isRecordReserved( cursor ) ) return defragCount; } - protected boolean doFastIdGeneratorRebuild() + private void checkIdScanCursorBounds( PageCursor cursor ) { - return configuration.get( Configuration.rebuild_idgenerators_fast ); + if ( cursor.checkAndClearBoundsFlag() ) + { + throw new UnderlyingStorageException( + "Out of bounds access on page " + cursor.getCurrentPageId() + " detected while scanning the " + + storageFileName + " file for deleted records" ); + } } /** @@ -653,11 +699,14 @@ protected long scanForHighId() long nextPageId = storeFile.getLastPageId(); int recordsPerPage = getRecordsPerPage(); int recordSize = getRecordSize(); + long highestId = getNumberOfReservedLowIds(); while ( nextPageId >= 0 && cursor.next( nextPageId ) ) { nextPageId--; + boolean found; do { + found = false; int currentRecord = recordsPerPage; while ( currentRecord-- > 0 ) { @@ -671,12 +720,19 @@ protected long scanForHighId() if ( !justLegacyStoreTrailer ) { // We've found the highest id in use - return recordId + 1 /*+1 since we return the high id*/; + found = true; + highestId = recordId + 1; /*+1 since we return the high id*/; + break; } } } } while ( cursor.shouldRetry() ); + checkIdScanCursorBounds( cursor ); + if ( found ) + { + return highestId; + } } return getNumberOfReservedLowIds(); @@ -986,6 +1042,7 @@ public RECORD getRecord( long id, RECORD record, RecordLoad mode ) recordFormat.read( record, cursor, mode, recordSize, storeFile ); } while ( cursor.shouldRetry() ); + checkForOutOfBounds( cursor, id ); verifyAfterReading( record, mode ); } else @@ -1018,6 +1075,7 @@ public void updateRecord( RECORD record ) recordFormat.write( record, cursor, recordSize, storeFile ); } while ( cursor.shouldRetry() ); + checkForOutOfBounds( cursor, id ); // We don't free ids if something weird goes wrong if ( !record.inUse() ) { freeId( id ); @@ -1112,6 +1170,7 @@ public boolean next() recordFormat.read( record, pageCursor, mode, recordSize, storeFile ); } while ( pageCursor.shouldRetry() ); + checkForOutOfBounds( pageCursor, currentId ); verifyAfterReading( record, mode ); } else diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TokenStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TokenStore.java index 65e6fdda3ed82..c0491bb43ca68 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TokenStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/TokenStore.java @@ -76,7 +76,7 @@ public DynamicStringStore getNameStore() } @Override - protected boolean doFastIdGeneratorRebuild() + protected boolean isOnlyFastIdGeneratorRebuildEnabled( Config config ) { return false; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java new file mode 100644 index 0000000000000..b37faee16ff8b --- /dev/null +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java @@ -0,0 +1,321 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.impl.store; + +import org.junit.After; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.RuleChain; +import org.junit.rules.TestRule; + +import java.io.File; +import java.io.IOException; +import java.util.Map; +import java.util.Queue; +import java.util.concurrent.ConcurrentLinkedQueue; + +import org.neo4j.function.ThrowingAction; +import org.neo4j.graphdb.factory.GraphDatabaseSettings; +import org.neo4j.io.pagecache.PageCache; +import org.neo4j.io.pagecache.PageCursor; +import org.neo4j.io.pagecache.PagedFile; +import org.neo4j.kernel.configuration.Config; +import org.neo4j.kernel.impl.store.format.BaseRecordFormat; +import org.neo4j.kernel.impl.store.id.DefaultIdGeneratorFactory; +import org.neo4j.kernel.impl.store.id.IdType; +import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; +import org.neo4j.kernel.impl.store.record.RecordLoad; +import org.neo4j.logging.NullLogProvider; +import org.neo4j.test.EphemeralFileSystemRule; +import org.neo4j.test.PageCacheRule; + +import static org.junit.Assert.fail; +import static org.neo4j.helpers.collection.MapUtil.stringMap; +import static org.neo4j.kernel.impl.store.record.RecordLoad.NORMAL; + +/** + * Test for {@link CommonAbstractStore}, but without using mocks. + * @see CommonAbstractStoreTest for testing with mocks. + */ +public class CommonAbstractStoreBehaviourTest +{ + /** + * Note that tests MUST use the non-modifying {@link Config#with(Map, Class[])} method, to make alternate copies + * of this settings class. + */ + private static final Config CONFIG = Config.empty().augment( stringMap( + GraphDatabaseSettings.pagecache_memory.name(), "8M" ) ); + + private final EphemeralFileSystemRule fs = new EphemeralFileSystemRule(); + private final PageCacheRule pageCacheRule = new PageCacheRule(); + + @Rule + public final TestRule rules = RuleChain.outerRule( fs ).around( pageCacheRule ); + + private final Queue nextPageId = new ConcurrentLinkedQueue<>(); + private final Queue nextPageOffset = new ConcurrentLinkedQueue<>(); + private int intsPerRecord = 1; + + private MyStore store; + private Config config = CONFIG; + + @After + public void tearDown() + { + if ( store != null ) + { + store.close(); + store = null; + } + nextPageOffset.clear(); + nextPageId.clear(); + } + + private void assertThrows( ThrowingAction action ) throws Exception + { + try + { + action.apply(); + fail( "expected an exception" ); + } + catch ( UnderlyingStorageException exception ) + { + // Good! + } + } + + private void verifyExceptionOnAccess( ThrowingAction access ) throws Exception + { + createStore(); + nextPageOffset.add( 8190 ); + assertThrows( access ); + } + + private void createStore() + { + store = new MyStore( config, pageCacheRule.getPageCache( fs.get(), config ) ); + store.initialise( true ); + } + + @Test + public void writingOfHeaderRecordDuringInitialiseNewStoreFileMustThrowOnPageOverflow() throws Exception + { + // 16-byte header will overflow an 8-byte page size + Config config = CONFIG.with( stringMap( GraphDatabaseSettings.mapped_memory_page_size.name(), "8" ) ); + MyStore store = new MyStore( config, pageCacheRule.getPageCache( fs.get(), config ) ); + assertThrows( () -> store.initialise( true ) ); + } + + @Test + public void extractHeaderRecordDuringLoadStorageMustThrowOnPageOverflow() throws Exception + { + MyStore first = new MyStore( config, pageCacheRule.getPageCache( fs.get(), config ) ); + first.initialise( true ); + first.close(); + + config = CONFIG.with( stringMap( GraphDatabaseSettings.mapped_memory_page_size.name(), "8" ) ); + MyStore second = new MyStore( config, pageCacheRule.getPageCache( fs.get(), config ) ); + assertThrows( () -> second.initialise( false ) ); + } + + @Test + public void getRawRecordDataMustThrowOnPageOverflow() throws Exception + { + verifyExceptionOnAccess( () -> store.getRawRecordData( 5 ) ); + } + + @Test + public void isInUseMustThrowOnPageOverflow() throws Exception + { + verifyExceptionOnAccess( () -> store.isInUse( 5 ) ); + } + + @Test + public void getRecordMustThrowOnPageOverflow() throws Exception + { + verifyExceptionOnAccess( () -> store.getRecord( 5, new IntRecord( 5 ), NORMAL ) ); + } + + @Test + public void updateRecordMustThrowOnPageOverflow() throws Exception + { + verifyExceptionOnAccess( () -> store.updateRecord( new IntRecord( 5 ) ) ); + } + + @Test + public void recordCursorNextMustThrowOnPageOverflow() throws Exception + { + verifyExceptionOnAccess( () -> { + try ( RecordCursor cursor = store.newRecordCursor( new IntRecord( 0 ) ).acquire( 5, NORMAL ) ) + { + cursor.next(); + } + } ); + } + + @Test + public void rebuildIdGeneratorSlowMustThrowOnPageOverflow() throws Exception + { + config = config.with( stringMap( + CommonAbstractStore.Configuration.rebuild_idgenerators_fast.name(), "false" ) ); + createStore(); + store.setStoreNotOk( new Exception() ); + IntRecord record = new IntRecord( 200 ); + record.value = 0xCAFEBABE; + store.updateRecord( record ); + intsPerRecord = 8192; + assertThrows( () -> store.makeStoreOk() ); + } + + @Test + public void scanForHighIdMustThrowOnPageOverflow() throws Exception + { + createStore(); + store.setStoreNotOk( new Exception() ); + IntRecord record = new IntRecord( 200 ); + record.value = 0xCAFEBABE; + store.updateRecord( record ); + intsPerRecord = 8192; + assertThrows( () -> store.makeStoreOk() ); + } + + private static class IntRecord extends AbstractBaseRecord + { + public int value; + + IntRecord( long id ) + { + super( id ); + setInUse( true ); + } + + @Override + public String toString() + { + return "IntRecord[" + getId() + "](" + value + ")"; + } + } + + private static class LongLongHeader implements StoreHeader + { + } + + private class MyFormat extends BaseRecordFormat implements StoreHeaderFormat + { + MyFormat() + { + super( (x) -> 4, 8, 32 ); + } + + @Override + public IntRecord newRecord() + { + return new IntRecord( 0 ); + } + + @Override + public boolean isInUse( PageCursor cursor ) + { + boolean inUse = false; + for ( int i = 0; i < intsPerRecord; i++ ) + { + inUse |= cursor.getInt() != 0; + } + return inUse; + } + + @Override + public void read( IntRecord record, PageCursor cursor, RecordLoad mode, int recordSize, PagedFile storeFile ) + throws IOException + { + for ( int i = 0; i < intsPerRecord; i++ ) + { + record.value = cursor.getInt(); + } + record.setInUse( true ); + } + + @Override + public void write( IntRecord record, PageCursor cursor, int recordSize, PagedFile storeFile ) throws IOException + { + for ( int i = 0; i < intsPerRecord; i++ ) + { + cursor.putInt( record.value ); + } + } + + @Override + public int numberOfReservedRecords() + { + return 4; // 2 longs occupy 4 int records + } + + @Override + public void writeHeader( PageCursor cursor ) + { + cursor.putLong( 0xA5A5A5_7E7E7EL ); + cursor.putLong( 0x3B3B3B_1A1A1AL ); + } + + @Override + public LongLongHeader readHeader( PageCursor cursor ) + { + LongLongHeader header = new LongLongHeader(); + cursor.getLong(); // pretend to read fields into the header + cursor.getLong(); + return header; + } + } + + private class MyStore extends CommonAbstractStore + { + MyStore( Config config, PageCache pageCache ) + { + this( config, pageCache, new MyFormat() ); + } + + MyStore( Config config, PageCache pageCache, MyFormat format ) + { + super( new File( "store" ), config, IdType.NODE, new DefaultIdGeneratorFactory( fs.get() ), pageCache, + NullLogProvider.getInstance(), "T", format, format, "XYZ" ); + } + + @Override + public void accept( Processor processor, IntRecord record ) + throws FAILURE + { + throw new UnsupportedOperationException(); + } + + @Override + protected long pageIdForRecord( long id ) + { + Long override = nextPageId.poll(); + return override != null ? override : super.pageIdForRecord( id ); + } + + @Override + protected int offsetForId( long id ) + { + Integer override = nextPageOffset.poll(); + return override != null ? override : super.offsetForId( id ); + } + } +}