GP-1620: Prevent reads of stale memory from reaching into scratch space.

This commit is contained in:
Dan 2022-01-20 14:51:36 -05:00
parent ba2eb53110
commit a716acc562
5 changed files with 109 additions and 20 deletions

View file

@ -75,6 +75,10 @@ public enum DBTraceUtils {
public int hashCode() {
return Objects.hash(offset, snap);
}
public boolean isScratch() {
return DBTraceUtils.isScratch(snap);
}
}
// TODO: Should this be in by default?
@ -311,6 +315,10 @@ public enum DBTraceUtils {
return a.isConnected(b) && !a.intersection(b).isEmpty();
}
public static boolean isScratch(long snap) {
return snap < 0;
}
public static <C extends Comparable<C>> int compareRanges(Range<C> a, Range<C> b) {
int result;
if (!a.hasLowerBound() && b.hasLowerBound()) {
@ -367,6 +375,7 @@ public enum DBTraceUtils {
/**
* TODO: Document me
*
* <p>
* Only call this method for entries which definitely intersect the given span
*
* @param data

View file

@ -45,7 +45,10 @@ class DBTraceMemoryBlockEntry extends DBAnnotatedObject {
return DBTraceUtils.tableName(TABLE_NAME, space, threadKey, frameLevel);
}
@DBAnnotatedField(column = LOCATION_COLUMN_NAME, indexed = true, codec = OffsetThenSnapDBFieldCodec.class)
@DBAnnotatedField(
column = LOCATION_COLUMN_NAME,
indexed = true,
codec = OffsetThenSnapDBFieldCodec.class)
private OffsetSnap location;
@DBAnnotatedField(column = BUFFER_COLUMN_NAME)
private long bufferKey = -1;
@ -73,6 +76,10 @@ class DBTraceMemoryBlockEntry extends DBAnnotatedObject {
return location.snap;
}
public boolean isScratch() {
return DBTraceUtils.isScratch(location.snap);
}
private int getBlockNumber() {
return Byte.toUnsignedInt(blockNum);
}
@ -82,6 +89,8 @@ class DBTraceMemoryBlockEntry extends DBAnnotatedObject {
assert loc.snap > location.snap;
DBTraceMemoryBlockEntry cp = space.blockStore.create();
cp.setLoc(loc);
space.blockCacheMostRecent.clear();
space.blockCacheMostRecent.put(loc, cp);
DBTraceMemoryBufferEntry myBuf = findAssignedBuffer();
if (myBuf == null) {
return cp;
@ -134,11 +143,11 @@ class DBTraceMemoryBlockEntry extends DBAnnotatedObject {
}
protected DBTraceMemoryBufferEntry findFreeBufferInFuture() throws IOException {
DBTraceMemoryBlockEntry prev = space.findSoonestBlockEntry(location, false);
if (prev == null) {
DBTraceMemoryBlockEntry next = space.findSoonestBlockEntry(location, false);
if (next == null) {
return null;
}
return findFreeBuffer(prev);
return findFreeBuffer(next);
}
protected DBTraceMemoryBufferEntry findFreeBuffer() throws IOException {

View file

@ -85,7 +85,7 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
protected final DBCachedObjectStore<DBTraceMemoryBufferEntry> bufferStore;
protected final DBCachedObjectStore<DBTraceMemoryBlockEntry> blockStore;
protected final DBCachedObjectIndex<OffsetSnap, DBTraceMemoryBlockEntry> blocksByOffset;
protected final Map<OffsetSnap, DBTraceMemoryBlockEntry> blockCache = CacheBuilder
protected final Map<OffsetSnap, DBTraceMemoryBlockEntry> blockCacheMostRecent = CacheBuilder
.newBuilder()
.removalListener(this::blockCacheEntryRemoved)
.maximumSize(10)
@ -495,7 +495,7 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
if (!inclusive) {
loc = new OffsetSnap(loc.offset, loc.snap - 1);
}
ent = blockCache.get(loc);
ent = blockCacheMostRecent.get(loc);
if (ent != null) {
return ent;
}
@ -504,13 +504,26 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
return null;
}
ent = it.next();
if (ent.getOffset() != loc.offset) {
if (ent.getOffset() != loc.offset || ent.isScratch() != loc.isScratch()) {
return null;
}
blockCache.put(loc, ent);
blockCacheMostRecent.put(loc, ent);
return ent;
}
/**
* Locate the soonest block entry for the given offset-snap pair
*
* <p>
* To qualify, the entry must have a snap greater than (or optionally equal to) that given and
* an offset exactly equal to that given. That is, it is the earliest in time, but most follow
* the given snap. Additionally, if the given snap is in scratch space, the found entry must
* also be in scratch space.
*
* @param loc the offset-snap pair
* @param inclusive true to allow equal snap
* @return the found entry, or null
*/
protected DBTraceMemoryBlockEntry findSoonestBlockEntry(OffsetSnap loc, boolean inclusive) {
Iterator<DBTraceMemoryBlockEntry> it;
if (inclusive) {
@ -524,7 +537,7 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
return null;
}
DBTraceMemoryBlockEntry next = it.next();
if (next.getOffset() != loc.offset) {
if (next.getOffset() != loc.offset || next.isScratch() != loc.isScratch()) {
return null;
}
return next;
@ -553,19 +566,19 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
}
ent = ent.copy(loc);
ent.setBytes(buf, dstOffset, maxLen);
blockCache.put(loc, ent);
return;
}
// (1) or (2a)
// (1) or (2b)
ent = blockStore.create();
ent.setLoc(loc);
blockCacheMostRecent.clear();
blockCacheMostRecent.put(loc, ent);
if (ent.cmpBytes(buf, dstOffset, maxLen) == 0) {
// Keep the entry, but don't allocate storage in a buffer
buf.position(buf.position() + maxLen);
return;
}
ent.setBytes(buf, dstOffset, maxLen);
blockCache.put(loc, ent);
}
protected static class OutSnap {
@ -580,13 +593,14 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
OutSnap lastSnap, Set<TraceAddressSnapRange> changed) throws IOException {
// NOTE: Do not leave the buffer advanced from here
int pos = buf.position();
// exclusive?
Iterator<DBTraceMemoryBlockEntry> it =
blocksByOffset.tail(new OffsetSnap(loc.offset, loc.snap + 1), true).values().iterator(); // exclusive
blocksByOffset.tail(new OffsetSnap(loc.offset, loc.snap + 1), true).values().iterator();
AddressSet remaining = new AddressSet(space.getAddress(loc.offset + dstOffset),
space.getAddress(loc.offset + dstOffset + maxLen - 1));
while (it.hasNext()) {
DBTraceMemoryBlockEntry next = it.next();
if (next.getOffset() != loc.offset) {
if (next.getOffset() != loc.offset || next.isScratch() != loc.isScratch()) {
break;
}
AddressSetView withState =
@ -904,6 +918,26 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
return BLOCK_SIZE;
}
protected boolean isCross(long lower, long upper) {
return lower < 0 && upper >= 0;
}
/**
* Determine the truncation snap if the given span and range include byte changes
*
* <p>
* Code units do not understand or accommodate changes in time, so the underlying bytes of the
* unit must be the same throughout its lifespan. Typically, units are placed with a desired
* creation snap, and then its life is extended into the future opportunistically. Thus, when
* truncating, we desire to keep the start snap, then search for the soonest byte change within
* the desired lifespan. Furthermore, we generally don't permit a unit to exist in both record
* and scratch spaces, i.e., it cannot span both the -1 and 0 snaps.
*
* @param span the desired lifespan
* @param range the address range covered
* @return the first snap that should be excluded, or {@link Long#MIN_VALUE} to indicate no
* change.
*/
public long getFirstChange(Range<Long> span, AddressRange range) {
assertInSpace(range);
long lower = DBTraceUtils.lowerEndpoint(span);
@ -911,21 +945,24 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
if (lower == upper) {
return Long.MIN_VALUE;
}
Range<Long> fwdOne = DBTraceUtils.toRange(lower + 1, upper);
boolean cross = isCross(lower, upper);
if (cross && lower == -1) {
return 0; // Avoid reversal of range end points.
}
Range<Long> fwdOne = DBTraceUtils.toRange(lower + 1, cross ? -1 : upper);
ByteBuffer buf1 = ByteBuffer.allocate(BLOCK_SIZE);
ByteBuffer buf2 = ByteBuffer.allocate(BLOCK_SIZE);
try (LockHold hold = LockHold.lock(lock.readLock())) {
for (TraceAddressSnapRange tasr : stateMapSpace.reduce(
TraceAddressSnapRangeQuery.intersecting(range, fwdOne)
.starting(
Rectangle2DDirection.BOTTOMMOST))
.starting(Rectangle2DDirection.BOTTOMMOST))
.orderedKeys()) {
AddressRange toExamine = range.intersect(tasr.getRange());
if (doCheckBytesChanged(tasr.getY1(), toExamine, buf1, buf2)) {
return tasr.getY1();
}
}
return Long.MIN_VALUE;
return cross ? 0 : Long.MIN_VALUE;
}
catch (IOException e) {
blockStore.dbError(e);
@ -948,7 +985,8 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
getBytes(snap, start, oldBytes);
// New in the sense that they're about to replace the old bytes
ByteBuffer newBytes = ByteBuffer.allocate(len);
if (snap != 0) {
// NB. Don't want to wrap to Long.MAX_VALUE, but also don't want to read from scratch
if (snap != 0 && snap != Long.MIN_VALUE) {
getBytes(snap - 1, start, newBytes);
newBytes.flip();
}
@ -1003,7 +1041,7 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
stateMapSpace.invalidateCache();
bufferStore.invalidateCache();
blockStore.invalidateCache();
blockCache.clear();
blockCacheMostRecent.clear();
}
}
}

View file

@ -201,6 +201,24 @@ public class DBTraceCodeManagerTest extends AbstractGhidraHeadlessIntegrationTes
}
}
@Test
public void testAddInstructionInScratch() throws CodeUnitInsertionException {
try (UndoableTransaction tid = b.startTransaction()) {
b.trace.getMemoryManager().putBytes(-5L, b.addr(0x4001), b.buf(0xaa));
TraceInstruction i4000 =
b.addInstruction(-10, b.addr(0x4000), b.language, b.buf(0xf4, 0));
assertEquals(Range.closed(-10L, -6L), i4000.getLifespan());
TraceInstruction i4004 =
b.addInstruction(-1, b.addr(0x4004), b.language, b.buf(0xf4, 0));
assertEquals(Range.closed(-1L, -1L), i4004.getLifespan());
TraceInstruction i4008 =
b.addInstruction(-10, b.addr(0x4008), b.language, b.buf(0xf4, 0));
assertEquals(Range.closed(-10L, -1L), i4008.getLifespan());
}
}
@Test
public void testPutBytesTruncatesInstruction() throws CodeUnitInsertionException {
try (UndoableTransaction tid = b.startTransaction()) {

View file

@ -708,6 +708,21 @@ public abstract class AbstractDBTraceMemoryManagerTest
assertArrayEquals(arr(0, 0, 9, 10, 5, 6, 7, 8, 3, 4, 17, 18, 0, 0), read.array());
}
@Test
public void testGetBytesCrossScratch() {
try (UndoableTransaction tid = b.startTransaction()) {
assertEquals(4, memory.putBytes(Long.MIN_VALUE, b.addr(0x4000), buf(1, 2, 3, 4)));
}
ByteBuffer read = buf(-1, -1, -1, -1);
assertEquals(4, memory.getBytes(1, b.addr(0x4000), read));
assertArrayEquals(arr(-1, -1, -1, -1), read.array());
read.position(0);
assertEquals(4, memory.getBytes(-1, b.addr(0x4000), read));
assertArrayEquals(arr(1, 2, 3, 4), read.array());
}
@Test
public void testPutBytesPackGetBytes() {
try (UndoableTransaction tid = b.startTransaction()) {