Skip to content

Commit

Permalink
Exception-free, branch-free PageCursor bounds-checks.
Browse files Browse the repository at this point in the history 
Performing an out-of-bounds memory access to a page via a page cursor now no longer throws an exception.
The reason is that we might have an inconsistent read that fools decoding logic computing wrong offsets for data, or to try and read more data than can fit on a page.
Previously, such out-of-bounds accesses would cause the PageCursor to throw an exception, but this is inconsiderate - it is not the fault of the decoding logic that it was fed wrong data and tricked into an out-of-bounds access.
So instead we now raise a boolean flag the cursor has experienced and out-of-bounds access, and this flag can then be checked after the decoder has checked that its read was consistent.
Then it is up to the decoder to make a decision about how to handle the fact that it did an out-of-bounds access.
At present, the record formats (where the decoding logic is) don't do anything, but this is just because this change is not yet complete.
I will make them do something in the next commit.
  • Loading branch information
@chrisvest
chrisvest committed Apr 7, 2016
1 parent 9af30da commit 7743afe
Show file tree
Hide file tree
Showing 13 changed files with 499 additions and 330 deletions.
8 changes: 8 additions & 0 deletions community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java
View file
Expand Up @@ -348,4 +348,12 @@ public interface PageCursor extends AutoCloseable
* @return The number of bytes actually copied.
*/
int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, int lengthInBytes );

/**
* Discern whether an out-of-bounds access has occurred since the last call to {@link #next()} or
* {@link #next(long)}, or since the last call to {@link #shouldRetry()} that returned {@code true}, or since the
* last call to this method.
* @return {@code true} if an access was out of bounds.
*/
boolean checkAndClearBoundsFlag();
}
14 changes: 7 additions & 7 deletions community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/CursorPool.java
View file
Expand Up @@ -22,12 +22,12 @@
final class CursorPool extends ThreadLocal<CursorPool.CursorSets>
{
private final MuninnPagedFile pagedFile;
private final int cachePageSize;
private final long victimPage;

public CursorPool( MuninnPagedFile pagedFile )
CursorPool( MuninnPagedFile pagedFile )
{
this.pagedFile = pagedFile;
this.cachePageSize = pagedFile.pageCache.pageSize();
this.victimPage = pagedFile.pageCache.victimPage;
}

@Override
Expand All @@ -36,7 +36,7 @@ protected CursorSets initialValue()
return new CursorSets();
}

public MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags )
MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags )
{
CursorSets cursorSets = get();
MuninnReadPageCursor cursor = cursorSets.readCursors;
Expand All @@ -54,12 +54,12 @@ public MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags )

private MuninnReadPageCursor createReadCursor( CursorSets cursorSets )
{
MuninnReadPageCursor cursor = new MuninnReadPageCursor( cursorSets, cachePageSize );
MuninnReadPageCursor cursor = new MuninnReadPageCursor( cursorSets, victimPage );
cursor.initialiseFile( pagedFile );
return cursor;
}

public MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags )
MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags )
{
CursorSets cursorSets = get();
MuninnWritePageCursor cursor = cursorSets.writeCursors;
Expand All @@ -77,7 +77,7 @@ public MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags )

private MuninnWritePageCursor createWriteCursor( CursorSets cursorSets )
{
MuninnWritePageCursor cursor = new MuninnWritePageCursor( cursorSets, cachePageSize );
MuninnWritePageCursor cursor = new MuninnWritePageCursor( cursorSets, victimPage );
cursor.initialiseFile( pagedFile );
return cursor;
}
Expand Down
92 changes: 40 additions & 52 deletions community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java
View file
Expand Up @@ -148,7 +148,9 @@ public class MuninnPageCache implements PageCache
private final int keepFree;
private final PageCacheTracer tracer;
private final MuninnPage[] pages;
private final AtomicInteger backgroundFlushPauseRequests;
// All PageCursors are initialised with their pointers pointing to the victim page. This way, we can do branch-free
// bounds checking of page accesses without fear of segfaulting newly allocated cursors.
final long victimPage;

// The freelist is a thread-safe linked-list of 2 types of objects. A link
// can either be a MuninnPage or a FreePage.
Expand Down Expand Up @@ -178,18 +180,13 @@ public class MuninnPageCache implements PageCache
// threads scheduling meta-data in the OS kernel.
private volatile boolean evictorParked;
private volatile IOException evictorException;
// The thread that does background flushing.
private volatile Thread flushThread;

// Flag for when page cache is closed - writes guarded by synchronized(this), reads can be unsynchronized
private volatile boolean closed;

// Only used by ensureThreadsInitialised while holding the monitor lock on this MuninnPageCache instance.
private boolean threadsInitialised;

// The accumulator for the flush task sleep debt. This is only accessed from the flush task.
private long sleepDebtNanos;

// 'true' (the default) if we should print any exceptions we get when unmapping a file.
private boolean printExceptionsOnClose;

Expand All @@ -209,12 +206,12 @@ public MuninnPageCache(
this.keepFree = Math.min( pagesToKeepFree, maxPages / 2 );
this.tracer = tracer;
this.pages = new MuninnPage[maxPages];
this.backgroundFlushPauseRequests = new AtomicInteger();
this.printExceptionsOnClose = true;

long alignment = swapperFactory.getRequiredBufferAlignment();
long expectedMaxMemory = ((long) maxPages) * cachePageSize; // cast to long prevents overflow
MemoryManager memoryManager = new MemoryManager( expectedMaxMemory, alignment );
this.victimPage = VictimPageReference.getVictimPage( cachePageSize );
Object pageList = null;
int pageIndex = maxPages;
while ( pageIndex --> 0 )
Expand Down Expand Up @@ -279,9 +276,15 @@ public synchronized PagedFile map( File file, int filePageSize, OpenOption... op
ensureThreadsInitialised();
if ( filePageSize > cachePageSize )
{
throw new IllegalArgumentException( "Cannot map files with a filePageSize (" +
filePageSize + ") that is greater than the cachePageSize (" +
cachePageSize + ")" );
throw new IllegalArgumentException(
"Cannot map files with a filePageSize (" + filePageSize + ") that is greater than the " +
"cachePageSize (" + cachePageSize + ")" );
}
if ( filePageSize < Long.BYTES )
{
throw new IllegalArgumentException(
"Cannot map files with a filePageSize (" + filePageSize + ") that is less than " +
Long.BYTES + " bytes" );
}
boolean createIfNotExists = false;
boolean truncateExisting = false;
Expand Down Expand Up @@ -544,8 +547,6 @@ public synchronized void close() throws IOException

interrupt( evictionThread );
evictionThread = null;
interrupt( flushThread );
flushThread = null;
}

private void interrupt( Thread thread )
Expand Down Expand Up @@ -788,7 +789,7 @@ private int parkUntilEvictionRequired( int keepFree )
for (;;)
{
parkEvictor( parkNanos );
if ( Thread.currentThread().isInterrupted() || closed )
if ( Thread.interrupted() || closed )
{
return 0;
}
Expand Down Expand Up @@ -841,34 +842,36 @@ int evictPages( int pageCountToEvict, int clockArm, EvictionRunEvent evictionRun
// This reduces the CPU load and power usage in such a
// scenario.
pageCountToEvict--;
boolean pageEvicted;
boolean pageEvicted = false;

try ( EvictionEvent evictionEvent = evictionRunEvent.beginEviction() )
{
pageEvicted = page.isLoaded() && evictPage( page, evictionEvent );
}

if ( pageEvicted )
{
Object current;
Object nextListHead;
FreePage freePage = null;
do
if ( pageEvicted )
{
current = getFreelistHead();
freePage = freePage == null?
new FreePage( page ) : freePage;
freePage.setNext( (FreePage) current );
nextListHead = freePage;
Object current;
Object nextListHead;
FreePage freePage = null;
do
{
current = getFreelistHead();
freePage = freePage == null?
new FreePage( page ) : freePage;
freePage.setNext( (FreePage) current );
nextListHead = freePage;
}
while ( !compareAndSetFreelistHead( current, nextListHead ) );
}
while ( !compareAndSetFreelistHead( current, nextListHead ) );
}
else
finally
{
// Pages we put into the free-list remain exclusively locked until a page fault unlocks them
// If we somehow failed to evict the page, then we need to make sure that we release the
// exclusive lock.
page.unlockExclusive();
if ( !pageEvicted )
{
// Pages we put into the free-list remain exclusively locked until a page fault unlocks
// them. If we somehow failed to evict the page, then we need to make sure that we release
// the exclusive lock.
page.unlockExclusive();
}
}
}
}
Expand All @@ -879,6 +882,10 @@ int evictPages( int pageCountToEvict, int clockArm, EvictionRunEvent evictionRun
return clockArm;
}

/**
* Evict the given page, or return {@code false} if the eviction failed for any reason.
* This method will never throw an exception!
*/
private boolean evictPage( MuninnPage page, EvictionEvent evictionEvent )
{
//noinspection TryWithIdenticalCatches - this warning is a false positive; bug in Intellij inspection
Expand Down Expand Up @@ -915,25 +922,6 @@ private void clearEvictorException()
}
}

void pauseBackgroundFlushTask()
{
backgroundFlushPauseRequests.getAndIncrement();
}

void unpauseBackgroundFlushTask()
{
backgroundFlushPauseRequests.getAndDecrement();
LockSupport.unpark( flushThread );
}

private void checkBackgroundFlushPause()
{
while ( backgroundFlushPauseRequests.get() > 0 )
{
LockSupport.parkNanos( TimeUnit.MILLISECONDS.toNanos( 10 ) );
}
}

@Override
public String toString()
{
Expand Down

0 comments on commit 7743afe

Please sign in to comment.